New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 768983 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocked on:
issue 768586

Blocking:
issue 768576



Sign in to add a comment

clang-format should sort java imports

Project Member Reported by agrieve@chromium.org, Sep 26 2017

Issue description

Although clang-format can sort #includes, and javascript imports, it doesn't yet have support for java imports.

Java import order is documented in the style guide here:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#import-order
 
Cc: nyquist@chromium.org

Comment 2 by thakis@chromium.org, Sep 26 2017

Labels: clang-format
https://reviews.llvm.org/D38294 is a hack that makes clang-format's #include sorting work for import lines too. It behaves like the #include sorting, which is fairly different from https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#import-order :

* clang-format only reorders lines within an import block (which is a newline-separated list of imports)
* within an import block, it puts `import static` lines at the top
* it then sorts alphabetically
* users can opt to use the IncludeCategories style option to order entries within a block by some system (e.g. the "android, then com, then dalvik, then junit, then org, then com.google, then org.chromium, then java, then javax" -- but this is almost but not quite alphabetical -- why don't we just sort alphabetically?)


While consistency with the c++ behavior is kind of nice, I'm assuming that this behavior is not what we want, and I should implement dedicated java import sorting that sorts across import blocks, inserts newlines as desired, etc. Is that correct?
Cc: agrieve@chromium.org
Sorry about the weird sorting. It's mostly based off of https://source.android.com/source/code-style#order-import-statements which is our "parent" style guide for Java.

I don't think we want to keep consistency with C++ here. Sorry about that.

So yeah, it would be greatly appreciated if you added dedicated Java import sorting like you specified.

If you for some reason want to try and see whether clang-format creates the same format as what the IDE of the people writing Java produces, you can follow the guidelines here to get the import scheme from https://chromium.googlesource.com/chromium/src/+/7ea184854a516e5352ef11cd482eec783b0aaa52/tools/android/android_studio/ChromiumStyle.xml#18 :
https://chromium.googlesource.com/chromium/src/+/master/docs/android_studio.md#android-studio-tips
Components: Tools
Status: Assigned (was: Available)
Owner: smaier@chromium.org
Status: Started (was: Assigned)
I've uploaded a patch here: https://reviews.llvm.org/D52800

I'm seeing "context not available" for the files - not sure what to do there. I believe this will meet all our requirements for Java import formatting. Hopefully someone with clang knowledge (thakis@?) can help me with getting it reviewed/submitted.
Blockedon: 768586
Looks like that landed upstream in https://reviews.llvm.org/rC343862 , so it's "only" blocked on a clang-format roll.
Yep, I've got the clang-format roll on the way. All 3 binaries are built, review should be up today.
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d049675203babf675a40e2cb583ae61211057e68

commit d049675203babf675a40e2cb583ae61211057e68
Author: Sam Maier <smaier@chromium.org>
Date: Wed Oct 10 17:35:35 2018

Java: Fixing all imports with clang-format change

This is a test run with the clang-format Java import reorderer.

TBR=mkearney@chromium.org let me know if the docs ordering is intentional

No-Presubmit: true
Bug:  768983 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester
Change-Id: Ic2bce530e5e806ca2de6c0ad266f2e5ae38dba82
Reviewed-on: https://chromium-review.googlesource.com/c/1262969
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598388}
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/java/src/org/chromium/chrome/browser/engagement/SiteEngagementService.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchBackgroundTaskTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/junit/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandlerTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/junit/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionControllerTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationTestTabHolder.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/junit/src/org/chromium/chrome/browser/omaha/VersionNumberTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/common/extensions/docs/examples/apps/hello-java/HelloLicenseServlet.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/chrome/common/extensions/docs/examples/extensions/irc/servlet/src/org/chromium/IRCProxyWebSocket.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBase.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/content/public/android/junit/src/org/chromium/content/browser/SpareChildConnectionTest.java
[modify] https://crrev.com/d049675203babf675a40e2cb583ae61211057e68/ui/android/junit/src/org/chromium/ui/base/ClipboardTest.java

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/buildtools/+/24ebce4578745db15274e180da1938ebc1358243

commit 24ebce4578745db15274e180da1938ebc1358243
Author: Sam Maier <smaier@chromium.org>
Date: Fri Nov 16 15:36:49 2018

Update clang-format binaries and scripts for all platforms.

I followed these instructions:
https://chromium.googlesource.com/chromium/src/+/master/docs/updating_clang_format_binaries.md

The binaries were built at clang revision 346566 on November 15, 2018.

Rolling because 344070 introduces Java import reordering in
clang-format, and 346566 fixes a critical bug with Objective C.

OLD: https://chromium-review.googlesource.com/c/chromium/src/+/1296816
NEW: https://chromium-review.googlesource.com/c/chromium/src/+/1331609

ls -l output
-rwxr-xr-x 1 smaier primarygroup 1967524 Nov 15 16:47 buildtools/mac/clang-format
-rwxr-xr-x 1 smaier primarygroup 1862936 Nov 15 14:06 buildtools/linux64/clang-format
-rw-r--r-- 1 smaier primarygroup 2637824 Nov 15 14:04 buildtools/win/clang-format.exe


Bug:  768586 , 768983 
Change-Id: I2761fc91201d2ffa0d999e637f8f8ba53bcccb5e

[modify] https://crrev.com/24ebce4578745db15274e180da1938ebc1358243/linux64/clang-format.sha1
[modify] https://crrev.com/24ebce4578745db15274e180da1938ebc1358243/deps_revisions.gni
[modify] https://crrev.com/24ebce4578745db15274e180da1938ebc1358243/win/clang-format.exe.sha1
[modify] https://crrev.com/24ebce4578745db15274e180da1938ebc1358243/clang_format/README.chromium
[modify] https://crrev.com/24ebce4578745db15274e180da1938ebc1358243/DEPS
[modify] https://crrev.com/24ebce4578745db15274e180da1938ebc1358243/mac/clang-format.sha1

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf89178f52ee6fe536140f83338af3494e8355cc

commit bf89178f52ee6fe536140f83338af3494e8355cc
Author: Nico Weber <thakis@chromium.org>
Date: Wed Nov 21 02:07:20 2018

Roll src/buildtools/ 13a00f110..da9b2941c (3 commits)

https://chromium.googlesource.com/chromium/buildtools.git/+log/13a00f110ef9..da9b2941cbf6

$ git log 13a00f110..da9b2941c --date=short --no-merges --format='%ad %ae %s'
2018-11-20 thakis Revert "Roll libcxx{abi}
2018-11-16 smaier Update clang-format binaries and scripts for all platforms.
2018-10-29 thomasanderson Roll libcxx{abi}

Created with:
  roll-dep src/buildtools

Bug:  768586 , 768983 
Change-Id: Ib72e2e4f38b9ed662652a440925ff9260f97abcc
Reviewed-on: https://chromium-review.googlesource.com/c/1344293
Reviewed-by: Sam Maier <smaier@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609898}
[modify] https://crrev.com/bf89178f52ee6fe536140f83338af3494e8355cc/DEPS

Status: Fixed (was: Started)
A clang-format with the fix got deployed on Nov 21 2018.

Sign in to add a comment