[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee added a comment. Thanks for the review. Since I don't have commit access, so I want someone else to apply this patch. 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
This revision was automatically updated to reflect the committed changes. Closed by commit rG2e7add812eb7: [clang-format] Add a option for the position of Java static import (authored by MyDeveloperDay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87201/new/ https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/FormatTest.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,62 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, SortJavaStaticImport) { + FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before; + EXPECT_EQ("import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n" +"\n" +"import com.test.b;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n")); + + FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After; + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + +TEST_F(SortImportsTestJava, SortJavaStaticImportAsGroup) { + FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After; + + EXPECT_EQ("import com.test.a;\n" +"import com.test.b;\n" +"\n" +"import static org.a;\n" +"import static org.b;\n", +sort("import com.test.a;\n" + "import static org.a;\n" + "import com.test.b;\n" + "import static org.b;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/unittests/Format/FormatTest.cpp === --- clang/unittests/Format/FormatTest.cpp +++ clang/unittests/Format/FormatTest.cpp @@ -14328,6 +14328,12 @@ CHECK_PARSE("BitFieldColonSpacing: After", BitFieldColonSpacing, FormatStyle::BFCS_After); + Style.SortJavaStaticImport = FormatStyle::SJSIO_Before; + CHECK_PARSE("SortJavaStaticImport: After", SortJavaStaticImport, + FormatStyle::SJSIO_After); + CHECK_PARSE("SortJavaStaticImport: Before", SortJavaStaticImport, + FormatStyle::SJSIO_Before); + // FIXME: This is required because parsing a configuration simply overwrites // the first N elements of the list instead of resetting it. Style.ForEachMacros.clear(); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -377,6 +377,15 @@ } }; +template <> +struct ScalarEnumerationTraits { + static void enumeration(IO , + FormatStyle::SortJavaStaticImportOptions ) { +IO.enumCase(Value, "Before", FormatStyle::SJSIO_Before); +IO.enumCase(Value, "After", FormatStyle::SJSIO_After); + } +}; + template <> struct MappingTraits { static void mapping(IO , FormatStyle ) { // When reading, read the language first, we need it for getPredefinedStyle. @@ -574,6 +583,7 @@ IO.mapOptional("RawStringFormats", Style.RawStringFormats); IO.mapOptional("ReflowComments", Style.ReflowComments); IO.mapOptional("SortIncludes", Style.SortIncludes); +IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport); IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations); IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast); IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot); @@ -947,6 +957,7 @@ LLVMStyle.DisableFormat = false; LLVMStyle.SortIncludes = true; + LLVMStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before; LLVMStyle.SortUsingDeclarations = true;
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee added a comment. Can someone commit my changes on behalf of it? 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee marked an inline comment as done. bc-lee added a comment. It's okay for some reviewers to make this change on my behalf. Thanks for reviewing this! 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. Sorry on the delay, LGTM too. It looks like you're a first time contributor and probably don't have write access to the repo, do you want one of us to push this on your behalf? If you intend to have more commits in the future, write access to github isn't hard to get (just request it once you have a few changes in). 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. LGTM, I'm happy if @JakeMerdichAMD is Comment at: clang/include/clang/Format/Format.h:1708 + /// \endcode + bool JavaStaticImportAfterImport; + bc-lee wrote: > 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. > true.. it will sort it I assume one way or the other 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee updated this revision to Diff 290619. bc-lee added a comment. Add more tests and rename options. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87201/new/ https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/FormatTest.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,62 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, SortJavaStaticImport) { + FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before; + EXPECT_EQ("import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n" +"\n" +"import com.test.b;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n")); + + FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After; + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + +TEST_F(SortImportsTestJava, SortJavaStaticImportAsGroup) { + FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After; + + EXPECT_EQ("import com.test.a;\n" +"import com.test.b;\n" +"\n" +"import static org.a;\n" +"import static org.b;\n", +sort("import com.test.a;\n" + "import static org.a;\n" + "import com.test.b;\n" + "import static org.b;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/unittests/Format/FormatTest.cpp === --- clang/unittests/Format/FormatTest.cpp +++ clang/unittests/Format/FormatTest.cpp @@ -14276,6 +14276,12 @@ CHECK_PARSE("BitFieldColonSpacing: After", BitFieldColonSpacing, FormatStyle::BFCS_After); + Style.SortJavaStaticImport = FormatStyle::SJSIO_Before; + CHECK_PARSE("SortJavaStaticImport: After", SortJavaStaticImport, + FormatStyle::SJSIO_After); + CHECK_PARSE("SortJavaStaticImport: Before", SortJavaStaticImport, + FormatStyle::SJSIO_Before); + // FIXME: This is required because parsing a configuration simply overwrites // the first N elements of the list instead of resetting it. Style.ForEachMacros.clear(); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -377,6 +377,15 @@ } }; +template <> +struct ScalarEnumerationTraits { + static void enumeration(IO , + FormatStyle::SortJavaStaticImportOptions ) { +IO.enumCase(Value, "Before", FormatStyle::SJSIO_Before); +IO.enumCase(Value, "After", FormatStyle::SJSIO_After); + } +}; + template <> struct MappingTraits { static void mapping(IO , FormatStyle ) { // When reading, read the language first, we need it for getPredefinedStyle. @@ -574,6 +583,7 @@ IO.mapOptional("RawStringFormats", Style.RawStringFormats); IO.mapOptional("ReflowComments", Style.ReflowComments); IO.mapOptional("SortIncludes", Style.SortIncludes); +IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport); IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations); IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast); IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot); @@ -947,6 +957,7 @@ LLVMStyle.DisableFormat = false; LLVMStyle.SortIncludes = true; + LLVMStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before; LLVMStyle.SortUsingDeclarations = true; LLVMStyle.StatementMacros.push_back("Q_UNUSED"); LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION"); @@
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:1970 + but this behavior is changed by another option, + ``JavaStaticImportAfterImport``. 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; ``` Comment at: clang/include/clang/Format/Format.h:1708 + /// \endcode + bool JavaStaticImportAfterImport; + 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+ Comment at: clang/lib/Format/Format.cpp:906 LLVMStyle.JavaScriptWrapImports = true; + LLVMStyle.JavaStaticImportAfterImport = false; LLVMStyle.TabWidth = 8; We can't have a default that does will cause a change Comment at: clang/unittests/Format/SortImportsTestJava.cpp:253 +TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) { + FmtStyle.JavaStaticImportAfterImport = true; please test all options You are also missing a test for checking the PARSING of the options 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee updated this revision to Diff 290381. bc-lee added a comment. Fix the example to match the description. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87201/new/ https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/FormatTest.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,30 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) { + FmtStyle.JavaStaticImportAfterImport = true; + + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/unittests/Format/FormatTest.cpp === --- clang/unittests/Format/FormatTest.cpp +++ clang/unittests/Format/FormatTest.cpp @@ -13929,6 +13929,7 @@ CHECK_PARSE_BOOL(IndentCaseBlocks); CHECK_PARSE_BOOL(IndentGotoLabels); CHECK_PARSE_BOOL(IndentWrappedFunctionNames); + CHECK_PARSE_BOOL(JavaStaticImportAfterImport); CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks); CHECK_PARSE_BOOL(ObjCSpaceAfterProperty); CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -544,6 +544,8 @@ IO.mapOptional("JavaImportGroups", Style.JavaImportGroups); IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes); IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports); +IO.mapOptional("JavaStaticImportAfterImport", + Style.JavaStaticImportAfterImport); IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks", Style.KeepEmptyLinesAtTheStartOfBlocks); IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin); @@ -901,6 +903,7 @@ LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None; LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave; LLVMStyle.JavaScriptWrapImports = true; + LLVMStyle.JavaStaticImportAfterImport = false; LLVMStyle.TabWidth = 8; LLVMStyle.MaxEmptyLinesToKeep = 1; LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true; @@ -2312,12 +2315,15 @@ JavaImportGroups.push_back( findJavaImportGroup(Style, Imports[i].Identifier)); } + bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport; llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) { // Negating IsStatic to push static imports above non-static imports. -return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI], - Imports[LHSI].Identifier) < - std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI], - Imports[RHSI].Identifier); +return std::make_tuple(!Imports[LHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[LHSI], Imports[LHSI].Identifier) < + std::make_tuple(!Imports[RHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[RHSI], Imports[RHSI].Identifier); }); // Deduplicate imports. Index: clang/include/clang/Format/Format.h === --- clang/include/clang/Format/Format.h +++ clang/include/clang/Format/Format.h @@ -1618,10 +1618,12 @@ /// A vector of prefixes ordered by the desired groups for Java imports. /// - /// Each group is separated by a newline. Static imports will also follow the - /// same grouping convention above all non-static imports. One group's prefix - /// can be a subset of another - the longest prefix is always matched. Within - /// a group, the imports are ordered lexicographically. + /// One group's prefix can be a subset of another - the longest prefix is + /// always matched. Within a group, the imports
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
JakeMerdichAMD added inline comments. Comment at: clang/include/clang/Format/Format.h:1705 + /// \endcode + bool JavaStaticImportAfterImport; + bc-lee wrote: > JakeMerdichAMD wrote: > > 3 things here: > > > > 1. Did you mix up the true and false cases? > > 2. (nit) You should also note that if this option is false, all static > > imports are before all non-static imports (as opposed to mixed together > > alphabetically). I was confused on first glance. > > 3. Please add this config option to > > FormatTests.cpp:ParsesConfigurationBools. > I understand. The description is somewhat misleading. New phrasing makes it clear, but true and false are still inverted. 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee updated this revision to Diff 290303. bc-lee added a comment. Some comments have been corrected and a unittest has been added in FormatTest.ParsesConfigurationBools Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87201/new/ https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/FormatTest.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,30 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) { + FmtStyle.JavaStaticImportAfterImport = true; + + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/unittests/Format/FormatTest.cpp === --- clang/unittests/Format/FormatTest.cpp +++ clang/unittests/Format/FormatTest.cpp @@ -13929,6 +13929,7 @@ CHECK_PARSE_BOOL(IndentCaseBlocks); CHECK_PARSE_BOOL(IndentGotoLabels); CHECK_PARSE_BOOL(IndentWrappedFunctionNames); + CHECK_PARSE_BOOL(JavaStaticImportAfterImport); CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks); CHECK_PARSE_BOOL(ObjCSpaceAfterProperty); CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -544,6 +544,8 @@ IO.mapOptional("JavaImportGroups", Style.JavaImportGroups); IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes); IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports); +IO.mapOptional("JavaStaticImportAfterImport", + Style.JavaStaticImportAfterImport); IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks", Style.KeepEmptyLinesAtTheStartOfBlocks); IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin); @@ -901,6 +903,7 @@ LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None; LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave; LLVMStyle.JavaScriptWrapImports = true; + LLVMStyle.JavaStaticImportAfterImport = false; LLVMStyle.TabWidth = 8; LLVMStyle.MaxEmptyLinesToKeep = 1; LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true; @@ -2312,12 +2315,15 @@ JavaImportGroups.push_back( findJavaImportGroup(Style, Imports[i].Identifier)); } + bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport; llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) { // Negating IsStatic to push static imports above non-static imports. -return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI], - Imports[LHSI].Identifier) < - std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI], - Imports[RHSI].Identifier); +return std::make_tuple(!Imports[LHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[LHSI], Imports[LHSI].Identifier) < + std::make_tuple(!Imports[RHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[RHSI], Imports[RHSI].Identifier); }); // Deduplicate imports. Index: clang/include/clang/Format/Format.h === --- clang/include/clang/Format/Format.h +++ clang/include/clang/Format/Format.h @@ -1618,10 +1618,12 @@ /// A vector of prefixes ordered by the desired groups for Java imports. /// - /// Each group is separated by a newline. Static imports will also follow the - /// same grouping convention above all non-static imports. One group's prefix - /// can be a subset of another - the longest prefix is always matched. Within - /// a group, the imports are ordered lexicographically. + /// One group's prefix can be a subset of another - the longest
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:2027 + + .. code-block:: java + true: MyDeveloperDay wrote: > The ClangFormatStyleOptions.rst is generated using > doc/tools/dump_format_style.py which reads Format.h and generates this, > > If this code block in not in the Format.h it will get removed the next time > the script is run, please don't change ClangFormatStyleOption.rst by hand use > the script, so add the code block to the Format.h file (see others options > for now to do this) Done. Comment at: clang/include/clang/Format/Format.h:1705 + /// \endcode + bool JavaStaticImportAfterImport; + JakeMerdichAMD wrote: > 3 things here: > > 1. Did you mix up the true and false cases? > 2. (nit) You should also note that if this option is false, all static > imports are before all non-static imports (as opposed to mixed together > alphabetically). I was confused on first glance. > 3. Please add this config option to FormatTests.cpp:ParsesConfigurationBools. I understand. The description is somewhat misleading. 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
JakeMerdichAMD requested changes to this revision. JakeMerdichAMD added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Format/Format.h:1705 + /// \endcode + bool JavaStaticImportAfterImport; + 3 things here: 1. Did you mix up the true and false cases? 2. (nit) You should also note that if this option is false, all static imports are before all non-static imports (as opposed to mixed together alphabetically). I was confused on first glance. 3. Please add this config option to FormatTests.cpp:ParsesConfigurationBools. 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee updated this revision to Diff 290246. bc-lee added a comment. Modify the comment of Format.h to sync ClangFormatStyleOptions.rst Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87201/new/ https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,30 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) { + FmtStyle.JavaStaticImportAfterImport = true; + + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -544,6 +544,8 @@ IO.mapOptional("JavaImportGroups", Style.JavaImportGroups); IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes); IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports); +IO.mapOptional("JavaStaticImportAfterImport", + Style.JavaStaticImportAfterImport); IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks", Style.KeepEmptyLinesAtTheStartOfBlocks); IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin); @@ -901,6 +903,7 @@ LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None; LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave; LLVMStyle.JavaScriptWrapImports = true; + LLVMStyle.JavaStaticImportAfterImport = false; LLVMStyle.TabWidth = 8; LLVMStyle.MaxEmptyLinesToKeep = 1; LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true; @@ -2312,12 +2315,15 @@ JavaImportGroups.push_back( findJavaImportGroup(Style, Imports[i].Identifier)); } + bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport; llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) { // Negating IsStatic to push static imports above non-static imports. -return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI], - Imports[LHSI].Identifier) < - std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI], - Imports[RHSI].Identifier); +return std::make_tuple(!Imports[LHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[LHSI], Imports[LHSI].Identifier) < + std::make_tuple(!Imports[RHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[RHSI], Imports[RHSI].Identifier); }); // Deduplicate imports. Index: clang/include/clang/Format/Format.h === --- clang/include/clang/Format/Format.h +++ clang/include/clang/Format/Format.h @@ -1689,6 +1689,21 @@ bool JavaScriptWrapImports; // clang-format on + /// If true, clang-format will put Java static imports after all non-static + /// imports. + /// \code{.java} + /// true: + /// import static org.example.function1; + /// + /// import org.example.ClassA; + /// + /// false: + /// import org.example.ClassA; + /// + /// import static org.example.function1; + /// \endcode + bool JavaStaticImportAfterImport; + /// If true, the empty line at the start of blocks is kept. /// \code ///true: false: @@ -2410,6 +2425,7 @@ JavaImportGroups == R.JavaImportGroups && JavaScriptQuotes == R.JavaScriptQuotes && JavaScriptWrapImports == R.JavaScriptWrapImports && + JavaStaticImportAfterImport == R.JavaStaticImportAfterImport && KeepEmptyLinesAtTheStartOfBlocks == R.KeepEmptyLinesAtTheStartOfBlocks && MacroBlockBegin == R.MacroBlockBegin && Index: clang/docs/ClangFormatStyleOptions.rst
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. Thanks for the patch, You need to generate a fill context diff (see Contributing to LLVM) ensure the diff is clang-formatted itself (can't quite tell if it is or not) Comment at: clang/docs/ClangFormatStyleOptions.rst:2027 + + .. code-block:: java + true: The ClangFormatStyleOptions.rst is generated using doc/tools/dump_format_style.py which reads Format.h and generates this, If this code block in not in the Format.h it will get removed the next time the script is run, please don't change ClangFormatStyleOption.rst by hand use the script, so add the code block to the Format.h file (see others options for now to do this) 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
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee updated this revision to Diff 290137. bc-lee added a comment. Add missing initializer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87201/new/ https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,30 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) { + FmtStyle.JavaStaticImportAfterImport = true; + + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -543,6 +543,8 @@ IO.mapOptional("JavaImportGroups", Style.JavaImportGroups); IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes); IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports); +IO.mapOptional("JavaStaticImportAfterImport", + Style.JavaStaticImportAfterImport); IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks", Style.KeepEmptyLinesAtTheStartOfBlocks); IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin); @@ -899,6 +901,7 @@ LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None; LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave; LLVMStyle.JavaScriptWrapImports = true; + LLVMStyle.JavaStaticImportAfterImport = false; LLVMStyle.TabWidth = 8; LLVMStyle.MaxEmptyLinesToKeep = 1; LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true; @@ -2310,12 +2313,15 @@ JavaImportGroups.push_back( findJavaImportGroup(Style, Imports[i].Identifier)); } + bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport; llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) { // Negating IsStatic to push static imports above non-static imports. -return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI], - Imports[LHSI].Identifier) < - std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI], - Imports[RHSI].Identifier); +return std::make_tuple(!Imports[LHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[LHSI], Imports[LHSI].Identifier) < + std::make_tuple(!Imports[RHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[RHSI], Imports[RHSI].Identifier); }); // Deduplicate imports. Index: clang/include/clang/Format/Format.h === --- clang/include/clang/Format/Format.h +++ clang/include/clang/Format/Format.h @@ -1671,6 +1671,10 @@ bool JavaScriptWrapImports; // clang-format on + /// If true, clang-format will put Java Static imports after all non-static + /// imports. + bool JavaStaticImportAfterImport; + /// If true, the empty line at the start of blocks is kept. /// \code ///true: false: @@ -2391,6 +2395,7 @@ JavaImportGroups == R.JavaImportGroups && JavaScriptQuotes == R.JavaScriptQuotes && JavaScriptWrapImports == R.JavaScriptWrapImports && + JavaStaticImportAfterImport == R.JavaStaticImportAfterImport && KeepEmptyLinesAtTheStartOfBlocks == R.KeepEmptyLinesAtTheStartOfBlocks && MacroBlockBegin == R.MacroBlockBegin && Index: clang/docs/ClangFormatStyleOptions.rst === --- clang/docs/ClangFormatStyleOptions.rst +++ clang/docs/ClangFormatStyleOptions.rst @@ -2021,6 +2021,20 @@ false: import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
[PATCH] D87201: [clang-format] Add a option for the position of Java static import
bc-lee created this revision. bc-lee added a reviewer: MyDeveloperDay. bc-lee added projects: clang, clang-format. Herald added a subscriber: cfe-commits. bc-lee requested review of this revision. Some Java style guides and IDEs group Java static imports after non-static imports. This patch allows clang-format to control the location of static imports. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D87201 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/unittests/Format/SortImportsTestJava.cpp Index: clang/unittests/Format/SortImportsTestJava.cpp === --- clang/unittests/Format/SortImportsTestJava.cpp +++ clang/unittests/Format/SortImportsTestJava.cpp @@ -250,6 +250,30 @@ "import org.c;\n")); } +TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) { + FmtStyle.JavaStaticImportAfterImport = true; + + EXPECT_EQ("import com.test.b;\n" +"import com.test.c;\n" +"\n" +"import org.b;\n" +"\n" +"import com.b;\n" +"\n" +"import static com.test.a;\n" +"\n" +"import static org.a;\n" +"\n" +"import static com.a;\n", +sort("import static com.test.a;\n" + "import static org.a;\n" + "import static com.a;\n" + "import com.test.b;\n" + "import org.b;\n" + "import com.b;\n" + "import com.test.c;\n")); +} + TEST_F(SortImportsTestJava, DeduplicateImports) { EXPECT_EQ("import org.a;\n", sort("import org.a;\n" "import org.a;\n")); Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -541,6 +541,8 @@ Style.IndentWrappedFunctionNames); IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas); IO.mapOptional("JavaImportGroups", Style.JavaImportGroups); +IO.mapOptional("JavaStaticImportAfterImport", + Style.JavaStaticImportAfterImport); IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes); IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports); IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks", @@ -2310,12 +2312,15 @@ JavaImportGroups.push_back( findJavaImportGroup(Style, Imports[i].Identifier)); } + bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport; llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) { // Negating IsStatic to push static imports above non-static imports. -return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI], - Imports[LHSI].Identifier) < - std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI], - Imports[RHSI].Identifier); +return std::make_tuple(!Imports[LHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[LHSI], Imports[LHSI].Identifier) < + std::make_tuple(!Imports[RHSI].IsStatic ^ + StaticImportAfterNormalImport, + JavaImportGroups[RHSI], Imports[RHSI].Identifier); }); // Deduplicate imports. Index: clang/include/clang/Format/Format.h === --- clang/include/clang/Format/Format.h +++ clang/include/clang/Format/Format.h @@ -1671,6 +1671,10 @@ bool JavaScriptWrapImports; // clang-format on + /// If true, clang-format will put Java Static imports after all non-static + /// imports. + bool JavaStaticImportAfterImport; + /// If true, the empty line at the start of blocks is kept. /// \code ///true: false: @@ -2389,6 +2393,7 @@ IndentWidth == R.IndentWidth && Language == R.Language && IndentWrappedFunctionNames == R.IndentWrappedFunctionNames && JavaImportGroups == R.JavaImportGroups && + JavaStaticImportAfterImport == R.JavaStaticImportAfterImport && JavaScriptQuotes == R.JavaScriptQuotes && JavaScriptWrapImports == R.JavaScriptWrapImports && KeepEmptyLinesAtTheStartOfBlocks == Index: clang/docs/ClangFormatStyleOptions.rst === --- clang/docs/ClangFormatStyleOptions.rst +++ clang/docs/ClangFormatStyleOptions.rst @@ -2021,6 +2021,20 @@ false: import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js" +**JavaStaticImportAfterImport** (``bool``) + If true, clang-format will put Java static imports after all non-static imports.