[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Jake Merdich via Phabricator via cfe-commits]

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