git cl format and checkstyle don't agree on generics formatting |
|||||||
Issue description
$ git cl presubmit -u
Running presubmit upload checks ...
** Presubmit Warnings **
The src directory requires source formatting. Please run git cl format .
** Presubmit ERRORS **
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:808:65: GenericWhitespace '>' is followed by whitespace.
$ git cl format && git diff
@@ -805,6 +805,6 @@ private static void verifyAction(
verify(suggestionsSource,
(action == ContentSuggestionsAdditionalAction.FETCH ? times(1) : never()))
.fetchSuggestions(anyInt(), any(String[].class),
- Mockito.<Callback<List<SnippetArticle>>> any());
+ Mockito.< Callback < List<SnippetArticle>>> any());
}
}
$ git commit --amend -a -C HEAD && git cl presubmit -u
Running presubmit upload checks ...
** Presubmit ERRORS **
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:808:34: GenericWhitespace '<' is followed by whitespace.
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:808:43: GenericWhitespace '<' is preceded with whitespace.
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:808:45: GenericWhitespace '<' is followed by whitespace.
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:808:68: GenericWhitespace '>' is followed by whitespace.
,
Jul 7 2017
Yep, I agree with jbudorick, this needs to be fixed by Android folks. The GenericWhitespace messages are android-specific, and are generated by checkstyle, which is pulled in via DEPS: https://cs.chromium.org/chromium/src/third_party/leakcanary/src/checkstyle.xml?q=GenericWhitespace&sq=package:chromium&dr=C If they disagree with git-cl-format (which just calls through to clang-format, also pulled in via chromium DEPS https://cs.chromium.org/chromium/src/buildtools/clang_format/), then that needs to be fixed by the maintainers of checkstyle and/or its configuration.
,
Jul 10 2017
In this specific case, I'd say clang-format is wrong, probably mixing up generics usage with comparison operators. At least that's not a usual Java style and checkstyle is right to flag it. But yes, having the two systems is frustrating. Maybe the fix would be just to remove the clang-format check from submit checks? It would only run at upload time so the CL author can choose to override that? I'd say checkstyle targets Java primarily unlike clang-format, so it might be correct more often and could keep running on the bots.
,
Sep 25 2017
,
Sep 26 2017
I debugged this a bit, I think the root cause is that clang-format sees the >>> and figures it's a logical right shift op and then can't match that one token to the three < tokens. If the input is
void foo() { Mockito.<Callback<List<SnippetArticle> > > any(); }
then the output is correct O_o
Looking more…
,
Sep 26 2017
Turns out this is a recent regression (from a patch that I reviewed shame shame shame). Fix attempt: https://reviews.llvm.org/D38291
,
Sep 27 2017
http://llvm.org/viewvc/llvm-project?view=revision&revision=314325
,
Dec 4 2017
Moving Infra>Client>Android -> Infra>Client>Chrome+OS=Android
,
Dec 4 2017
,
Nov 26
A clang-format with the fix got deployed on Nov 21 2018. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by jbudorick@chromium.org
, Jul 7 2017