JakeMerdichAMD added inline comments.
================
Comment at: clang/include/clang/Format/Format.h:1708
+ /// \endcode
+ bool JavaStaticImportAfterImport;
+
----------------
MyDeveloperDay wrote:
> Can we consider changing the name or the option to make it clearer what its
> for?
>
> `SortJavaStaticImport`
>
> and make it an enumeration rather than true/false
>
> `Before,After,Never`
>
> There need to be a "don't touch it option (.e.g. Never)" that does what it
> does today (and that should be the default, otherwise we end up causing
> clang-format to change ALL java code" which could be 100's of millions of
> lines+
>
>
I'm confused, there is not currently a never option... Right now the behavior
is always 'before', there is no 'after' or 'never'.
Making it an enum would probably be more ergonomic though, even there is no
never option.
================
Comment at: clang/lib/Format/Format.cpp:906
LLVMStyle.JavaScriptWrapImports = true;
+ LLVMStyle.JavaStaticImportAfterImport = false;
LLVMStyle.TabWidth = 8;
----------------
MyDeveloperDay wrote:
> We can't have a default that does will cause a change
Not a default change, see previous comment for discussion.
================
Comment at: clang/unittests/Format/SortImportsTestJava.cpp:253
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+ FmtStyle.JavaStaticImportAfterImport = true;
----------------
MyDeveloperDay wrote:
> please test all options
>
> You are also missing a test for checking the PARSING of the options
Parsing test was already noted and done (unless this changes to an enum of
course). But testing the 'false' case here makes sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87201/new/
https://reviews.llvm.org/D87201
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
