[PATCH] D53434: Java annotation declaration being handled correctly

[Sam Maier via Phabricator via cfe-commits]

SamMaier created this revision.
Herald added a subscriber: cfe-commits.

Previously, Java annotation declarations (`@interface AnnotationName`) were 
being handled as ObjC interfaces. This caused the brace formatting to mess up, 
so that when you had a class with an interface defined in it, it would indent 
the final brace of the class.

It used to format this class like so:

  class A {
@interface B {}
}

But will now just skip the @interface and format it like so:

  class A {
@interface B {}
  }


Repository:
  rC Clang

https://reviews.llvm.org/D53434

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestJava.cpp


Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -155,6 +155,15 @@
"  void doStuff(int theStuff);\n"
"  void doMoreStuff(int moreStuff);\n"
"}");
+  verifyFormat("class A {\n"
+   "  public @interface SomeInterface {\n"
+   "int stuff;\n"
+   "void doMoreStuff(int moreStuff);\n"
+   "  }\n"
+   "}");
+  verifyFormat("class A {\n"
+   "  public @interface SomeInterface {}\n"
+   "}");
 }
 
 TEST_F(FormatTestJava, AnonymousClasses) {
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1130,6 +1130,10 @@
 nextToken();
 parseBracedList();
 break;
+  } else if (Style.Language == FormatStyle::LK_Java &&
+ FormatTok->is(Keywords.kw_interface)) {
+nextToken();
+break;
   }
   switch (FormatTok->Tok.getObjCKeywordID()) {
   case tok::objc_public:


Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -155,6 +155,15 @@
"  void doStuff(int theStuff);\n"
"  void doMoreStuff(int moreStuff);\n"
"}");
+  verifyFormat("class A {\n"
+   "  public @interface SomeInterface {\n"
+   "int stuff;\n"
+   "void doMoreStuff(int moreStuff);\n"
+   "  }\n"
+   "}");
+  verifyFormat("class A {\n"
+   "  public @interface SomeInterface {}\n"
+   "}");
 }
 
 TEST_F(FormatTestJava, AnonymousClasses) {
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1130,6 +1130,10 @@
 nextToken();
 parseBracedList();
 break;
+  } else if (Style.Language == FormatStyle::LK_Java &&
+ FormatTok->is(Keywords.kw_interface)) {
+nextToken();
+break;
   }
   switch (FormatTok->Tok.getObjCKeywordID()) {
   case tok::objc_public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60203: Updating Chromium's Java import order

[Sam Maier via Phabricator via cfe-commits]

SamMaier created this revision.
SamMaier added a reviewer: thakis.
Herald added subscribers: cfe-commits, srhines.
Herald added a project: clang.

Adding in androidx as another import group.


Repository:
  rC Clang

https://reviews.llvm.org/D60203

Files:
  lib/Format/Format.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -893,9 +893,16 @@
 // See styleguide for import groups:
 // 
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Import-Order
 ChromiumStyle.JavaImportGroups = {
-"android",  "com",  "dalvik",
-"junit","org",  "com.google.android.apps.chrome",
-"org.chromium", "java", "javax",
+"android",
+"androidx",
+"com",
+"dalvik",
+"junit",
+"org",
+"com.google.android.apps.chrome",
+"org.chromium",
+"java",
+"javax",
 };
 ChromiumStyle.SortIncludes = true;
   } else if (Language == FormatStyle::LK_JavaScript) {


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -893,9 +893,16 @@
 // See styleguide for import groups:
 // https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Import-Order
 ChromiumStyle.JavaImportGroups = {
-"android",  "com",  "dalvik",
-"junit","org",  "com.google.android.apps.chrome",
-"org.chromium", "java", "javax",
+"android",
+"androidx",
+"com",
+"dalvik",
+"junit",
+"org",
+"com.google.android.apps.chrome",
+"org.chromium",
+"java",
+"javax",
 };
 ChromiumStyle.SortIncludes = true;
   } else if (Language == FormatStyle::LK_JavaScript) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60203: Updating Chromium's Java import order

[Sam Maier via Phabricator via cfe-commits]

SamMaier added a comment.

In D60203#1453313 , @thakis wrote:

> Thanks!
>
> (Test?)


We currently don't have tests for Chromium's specific Java import order. The 
tests for Java import order use their own order. Should this change?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60203/new/

https://reviews.llvm.org/D60203



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52800: Java import sorting in clang-format

[Sam Maier via Phabricator via cfe-commits]

SamMaier created this revision.
Herald added subscribers: cfe-commits, mgrang, mgorny, srhines.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D52800

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJava.cpp

Index: unittests/Format/SortImportsTestJava.cpp
===
--- unittests/Format/SortImportsTestJava.cpp
+++ unittests/Format/SortImportsTestJava.cpp
@@ -0,0 +1,141 @@
+//===- unittest/Format/SortImportsTestJava.cpp - Import sort unit tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJava : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, std::vector Ranges) {
+auto Replaces = sortIncludes(FmtStyle, Code, Ranges, "input.java");
+Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
+auto Sorted = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Sorted));
+auto Result = applyAllReplacements(
+*Sorted, reformat(FmtStyle, *Sorted, Ranges, "input.java"));
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  std::string sort(StringRef Code) { return sort(Code, GetCodeRange(Code)); }
+
+  FormatStyle FmtStyle;
+
+public:
+  SortImportsTestJava() {
+FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
+FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
+FmtStyle.SortIncludes = true;
+  }
+};
+
+TEST_F(SortImportsTestJava, StaticSplitFromNormal) {
+  EXPECT_EQ("import static org.b;\n"
+"\n"
+"import org.a;\n",
+sort("import org.a;\n"
+ "import static org.b;\n"));
+}
+
+TEST_F(SortImportsTestJava, CapitalBeforeLowercase) {
+  EXPECT_EQ("import org.Test;\n"
+"import org.a.Test;\n"
+"import org.b;\n",
+sort("import org.a.Test;\n"
+ "import org.Test;\n"
+ "import org.b;\n"));
+}
+
+TEST_F(SortImportsTestJava, SplitGroupsWithNewline) {
+  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"));
+}
+
+TEST_F(SortImportsTestJava, UnspecifiedGroupAfterAllGroups) {
+  EXPECT_EQ("import com.test.a;\n"
+"\n"
+"import org.a;\n"
+"\n"
+"import com.a;\n"
+"\n"
+"import abc.a;\n"
+"import xyz.b;\n",
+sort("import com.test.a;\n"
+ "import com.a;\n"
+ "import xyz.b;\n"
+ "import abc.a;\n"
+ "import org.a;\n"));
+}
+
+TEST_F(SortImportsTestJava, NoSortOutsideRange) {
+  std::vector Ranges = {tooling::Range(27, 15)};
+  EXPECT_EQ("import org.b;\n"
+"import org.a;\n"
+"// comments\n"
+"// that do\n"
+"// nothing\n",
+sort("import org.b;\n"
+ "import org.a;\n"
+ "// comments\n"
+ "// that do\n"
+ "// nothing\n",
+ Ranges));
+}
+
+TEST_F(SortImportsTestJava, SortWhenRangeContainsOneLine) {
+  std::vector Ranges = {tooling::Range(27, 20)};
+  EXPECT_EQ("import org.a;\n"
+"import org.b;\n"
+"\n"
+"import com.a;\n"
+"// comments\n"
+"// that do\n"
+"// nothing\n",
+sort("import org.b;\n"
+ "import org.a;\n"
+ "import com.a;\n"
+ "// comments\n"
+ "// that do\n"
+ "// nothing\n",
+ Ranges));
+}
+
+TEST_F(SortImportsTestJava, DeduplicateImports) {
+  EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
+"import org.a;\n"));
+}
+
+} // end namespace
+} // end namespace format
+} // end namespace clang
Ind

[PATCH] D52800: Java import sorting in clang-format

[Sam Maier via Phabricator via cfe-commits]

SamMaier updated this revision to Diff 168111.
SamMaier set the repository for this revision to rC Clang.
SamMaier added a comment.

Changed std::sort to llvm::sort


Repository:
  rC Clang

https://reviews.llvm.org/D52800

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJava.cpp

Index: unittests/Format/SortImportsTestJava.cpp
===
--- unittests/Format/SortImportsTestJava.cpp
+++ unittests/Format/SortImportsTestJava.cpp
@@ -0,0 +1,141 @@
+//===- unittest/Format/SortImportsTestJava.cpp - Import sort unit tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJava : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, std::vector Ranges) {
+auto Replaces = sortIncludes(FmtStyle, Code, Ranges, "input.java");
+Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
+auto Sorted = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Sorted));
+auto Result = applyAllReplacements(
+*Sorted, reformat(FmtStyle, *Sorted, Ranges, "input.java"));
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  std::string sort(StringRef Code) { return sort(Code, GetCodeRange(Code)); }
+
+  FormatStyle FmtStyle;
+
+public:
+  SortImportsTestJava() {
+FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
+FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
+FmtStyle.SortIncludes = true;
+  }
+};
+
+TEST_F(SortImportsTestJava, StaticSplitFromNormal) {
+  EXPECT_EQ("import static org.b;\n"
+"\n"
+"import org.a;\n",
+sort("import org.a;\n"
+ "import static org.b;\n"));
+}
+
+TEST_F(SortImportsTestJava, CapitalBeforeLowercase) {
+  EXPECT_EQ("import org.Test;\n"
+"import org.a.Test;\n"
+"import org.b;\n",
+sort("import org.a.Test;\n"
+ "import org.Test;\n"
+ "import org.b;\n"));
+}
+
+TEST_F(SortImportsTestJava, SplitGroupsWithNewline) {
+  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"));
+}
+
+TEST_F(SortImportsTestJava, UnspecifiedGroupAfterAllGroups) {
+  EXPECT_EQ("import com.test.a;\n"
+"\n"
+"import org.a;\n"
+"\n"
+"import com.a;\n"
+"\n"
+"import abc.a;\n"
+"import xyz.b;\n",
+sort("import com.test.a;\n"
+ "import com.a;\n"
+ "import xyz.b;\n"
+ "import abc.a;\n"
+ "import org.a;\n"));
+}
+
+TEST_F(SortImportsTestJava, NoSortOutsideRange) {
+  std::vector Ranges = {tooling::Range(27, 15)};
+  EXPECT_EQ("import org.b;\n"
+"import org.a;\n"
+"// comments\n"
+"// that do\n"
+"// nothing\n",
+sort("import org.b;\n"
+ "import org.a;\n"
+ "// comments\n"
+ "// that do\n"
+ "// nothing\n",
+ Ranges));
+}
+
+TEST_F(SortImportsTestJava, SortWhenRangeContainsOneLine) {
+  std::vector Ranges = {tooling::Range(27, 20)};
+  EXPECT_EQ("import org.a;\n"
+"import org.b;\n"
+"\n"
+"import com.a;\n"
+"// comments\n"
+"// that do\n"
+"// nothing\n",
+sort("import org.b;\n"
+ "import org.a;\n"
+ "import com.a;\n"
+ "// comments\n"
+ "// that do\n"
+ "// nothing\n",
+ Ranges));
+}
+
+TEST_F(SortImportsTestJava, DeduplicateImports) {
+  EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
+"import org.a;\n"));
+}
+
+} // end namespace
+} // end namespace format
+} // end namespace clang
Index: unittests/Format/CMakeLists.txt
==

[PATCH] D52800: Java import sorting in clang-format

[Sam Maier via Phabricator via cfe-commits]

SamMaier updated this revision to Diff 168304.
SamMaier marked 11 inline comments as done.
SamMaier added a comment.

Addressing comments


Repository:
  rC Clang

https://reviews.llvm.org/D52800

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJava.cpp

Index: unittests/Format/SortImportsTestJava.cpp
===
--- unittests/Format/SortImportsTestJava.cpp
+++ unittests/Format/SortImportsTestJava.cpp
@@ -0,0 +1,276 @@
+//===- unittest/Format/SortImportsTestJava.cpp - Import sort unit tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJava : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, std::vector Ranges) {
+auto Replaces = sortIncludes(FmtStyle, Code, Ranges, "input.java");
+Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
+auto Sorted = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Sorted));
+auto Result = applyAllReplacements(
+*Sorted, reformat(FmtStyle, *Sorted, Ranges, "input.java"));
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  std::string sort(StringRef Code) { return sort(Code, GetCodeRange(Code)); }
+
+  FormatStyle FmtStyle;
+
+public:
+  SortImportsTestJava() {
+FmtStyle = getGoogleStyle(FormatStyle::LK_Java);
+FmtStyle.JavaImportGroups = {"com.test", "org", "com"};
+FmtStyle.SortIncludes = true;
+  }
+};
+
+TEST_F(SortImportsTestJava, StaticSplitFromNormal) {
+  EXPECT_EQ("import static org.b;\n"
+"\n"
+"import org.a;\n",
+sort("import org.a;\n"
+ "import static org.b;\n"));
+}
+
+TEST_F(SortImportsTestJava, CapitalBeforeLowercase) {
+  EXPECT_EQ("import org.Test;\n"
+"import org.a.Test;\n"
+"import org.b;\n",
+sort("import org.a.Test;\n"
+ "import org.Test;\n"
+ "import org.b;\n"));
+}
+
+TEST_F(SortImportsTestJava, KeepSplitGroupsWithOneNewImport) {
+  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"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n",
+sort("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"
+ "import com.test.c;\n"));
+}
+
+TEST_F(SortImportsTestJava, SplitGroupsWithNewline) {
+  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"));
+}
+
+TEST_F(SortImportsTestJava, UnspecifiedGroupAfterAllGroups) {
+  EXPECT_EQ("import com.test.a;\n"
+"\n"
+"import org.a;\n"
+"\n"
+"import com.a;\n"
+"\n"
+"import abc.a;\n"
+"import xyz.b;\n",
+sort("import com.test.a;\n"
+ "import com.a;\n"
+ "import xyz.b;\n"
+ "import abc.a;\n"
+ "import org.a;\n"));
+}
+
+TEST_F(SortImportsTestJava, NoSortOutsideRange) {
+  std::vector Ranges = {tooling::Range(27, 15)};
+  EXPECT_EQ("import org.b;\n"
+"import org.a;\n"
+"// comments\n"
+"// that do\n"
+"// nothing\n",
+sort("import org.b;\n"
+ "import org.a;\n"
+ "// comments\n"
+ "// that do\n"
+ "// nothing\n",
+ Ranges));
+}
+
+TEST_F(SortImportsTestJava, SortWhenRangeContainsOneLine) {
+  std::vector Ranges = {tooling::Ra

[PATCH] D52800: Java import sorting in clang-format

[Sam Maier via Phabricator via cfe-commits]

SamMaier added inline comments.



Comment at: lib/Format/Format.cpp:1932
+bool IsStatic = false;
+if (Static.contains("static")) {
+  IsStatic = true;

krasimir wrote:
> Hm, this would also pick up the `static` in `import a.*; // static`, right? 
> IMO not a big problem, just add a note about it.
It does not, since `Static` is the first group of the regex. Test has been 
added for this



Comment at: unittests/Format/SortImportsTestJava.cpp:136
+  EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
+"import org.a;\n"));
+}

krasimir wrote:
> interested does the newline get taken into account? e.g. what happens with
> ```
> sort("import org.a;\n"
>  "import org.b;");
> ```
Added test, it works.


Repository:
  rC Clang

https://reviews.llvm.org/D52800



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67843: DisableFormat also now disables SortIncludes

[Sam Maier via Phabricator via cfe-commits]

SamMaier created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
SamMaier added a reviewer: thakis.

Previously, in order to have clang-format //not// format something, you had to 
give both:

  SortIncludes: false
  DisableFormat: true

This is confusing to users, who would expect that DisableFormat does what it 
says, and prevents clang-format from editing any files it applies to. See 
https://stackoverflow.com/questions/55833838/clang-format-still-formatting-with-disableformat-true
 for example.


Repository:
  rC Clang

https://reviews.llvm.org/D67843

Files:
  clang/lib/Format/Format.cpp
  clang/test/Format/disable-format.cpp


Index: clang/test/Format/disable-format.cpp
===
--- clang/test/Format/disable-format.cpp
+++ clang/test/Format/disable-format.cpp
@@ -1,5 +1,9 @@
 // RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=none \
 // RUN:   | FileCheck -strict-whitespace %s

-// CHECK: int   i;
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CHECK-NEXT: int   i;
+#include 
+#include 
 int   i;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1062,8 +1062,6 @@
 FormatStyle getNoStyle() {
   FormatStyle NoStyle = getLLVMStyle();
   NoStyle.DisableFormat = true;
-  NoStyle.SortIncludes = false;
-  NoStyle.SortUsingDeclarations = false;
   return NoStyle;
 }

@@ -2130,7 +2128,7 @@
ArrayRef Ranges,
StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
-  if (!Style.SortIncludes)
+  if (!Style.SortIncludes || Style.DisableFormat)
 return Replaces;
   if (isLikelyXml(Code))
 return Replaces;


Index: clang/test/Format/disable-format.cpp
===
--- clang/test/Format/disable-format.cpp
+++ clang/test/Format/disable-format.cpp
@@ -1,5 +1,9 @@
 // RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=none \
 // RUN:   | FileCheck -strict-whitespace %s

-// CHECK: int   i;
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CHECK-NEXT: int   i;
+#include 
+#include 
 int   i;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1062,8 +1062,6 @@
 FormatStyle getNoStyle() {
   FormatStyle NoStyle = getLLVMStyle();
   NoStyle.DisableFormat = true;
-  NoStyle.SortIncludes = false;
-  NoStyle.SortUsingDeclarations = false;
   return NoStyle;
 }

@@ -2130,7 +2128,7 @@
ArrayRef Ranges,
StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
-  if (!Style.SortIncludes)
+  if (!Style.SortIncludes || Style.DisableFormat)
 return Replaces;
   if (isLikelyXml(Code))
 return Replaces;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67843: DisableFormat also now disables SortIncludes

[Sam Maier via Phabricator via cfe-commits]

SamMaier updated this revision to Diff 221065.
SamMaier added a comment.

Diff with more context


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67843/new/

https://reviews.llvm.org/D67843

Files:
  clang/lib/Format/Format.cpp
  clang/test/Format/disable-format.cpp


Index: clang/test/Format/disable-format.cpp
===
--- clang/test/Format/disable-format.cpp
+++ clang/test/Format/disable-format.cpp
@@ -1,5 +1,9 @@
 // RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=none \
 // RUN:   | FileCheck -strict-whitespace %s
 
-// CHECK: int   i;
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CHECK-NEXT: int   i;
+#include 
+#include 
 int   i;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1062,8 +1062,6 @@
 FormatStyle getNoStyle() {
   FormatStyle NoStyle = getLLVMStyle();
   NoStyle.DisableFormat = true;
-  NoStyle.SortIncludes = false;
-  NoStyle.SortUsingDeclarations = false;
   return NoStyle;
 }
 
@@ -2130,7 +2128,7 @@
ArrayRef Ranges,
StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
-  if (!Style.SortIncludes)
+  if (!Style.SortIncludes || Style.DisableFormat)
 return Replaces;
   if (isLikelyXml(Code))
 return Replaces;


Index: clang/test/Format/disable-format.cpp
===
--- clang/test/Format/disable-format.cpp
+++ clang/test/Format/disable-format.cpp
@@ -1,5 +1,9 @@
 // RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=none \
 // RUN:   | FileCheck -strict-whitespace %s
 
-// CHECK: int   i;
+// CHECK: #include 
+// CHECK-NEXT: #include 
+// CHECK-NEXT: int   i;
+#include 
+#include 
 int   i;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1062,8 +1062,6 @@
 FormatStyle getNoStyle() {
   FormatStyle NoStyle = getLLVMStyle();
   NoStyle.DisableFormat = true;
-  NoStyle.SortIncludes = false;
-  NoStyle.SortUsingDeclarations = false;
   return NoStyle;
 }
 
@@ -2130,7 +2128,7 @@
ArrayRef Ranges,
StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
-  if (!Style.SortIncludes)
+  if (!Style.SortIncludes || Style.DisableFormat)
 return Replaces;
   if (isLikelyXml(Code))
 return Replaces;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67843: DisableFormat also now disables SortIncludes

[Sam Maier via Phabricator via cfe-commits]

SamMaier added a comment.

In D67843#1677983 , @MyDeveloperDay 
wrote:

> I assume the intention was that users could have DisableFormat=true and 
> SortIncludes=true when they want to sort the includes but not perform any 
> additional formatting in the code.
>
> I think by making this change you make it impossible to run clang-format 
> through a codebase with the sole intention of just sorting the headers. 
> (which I could see as potentially useful isolated functionality)..
>
> If SortIncludes is false by default? (which you are making it not for no 
> style so I'm unclear what it would be now if you running without a 
> BasedOnStyle setting (uninitialized?)) then you don't need to supply both 
> unless you are using LLVM style or one of the other styles that turn it on.
>
> Are you sure this is the right change?


This is from my experience with Chromium's use of clang-format.

People intuitively use DisableFormat: true to (they think) turn off formatting 
for that subdirectory. The Chromium project has 10 instances of this 
(https://cs.chromium.org/search/?q=file:.clang-format+DisableFormat:%5CWtrue&sq=package:chromium&type=cs).
 The "correct" thing to do here is to specify BasedOnStyle: none if they 
actually want no formatting. If you don't provide a BasedOnStyle, the 
DefaultFallbackStyle is LLVM, which means that SortIncludes is always true 
unless directly overridden.

I see 3 options:

1. Take this change as is. This breaks the ability to only sort includes 
without doing any other formatting. I'm not sure how much sort includes only is 
used, but my guess would be not often. It makes DisableFormat actually disable 
all formatting.
2. Don't change anything. This means that DisableFormat does not disable all 
formatting, despite the docs 
(https://clang.llvm.org/docs/ClangFormatStyleOptions.html) stating it "disables 
formatting completely". The only real way to disable formatting for a directory 
is to use BasedOnStyle: none, or to specify SortIncludes: false, which both are 
pretty unintuitive IMO.
3. Change the default fallback to something else if there is a .clang-format 
file with no BasedOnStyle. This could have farther reaching consequences which 
could affect more users who depend on clang-format just applying LLVM style 
even if they don't specify it.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67843/new/

https://reviews.llvm.org/D67843



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67843: DisableFormat also now disables SortIncludes

[Sam Maier via Phabricator via cfe-commits]

SamMaier added a comment.

In D67843#1679529 , @thakis wrote:

> 4. Make it so that if DisableFormat is explicitly set to true and 
> SortIncludes isn't explicitly set, then it disables SortIncludes. Or, put a 
> different way, when DisableFormat is set, set SortIncludes to false at that 
> point. Then an explicit `DisableFormat: true; SortIncludes: true` would still 
> work.
>
>   MyDeveloperDay, would you find that intuitive?
>
>   I think the patch as-is is fine as I said, but if folks want to sort 
> includes without formatting, that might be another option.


I wasn't sure about this since I didn't see any way of distinguishing an 
explicitly set Style attribute vs an inherited Style attribute from a 
BasedOnStyle. If there's an easy way to distinguish those two, I'm sure this is 
the preferred method.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67843/new/

https://reviews.llvm.org/D67843



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66964: Sort Java imports without newline

[Sam Maier via Phabricator via cfe-commits]

SamMaier added a comment.

In D66964#1651496 , @yannic wrote:

> Right now, this change only adds a test that shows the broken behavior.
>
> SamMaier@ you're the author of https://reviews.llvm.org/D52800 which 
> implemented Java import sorting. Is there a reason to not sort imports when 
> there's more than one in a line?


No reason - it never crossed my mind as a use case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66964/new/

https://reviews.llvm.org/D66964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits