This is an archive of the discontinued LLVM Phabricator instance.

Java import sorting in clang-format
ClosedPublic

Authored by SamMaier on Oct 2 2018, 1:49 PM.

Details

Summary

This is for https://bugs.chromium.org/p/chromium/issues/detail?id=768983 - however it will be useful for anyone using clang-format for Java, not just Chromium.

Diff Detail

Event Timeline

SamMaier created this revision.Oct 2 2018, 1:49 PM
SamMaier removed rC Clang as the repository for this revision.Oct 2 2018, 1:50 PM
mgrang added inline comments.Oct 2 2018, 3:42 PM
lib/Format/Format.cpp
1856
SamMaier updated this revision to Diff 168111.Oct 3 2018, 7:21 AM
SamMaier set the repository for this revision to rC Clang.

Changed std::sort to llvm::sort

SamMaier marked an inline comment as done.Oct 3 2018, 7:22 AM
krasimir added inline comments.Oct 3 2018, 9:17 AM
lib/Format/Format.cpp
848

It would be helpful to link to a style guide reference for this.

1837–1838

It would be helpful to spell out the sort order and add tests with respect to . and *. ASCII ordering has * < . < A < a which seems reasonable.

1922

Please add a test for this.

1932

Hm, this would also pick up the static in import a.*; // static, right? IMO not a big problem, just add a note about it.

unittests/Format/SortImportsTestJava.cpp
130

I wonder what happens if there are comments between import statements and comment lines after import statements. Consider adding some tests for that.

Another example: we have to be careful with something like this as we don't want to break correct code:

import x;
import a.loooooong.
  c;
import y;
unittests/Format/SortImportsTestJava.cpp
130

Specifically, as discussed, we wanna keep comments about imports together with them when we reorder stuff around.

krasimir added inline comments.Oct 3 2018, 5:09 PM
lib/Format/Format.cpp
1823

nit: LLVM Style uses CamelCase for variables, please use I.

1834

nit: Sorts and deduplicates

1836

nit: Import declarations...

unittests/Format/SortImportsTestJava.cpp
99

Add a test where there are existing blocks of import statements separated by empty lines. I'd expect that a newly added import gets added to the right already existing block, right?

136

interested does the newline get taken into account? e.g. what happens with

sort("import org.a;\n"
     "import org.b;");
SamMaier updated this revision to Diff 168304.Oct 4 2018, 8:55 AM
SamMaier marked 11 inline comments as done.

Addressing comments

lib/Format/Format.cpp
1932

It does not, since Static is the first group of the regex. Test has been added for this

unittests/Format/SortImportsTestJava.cpp
136

Added test, it works.

krasimir accepted this revision.Oct 5 2018, 9:05 AM

This gooks great! Thanks for the contribution!
If you don't have commit access to Clang I can commit this for you.

This revision is now accepted and ready to land.Oct 5 2018, 9:05 AM