Page MenuHomePhabricator

[clang-format] Add a option for the position of Java static import
ClosedPublic

Authored by bc-lee on Sep 5 2020, 9:51 PM.

Details

Summary

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.

Diff Detail

Event Timeline

bc-lee created this revision.Sep 5 2020, 9:51 PM
bc-lee requested review of this revision.Sep 5 2020, 9:51 PM
bc-lee updated this revision to Diff 290137.Sep 6 2020, 5:47 AM

Add missing initializer.

MyDeveloperDay requested changes to this revision.Sep 7 2020, 1:30 AM

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)

clang/docs/ClangFormatStyleOptions.rst
2027

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)

This revision now requires changes to proceed.Sep 7 2020, 1:30 AM
bc-lee updated this revision to Diff 290246.Sep 7 2020, 4:47 AM

Modify the comment of Format.h to sync ClangFormatStyleOptions.rst

JakeMerdichAMD requested changes to this revision.Sep 7 2020, 8:21 AM
JakeMerdichAMD added inline comments.
clang/include/clang/Format/Format.h
1707

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.
This revision now requires changes to proceed.Sep 7 2020, 8:21 AM
bc-lee added inline comments.Sep 7 2020, 8:52 AM
clang/docs/ClangFormatStyleOptions.rst
2027

Done.

clang/include/clang/Format/Format.h
1707

I understand. The description is somewhat misleading.

bc-lee updated this revision to Diff 290303.Sep 7 2020, 9:18 AM

Some comments have been corrected and a unittest has been added in FormatTest.ParsesConfigurationBools

JakeMerdichAMD added inline comments.Sep 7 2020, 1:11 PM
clang/include/clang/Format/Format.h
1707

New phrasing makes it clear, but true and false are still inverted.

bc-lee updated this revision to Diff 290381.Sep 7 2020, 5:36 PM

Fix the example to match the description.

MyDeveloperDay added inline comments.Sep 8 2020, 7:50 AM
clang/docs/ClangFormatStyleOptions.rst
1972

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;
clang/include/clang/Format/Format.h
1708

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+

clang/lib/Format/Format.cpp
914

We can't have a default that does will cause a change

clang/unittests/Format/SortImportsTestJava.cpp
253

please test all options

You are also missing a test for checking the PARSING of the options

MyDeveloperDay requested changes to this revision.Sep 8 2020, 7:50 AM
This revision now requires changes to proceed.Sep 8 2020, 7:50 AM
JakeMerdichAMD added inline comments.Sep 8 2020, 8:06 AM
clang/include/clang/Format/Format.h
1708

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.

clang/lib/Format/Format.cpp
914

Not a default change, see previous comment for discussion.

clang/unittests/Format/SortImportsTestJava.cpp
253

Parsing test was already noted and done (unless this changes to an enum of course). But testing the 'false' case here makes sense.

bc-lee updated this revision to Diff 290619.Sep 8 2020, 5:26 PM

Add more tests and rename options.

bc-lee added inline comments.Sep 8 2020, 5:28 PM
clang/docs/ClangFormatStyleOptions.rst
1972

Done.

clang/include/clang/Format/Format.h
1708

If SortIncludes is true, it doesn't make sense to not touch Java static include.
Making it an enum is also good for me.

clang/unittests/Format/SortImportsTestJava.cpp
253

Done

MyDeveloperDay accepted this revision.Sep 9 2020, 6:59 AM

LGTM, I'm happy if @JakeMerdichAMD is

clang/include/clang/Format/Format.h
1708

true.. it will sort it I assume one way or the other

JakeMerdichAMD accepted this revision.Sep 12 2020, 1:25 PM

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).

This revision is now accepted and ready to land.Sep 12 2020, 1:25 PM
bc-lee marked an inline comment as done.Sep 12 2020, 4:54 PM

It's okay for some reviewers to make this change on my behalf. Thanks for reviewing this!

Can someone commit my changes on behalf of it?