bc-lee added inline comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:1970
+ but this behavior is changed by another option,
+ ``JavaStaticImportAfterImport``.
----------------
MyDeveloperDay wrote:
> Can you add a test that shows if the sorting is still in the groups, i.e. I
> can't tell if
>
> ```
> import com.test.a;
> import static org.a;
>
> import com.test.b;
> import static org.b;
> ```
>
> becomes
>
> ```
> import static org.a;
> import static org.b;
> import com.test.a;
>
> import com.test.b;
> ```
>
> or
>
> ```
> import static org.a;
> import static org.b;
>
> import com.test.a;
>
> import com.test.b;
> ```
>
> or
>
> ```
> import static org.a;
> import com.test.a;
>
> import static org.b;
> import com.test.b;
>
> ```
>
>
>
>
>
Done.
================
Comment at: clang/include/clang/Format/Format.h:1708
+ /// \endcode
+ bool JavaStaticImportAfterImport;
+
----------------
JakeMerdichAMD wrote:
> 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.
If `SortIncludes` is `true`, it doesn't make sense to not touch Java static
include.
Making it an enum is also good for me.
================
Comment at: clang/unittests/Format/SortImportsTestJava.cpp:253
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+ FmtStyle.JavaStaticImportAfterImport = true;
----------------
JakeMerdichAMD wrote:
> 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.
Done
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
