[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Belated 'sounds good to me'.

Other things have been keeping me busy, sorry for the delayed response on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79388

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

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

I'd be very surprised if any of the tests included in this change pass with 
that line commented it's meant so that things like #if and #else properly 
separate alignment after the first preprocessor run, where the whitespace 
manager doesn't have the full context of things between it.

This will break with any PP statement though, not just if-elif-else-endif. I'll 
try moving it to somewhere more specific like the cases in parsePPDirective and 
put out a fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79388

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

[PATCH] D87587: [clang-format] Make one-line namespaces resistant to FixNamespaceComments, update documentation

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD edited reviewers, added: MyDeveloperDay; removed: jmerdich.
JakeMerdichAMD requested changes to this revision.
JakeMerdichAMD added a subscriber: MyDeveloperDay.
JakeMerdichAMD added a comment.
This revision now requires changes to proceed.

After looking at more of the history (ie, the commit you referenced), I'd 
definitely be open to something like this, provided that it doesn't affect 
namespaces that reside completely on one line. Since it was mostly a 
clang-format limitation and relatively rare, I think we can change the default 
here, but that's not up to just me (+@MyDeveloperDay), and extra scrutiny is 
definitely required when changing existing tests.

In any case, can you also add the full diff context 
?
 It makes it easier for us to review.




Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:426-428
+  EXPECT_EQ("namespace A { a }// namespace A",
+fixNamespaceEndComments("namespace A { a }"));
+  EXPECT_EQ("namespace A { a };// namespace A",

I strongly believe that these ones are not correct. Namespaces that are 
entirely on one line should never have a trailing comment, even if it has 
content in it. 

A solution would also have to take into account whether future passes would 
split this onto separate lines (and thus have a different result after 
re-running clang-format), which was the reason for this limitation in the first 
place. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87587

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

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

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

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

[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

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

[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:1705
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+

bc-lee wrote:
> JakeMerdichAMD wrote:
> > 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.
> I understand. The description is somewhat misleading. 
New phrasing makes it clear, but true and false are still inverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

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

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

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD requested changes to this revision.
JakeMerdichAMD added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Format/Format.h:1705
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

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

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

I can see the use of this, but I am also wary that ignoring style options will 
lead to people producing different results on different versions of 
clang-format. This is both because having set-or-unset an option will naturally 
lead to different code and also that newer options are a de-facto check that 
clang-format is at least a certain version (we have minor differences between 
major versions as bugs are fixed). In any case, I see this being *very* easy to 
misuse and the documentation should have a warning reflecting that.

As far as docs go, the bulk are in clang/docs/ClangFormat.rst and require no 
additional 'publish' step but definitely should be updated. The command line 
options are implicitly generated for the cli tool's --help flag, but the docs 
for them are all in that rst file and manually maintained.




Comment at: clang/tools/clang-format/ClangFormat.cpp:108
+static cl::opt
+IgnoreUnkownOptions("ignore-unknown-options",
+cl::desc("If set, unknown format options are 
ignored."),

fodinabor wrote:
> MyDeveloperDay wrote:
> > feels like a mouthful is there nothing shorter we could use?  -Wignore (or 
> > something)
> hmm... `-Wunknown`
> but the `-W` does not really make it clear that the default "errors" should 
> now be treated as warnings instead. From compiler conventions, I'd expect the 
> `-W` to enable a warning ... 
> 
> and something like `-Wno-error=unknown` is not really shorter...
I personally like -Wno-error=unknown if the behavior is to emit warnings and 
-Wno-unknown if the behavior is to be silent, for consistency with clang/gcc.

I would also put a note in the description saying that ignoring options can 
lead to dramatically different output between versions that do and don't 
support a given option, so it should be used with care.



Comment at: llvm/lib/Support/YAMLTraits.cpp:199
+  if (IgnoreUnkown)
+return;
   for (const auto &NN : MN->Mapping) {

grimar wrote:
> fodinabor wrote:
> > MyDeveloperDay wrote:
> > > do we want to flat out ignore or just report but not fatally. (just a 
> > > thought) silent failures are hard to diagnose
> > true.. don't know what's the best option?
> > 
> > keep it as a printed out error and just don't return an error code on exit? 
> > This option would make it a clang-format only change, but feels really 
> > dirty.
> > 
> > Otherwise I'd have to dig my way through to 
> > `llvm::yaml::Stream::printError` (or maybe rather add a `printWarning`) to 
> > conditionally change the message type for the ignore case.
> Yes, I think we might want to introduce a method, like `Input::reportWarning`
> which will call ` Strm->printError(node, message);`, but will not set a `EC`.
> Also, it should print "warning: ..." instead of "error: ..." prefix somehow.
> 
Something like how clang/gcc only report unknown -Wno-foo options if there's 
another error is an idea, but clang-format almost never fails unless there's a 
bad config or input file so that's not too useful.

I'm fine with either warning or being silent. If the user has opted-into 
ignoring missing options, we can assume they're willing to accept the 
consequences of such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137

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

[PATCH] D86713: [clang-format] Parse nullability attributes as a pointer qualifier

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

In D86713#2242300 , @arichardson wrote:

> In D86713#2242253 , @JakeMerdichAMD 
> wrote:
>
>> LGTM, again assuming tests pass locally (patch did not resolve).
>>
>> Out of curiosity, is _Atomic on your radar? I found some code in clang 
>> proper that handled restrict and _Atomic together. C/C++ have way too many 
>> qualifiers...
>
> I have not looked at _Atomic yet and it's probably low priority for me unless 
> it's a trivial change.
> My main motivation with these changes is to format a `__capability` pointer 
> qualifier correctly (an extension that we add for our out-of-tree CHERI C/C++ 
> dialect).

I don't know of anyone who uses it yet, so just adding it for posterity, 
definitely not a blocker. Were you planning on handling `__capability` directly 
or in a user-configurable option? I can imagine other ad-hoc pointer qualifiers 
specific to static analysis tools or as properly-ifdef'd aliases of 
`__attribute__(...)`, so an option might be useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86713

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

[PATCH] D86713: [clang-format] Parse nullability attributes as a pointer qualifier

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.
This revision is now accepted and ready to land.

LGTM, again assuming tests pass locally (patch did not resolve).

Out of curiosity, is _Atomic on your radar? I found some code in clang proper 
that handled restrict and _Atomic together. C/C++ have way too many 
qualifiers...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86713

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

[PATCH] D86710: [clang-format] Parse restrict as a pointer qualifier

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.
This revision is now accepted and ready to land.

LGTM, assuming tests pass (automated checks failed to resolve your patch since 
you based it off of your other one). Looks like enabling C99 should have no 
other effects, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86710

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

[PATCH] D86708: [clang-format] Parse volatile as a pointer qualifier

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.
This revision is now accepted and ready to land.

LGTM. Wait a bit to give others a chance to chime in before submitting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86708

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Agreed that this is a very nice solution for this case. LGTM too assuming it 
passes @MyDeveloperDay's tests.

Minor heads up: there are still some cases I can dream up where we'd currently 
spit out invalid code from splitting a right shift even after this fix. Long 
story short, C++14 made some cases completely ambiguous and impossible to 
detect without semantic analysis (https://godbolt.org/z/rxEqTG). I'll be having 
a fix out for that soon, but it's rather complementary to this one-- yours 
improves template detection, mine acts more conservative in case we guessed 
wrong about it being a template.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D84375: [git-clang-format] Add --diffstat parameter

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added reviewers: MyDeveloperDay, JakeMerdichAMD.
JakeMerdichAMD added a comment.

Reviving this since it looks perfectly fine to me (from my limited commit 
history in git-clang-format :P), is useful, and there's no good reason for it 
to be stalled.

I'll wait a day or two to see if there's any objections, then accept it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84375

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

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

[Jake Merdich via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0c332a7784c6: [clang-format] Preserve whitespace in selected 
macros (authored by JakeMerdichAMD).

Changed prior to commit:
  https://reviews.llvm.org/D82620?vs=273722&id=274102#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82620

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14021,6 +14021,19 @@
   CHECK_PARSE("NamespaceMacros: [TESTSUITE, SUITE]", NamespaceMacros,
   std::vector({"TESTSUITE", "SUITE"}));
 
+  Style.WhitespaceSensitiveMacros.clear();
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+  WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+  WhitespaceSensitiveMacros,
+  std::vector({"STRINGIZE", "ASSERT"}));
+  Style.WhitespaceSensitiveMacros.clear();
+  CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",
+  WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+  CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE', 'ASSERT']",
+  WhitespaceSensitiveMacros,
+  std::vector({"STRINGIZE", "ASSERT"}));
+
   Style.IncludeStyle.IncludeCategories.clear();
   std::vector ExpectedCategories = {
   {"abc/.*", 2, 0}, {".*", 1, 0}};
@@ -16530,6 +16543,36 @@
   verifyFormat("foo(operator, , -42);", Style);
 }
 
+TEST_F(FormatTest, WhitespaceSensitiveMacros) {
+  FormatStyle Style = getLLVMStyle();
+  Style.WhitespaceSensitiveMacros.push_back("FOO");
+
+  // Don't use the helpers here, since 'mess up' will change the whitespace
+  // and these are all whitespace sensitive by definition
+  EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
+  EXPECT_EQ(
+  "FOO(String-ized&Messy+But\\(: :Still)=Intentional);",
+  format("FOO(String-ized&Messy+But\\(: :Still)=Intentional);", Style));
+  EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
+  EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"
+"   Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :\n"
+   "   Still=Intentional);",
+   Style));
+  Style.AlignConsecutiveAssignments = true;
+  EXPECT_EQ("FOO(String-ized=&Messy+But,: :\n"
+"   Still=Intentional);",
+format("FOO(String-ized=&Messy+But,: :\n"
+   "   Still=Intentional);",
+   Style));
+
+  Style.ColumnLimit = 21;
+  EXPECT_EQ("FOO(String-ized&Messy+But: :Still=Intentional);",
+format("FOO(String-ized&Messy+But: :Still=Intentional);", Style));
+}
+
 TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
   // These tests are not in NamespaceFixer because that doesn't
   // test its interaction with line wrapping
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,27 @@
 return false;
   }
 
+  bool parseUntouchableParens() {
+while (CurrentToken) {
+  CurrentToken->Finalized = true;
+  switch (CurrentToken->Tok.getKind()) {
+  case tok::l_paren:
+next();
+if (!parseUntouchableParens())
+  return false;
+continue;
+  case tok::r_paren:
+next();
+return true;
+  default:
+// no-op
+break;
+  }
+  next();
+}
+return false;
+  }
+
   bool parseParens(bool LookForDecls = false) {
 if (!CurrentToken)
   return false;
@@ -171,6 +192,11 @@
 Contexts.back().ColonIsForRangeExpr =
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
+if (Left->Previous && Left->Previous->is(TT_UntouchableMacroFunc)) {
+  Left->Finalized = true;
+  return parseUntouchableParens();
+}
+
 bool StartsObjCMethodExpr = false;
 if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
@@ -1311,7 +1337,7 @@
 TT_TypenameMacro, TT_FunctionLBrace, TT_ImplicitStringLiteral,
 TT_InlineASMBrace, TT_JsFatArrow, TT_LambdaArrow, TT_NamespaceMacro,
 TT_OverloadedOperator, TT_RegexLiteral, TT_TemplateString,
-TT_ObjCStringLiteral))
+TT_ObjCStringLiteral, TT_Untoucha

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Thanks for the fast review, @curdeius, and thanks for mentioning PP_STRINGIZE 
and BOOST_PP_STRINGIZE too!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82620



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

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD updated this revision to Diff 273722.
JakeMerdichAMD marked 3 inline comments as done.
JakeMerdichAMD added a comment.

Address feedback (nits, better docs, more defaults)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82620

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13957,6 +13957,18 @@
   CHECK_PARSE("NamespaceMacros: [TESTSUITE, SUITE]", NamespaceMacros,
   std::vector({"TESTSUITE", "SUITE"}));
 
+  Style.WhitespaceSensitiveMacros.clear();
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+  WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+  WhitespaceSensitiveMacros,
+  std::vector({"STRINGIZE", "ASSERT"}));
+  CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",
+  WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+  CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE', 'ASSERT']",
+  WhitespaceSensitiveMacros,
+  std::vector({"STRINGIZE", "ASSERT"}));
+
   Style.IncludeStyle.IncludeCategories.clear();
   std::vector ExpectedCategories = {
   {"abc/.*", 2, 0}, {".*", 1, 0}};
@@ -16466,6 +16478,36 @@
   verifyFormat("foo(operator, , -42);", Style);
 }
 
+TEST_F(FormatTest, WhitespaceSensitiveMacros) {
+  FormatStyle Style = getLLVMStyle();
+  Style.WhitespaceSensitiveMacros.push_back("FOO");
+
+  // Don't use the helpers here, since 'mess up' will change the whitespace
+  // and these are all whitespace sensitive by definition
+  EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
+  EXPECT_EQ(
+  "FOO(String-ized&Messy+But\\(: :Still)=Intentional);",
+  format("FOO(String-ized&Messy+But\\(: :Still)=Intentional);", Style));
+  EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
+  EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"
+"   Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :\n"
+   "   Still=Intentional);",
+   Style));
+  Style.AlignConsecutiveAssignments = true;
+  EXPECT_EQ("FOO(String-ized=&Messy+But,: :\n"
+"   Still=Intentional);",
+format("FOO(String-ized=&Messy+But,: :\n"
+   "   Still=Intentional);",
+   Style));
+
+  Style.ColumnLimit = 21;
+  EXPECT_EQ("FOO(String-ized&Messy+But: :Still=Intentional);",
+format("FOO(String-ized&Messy+But: :Still=Intentional);", Style));
+}
+
 TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
   // These tests are not in NamespaceFixer because that doesn't
   // test its interaction with line wrapping
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,27 @@
 return false;
   }
 
+  bool parseUntouchableParens() {
+while (CurrentToken) {
+  CurrentToken->Finalized = true;
+  switch (CurrentToken->Tok.getKind()) {
+  case tok::l_paren:
+next();
+if (!parseUntouchableParens())
+  return false;
+continue;
+  case tok::r_paren:
+next();
+return true;
+  default:
+// no-op
+break;
+  }
+  next();
+}
+return false;
+  }
+
   bool parseParens(bool LookForDecls = false) {
 if (!CurrentToken)
   return false;
@@ -171,6 +192,11 @@
 Contexts.back().ColonIsForRangeExpr =
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
+if (Left->Previous && Left->Previous->is(TT_UntouchableMacroFunc)) {
+  Left->Finalized = true;
+  return parseUntouchableParens();
+}
+
 bool StartsObjCMethodExpr = false;
 if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
@@ -1311,7 +1337,7 @@
 TT_TypenameMacro, TT_FunctionLBrace, TT_ImplicitStringLiteral,
 TT_InlineASMBrace, TT_JsFatArrow, TT_LambdaArrow, TT_NamespaceMacro,
 TT_OverloadedOperator, TT_RegexLiteral, TT_TemplateString,
-TT_ObjCStringLiteral))
+TT_ObjCStringLiteral, TT_UntouchableMacroFunc))
   CurrentToken->setType(TT_Unknown);
 CurrentToken->Role.reset();
 CurrentToken->MatchingParen = nullptr

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD marked 10 inline comments as done.
JakeMerdichAMD added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2706
+
+  For example: STRINGIZE
+

curdeius wrote:
> Shouldn't there be a configuration example like what's in `ForEachMacros` doc?
> ```
>   In the .clang-format configuration file, this can be configured like:
> 
>   .. code-block:: yaml
> 
> WhitespaceSensitiveMacros: ['STRINGIZE', 'PP_STRINGIZE']
> 
>   For example: BOOST_PP_STRINGIZE.
> ```
> 
Done. I also added PP_STRINGIZE and BOOST_PP_STRINGIZE as defaults; seems 
reasonable.



Comment at: clang/unittests/Format/FormatTest.cpp:13961
+  Style.WhitespaceSensitiveMacros.clear();
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+  WhitespaceSensitiveMacros, 
std::vector{"STRINGIZE"});

curdeius wrote:
> Shouldn't that be:
> `CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",`
> as in other options that take vector of strings?
I'll add it since it ought to be tested, but they aren't required to my 
knowledge due to 'YAML reasons' and most of the other tests omit them.



Comment at: clang/unittests/Format/FormatTest.cpp:16482
+  // and these are all whitespace sensitive by definition
+  EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", 
Style));

curdeius wrote:
> How about a test with escaped parentheses `\(` inside the macro argument?
Done. Note that parens always need to be matched anyhow, and the escaping 
doesn't actually mean anything outside of a string literal (tested on MSVC, 
gcc, clang), so no functionality change is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82620



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

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD created this revision.
Herald added subscribers: cfe-commits, kristof.beyls.
Herald added a project: clang.
JakeMerdichAMD added reviewers: MyDeveloperDay, curdeius, sammccall, jbcoe.
JakeMerdichAMD added a project: clang-format.

https://bugs.llvm.org/show_bug.cgi?id=46383

When the c preprocessor stringizes tokens, the generated string literals
are affected by the whitespace. This means clang-format can affect
codegen silently, adding spaces and newlines to strings.  Practically
speaking, the vast majority of cases will be harmless, only affecting
single identifiers or debug macros.

In the interest of doing no harm in other cases though, this introduces
a blacklist option 'WhitespaceSensitiveMacros', which contains a list of
names of function-like macros whose contents should not be touched by
clang-format, period. Clang-format can't automatically detect these
without a real compile context, so users will have to specify it
explicitly (it still beats clang-format off'ing at every invocation).

I added one default, "STRINGIZE", but am not particularly attached to
it, nor have I given much thought to defaults.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82620

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13957,6 +13957,13 @@
   CHECK_PARSE("NamespaceMacros: [TESTSUITE, SUITE]", NamespaceMacros,
   std::vector({"TESTSUITE", "SUITE"}));
 
+  Style.WhitespaceSensitiveMacros.clear();
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+  WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+  CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+  WhitespaceSensitiveMacros,
+  std::vector({"STRINGIZE", "ASSERT"}));
+
   Style.IncludeStyle.IncludeCategories.clear();
   std::vector ExpectedCategories = {
   {"abc/.*", 2, 0}, {".*", 1, 0}};
@@ -16466,6 +16473,33 @@
   verifyFormat("foo(operator, , -42);", Style);
 }
 
+TEST_F(FormatTest, WhitespaceSensitiveMacros) {
+  FormatStyle Style = getLLVMStyle();
+  Style.WhitespaceSensitiveMacros.push_back("FOO");
+
+  // Don't use the helpers here, since 'mess up' will change the whitespace
+  // and these are all whitespace sensitive by definition
+  EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
+  EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
+  EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"
+"   Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :\n"
+   "   Still=Intentional);",
+   Style));
+  Style.AlignConsecutiveAssignments = true;
+  EXPECT_EQ("FOO(String-ized=&Messy+But,: :\n"
+"   Still=Intentional);",
+format("FOO(String-ized=&Messy+But,: :\n"
+   "   Still=Intentional);",
+   Style));
+
+  Style.ColumnLimit = 21;
+  EXPECT_EQ("FOO(String-ized&Messy+But: :Still=Intentional);",
+format("FOO(String-ized&Messy+But: :Still=Intentional);", Style));
+}
+
 TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
   // These tests are not in NamespaceFixer because that doesn't
   // test its interaction with line wrapping
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,27 @@
 return false;
   }
 
+  bool parseUntouchableParens() {
+while (CurrentToken) {
+  CurrentToken->Finalized = true;
+  switch (CurrentToken->Tok.getKind()) {
+  case tok::l_paren:
+next();
+if (!parseUntouchableParens())
+  return false;
+continue;
+  case tok::r_paren:
+next();
+return true;
+  default:
+// no-op
+break;
+  }
+  next();
+}
+return false;
+  }
+
   bool parseParens(bool LookForDecls = false) {
 if (!CurrentToken)
   return false;
@@ -171,6 +192,11 @@
 Contexts.back().ColonIsForRangeExpr =
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
+if (Left->Previous && Left->Previous->is(TT_UntouchableMacroFunc)) {
+  Left->Finalized = true;
+  return parseUntouchableParens();
+}
+
 bool StartsObjCMethodExpr = false;
 if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
@@ -1311,7 +1337,7 @@

[PATCH] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Great idea on this! I may borrow this idea and make something similar for some 
migrations I'm working on.

Sanity check: is this going to be run automatically when docs are generated or 
done offline with results committed? I don't have a preference, either has pros 
and cons, but I'm confused as to when this script will be run.


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

https://reviews.llvm.org/D80627



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

[PATCH] D80486: [clang-format][PR46043] Parse git config w/ implicit values

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD created this revision.
JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, sammccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

https://bugs.llvm.org/show_bug.cgi?id=46043

Git's config is generally of the format 'key=val', but a setting
'key=true' can be written as just 'key'.  The git-clang-format script
expects a value and crashes in this case; this change handles implicit
'true' values in the script.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80486

Files:
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -192,7 +192,12 @@
   out = {}
   for entry in run('git', 'config', '--list', '--null').split('\0'):
 if entry:
-  name, value = entry.split('\n', 1)
+  if '\n' in entry:
+name, value = entry.split('\n', 1)
+  else:
+# A setting with no '=' ('\n' with --null) is implicitly 'true'
+name = entry
+value = 'true'
   if name in non_string_options:
 value = run('git', 'config', non_string_options[name], name)
   out[name] = value


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -192,7 +192,12 @@
   out = {}
   for entry in run('git', 'config', '--list', '--null').split('\0'):
 if entry:
-  name, value = entry.split('\n', 1)
+  if '\n' in entry:
+name, value = entry.split('\n', 1)
+  else:
+# A setting with no '=' ('\n' with --null) is implicitly 'true'
+name = entry
+value = 'true'
   if name in non_string_options:
 value = run('git', 'config', non_string_options[name], name)
   out[name] = value
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80079: [clang-format] [NFC] isCpp() is inconsistently used to mean both C++ and Objective C, add language specific isXXX() functions

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

I'm a fan of the 'like' helpers, but I'm not entirely convinced that having 
helpers for languages covered by the 'like' is a bad thing-- it just needs to 
be very explicit that you do mean only that language. For example, an 
'isObjCOnly()' would hint to reviewers that ObjC is sometimes used in 
combination with other languages and there may be a more appropriate helper (of 
course, this should be clearly documented as well).

Ultimately, I don't really care one way or the other about this, except maybe 
for the current misnomer of isCpp(), but that's my 2c.


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

https://reviews.llvm.org/D80079



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

[PATCH] D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.

LGTM


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

https://reviews.llvm.org/D80214



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

[PATCH] D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Just belatedly caught something: Webkit style is supported too but not listed 
here. Can you add that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80214



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

[PATCH] D80309: [clang-format][docfix] Update predefined styles in docs

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, mitchell-stellar, 
sammccall.
JakeMerdichAMD added a project: clang-format.

The predefined styles that clang-format supports are listed in two
places, and neither is up-to-date. GNU style isn't mentioned at all!


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80309

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/LibFormat.rst


Index: clang/docs/LibFormat.rst
===
--- clang/docs/LibFormat.rst
+++ clang/docs/LibFormat.rst
@@ -40,7 +40,7 @@
 
 The style options describe specific formatting options that can be used in
 order to make `ClangFormat` comply with different style guides. Currently,
-two style guides are hard-coded:
+several style guides are hard-coded:
 
 .. code-block:: c++
 
@@ -52,6 +52,26 @@
   /// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.
   FormatStyle getGoogleStyle();
 
+  /// Returns a format style complying with Chromium's style guide:
+  /// 
https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md
+  FormatStyle getChromiumStyle();
+
+  /// Returns a format style complying with the GNU coding standards:
+  /// https://www.gnu.org/prep/standards/standards.html
+  FormatStyle getGNUStyle();
+
+  /// Returns a format style complying with Mozilla's style guide
+  /// 
https://firefox-source-docs.mozilla.org/code-quality/coding-style/index.html
+  FormatStyle getMozillaStyle();
+
+  /// Returns a format style complying with Webkit's style guide:
+  /// https://webkit.org/code-style-guidelines/
+  FormatStyle getWebkitStyle();
+
+  /// Returns a format style complying with Microsoft's style guide:
+  /// 
https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference
+  FormatStyle getMicrosoftStyle();
+
 These options are also exposed in the :doc:`standalone tools `
 through the `-style` option.
 
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -151,6 +151,9 @@
   * ``Microsoft``
 A style complying with `Microsoft's style guide
 
`_
+  * ``GNU``
+A style complying with the `GNU coding standards
+`_
 
 .. START_FORMAT_STYLE_OPTIONS
 


Index: clang/docs/LibFormat.rst
===
--- clang/docs/LibFormat.rst
+++ clang/docs/LibFormat.rst
@@ -40,7 +40,7 @@
 
 The style options describe specific formatting options that can be used in
 order to make `ClangFormat` comply with different style guides. Currently,
-two style guides are hard-coded:
+several style guides are hard-coded:
 
 .. code-block:: c++
 
@@ -52,6 +52,26 @@
   /// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.
   FormatStyle getGoogleStyle();
 
+  /// Returns a format style complying with Chromium's style guide:
+  /// https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md
+  FormatStyle getChromiumStyle();
+
+  /// Returns a format style complying with the GNU coding standards:
+  /// https://www.gnu.org/prep/standards/standards.html
+  FormatStyle getGNUStyle();
+
+  /// Returns a format style complying with Mozilla's style guide
+  /// https://firefox-source-docs.mozilla.org/code-quality/coding-style/index.html
+  FormatStyle getMozillaStyle();
+
+  /// Returns a format style complying with Webkit's style guide:
+  /// https://webkit.org/code-style-guidelines/
+  FormatStyle getWebkitStyle();
+
+  /// Returns a format style complying with Microsoft's style guide:
+  /// https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference
+  FormatStyle getMicrosoftStyle();
+
 These options are also exposed in the :doc:`standalone tools `
 through the `-style` option.
 
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -151,6 +151,9 @@
   * ``Microsoft``
 A style complying with `Microsoft's style guide
 `_
+  * ``GNU``
+A style complying with the `GNU coding standards
+`_
 
 .. START_FORMAT_STYLE_OPTIONS
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

+1 for this idea. It'd eventually be neat to also take all samples from the 
style guide of each project and test them, if there aren't licensing concerns.

LGTM with an appropriate merge fix/CI passing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80214



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

[PATCH] D79465: [clang-format] Fix line lengths w/ comments in align

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Hey @MyDeveloperDay, can I get your assistance committing this when you have 
the chance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79465



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

[PATCH] D80176: [clang-format][PR45816] Add AlignConsecutiveBitFields

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Can I get your assistance committing this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80176



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

[PATCH] D80176: [clang-format][PR45816] Add AlignConsecutiveBitFields

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD updated this revision to Diff 265048.
JakeMerdichAMD added a comment.

Reformat with a newer clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80176

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11955,6 +11955,48 @@
Alignment);
 }
 
+TEST_F(FormatTest, AlignConsecutiveBitFields) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveBitFields = true;
+  verifyFormat("int const a : 5;\n"
+   "int oneTwoThree : 23;",
+   Alignment);
+
+  // Initializers are allowed starting with c++2a
+  verifyFormat("int const a : 5 = 1;\n"
+   "int oneTwoThree : 23 = 0;",
+   Alignment);
+
+  Alignment.AlignConsecutiveDeclarations = true;
+  verifyFormat("int const a   : 5;\n"
+   "int   oneTwoThree : 23;",
+   Alignment);
+
+  verifyFormat("int const a   : 5;  // comment\n"
+   "int   oneTwoThree : 23; // comment",
+   Alignment);
+
+  verifyFormat("int const a   : 5 = 1;\n"
+   "int   oneTwoThree : 23 = 0;",
+   Alignment);
+
+  Alignment.AlignConsecutiveAssignments = true;
+  verifyFormat("int const a   : 5  = 1;\n"
+   "int   oneTwoThree : 23 = 0;",
+   Alignment);
+  verifyFormat("int const a   : 5  = {1};\n"
+   "int   oneTwoThree : 23 = 0;",
+   Alignment);
+
+  // Known limitations: ':' is only recognized as a bitfield colon when
+  // followed by a number.
+  /*
+  verifyFormat("int oneTwoThree : SOME_CONSTANT;\n"
+   "int a   : 5;",
+   Alignment);
+  */
+}
+
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveMacros = true;
@@ -13369,6 +13411,7 @@
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
+  CHECK_PARSE_BOOL(AlignConsecutiveBitFields);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
   CHECK_PARSE_BOOL(AlignConsecutiveMacros);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
Index: clang/lib/Format/WhitespaceManager.h
===
--- clang/lib/Format/WhitespaceManager.h
+++ clang/lib/Format/WhitespaceManager.h
@@ -184,6 +184,9 @@
   /// Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
+  /// Align consecutive bitfields over all \c Changes.
+  void alignConsecutiveBitFields();
+
   /// Align consecutive declarations over all \c Changes.
   void alignConsecutiveDeclarations();
 
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -95,6 +95,7 @@
   calculateLineBreakInformation();
   alignConsecutiveMacros();
   alignConsecutiveDeclarations();
+  alignConsecutiveBitFields();
   alignConsecutiveAssignments();
   alignChainedConditionals();
   alignTrailingComments();
@@ -609,6 +610,26 @@
   Changes, /*StartAt=*/0);
 }
 
+void WhitespaceManager::alignConsecutiveBitFields() {
+  if (!Style.AlignConsecutiveBitFields)
+return;
+
+  AlignTokens(
+  Style,
+  [&](Change const &C) {
+// Do not align on ':' that is first on a line.
+if (C.NewlinesBefore > 0)
+  return false;
+
+// Do not align on ':' that is last on a line.
+if (&C != &Changes.back() && (&C + 1)->NewlinesBefore > 0)
+  return false;
+
+return C.Tok->is(TT_BitFieldColon);
+  },
+  Changes, /*StartAt=*/0);
+}
+
 void WhitespaceManager::alignConsecutiveDeclarations() {
   if (!Style.AlignConsecutiveDeclarations)
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -403,6 +403,8 @@
 IO.mapOptional("AlignConsecutiveMacros", Style.AlignConsecutiveMacros);
 IO.mapOptional("AlignConsecutiveAssignments",
Style.AlignConsecutiveAssignments);
+IO.mapOptional("AlignConsecutiveBitFields",
+   Style.AlignConsecutiveBitFields);
 IO.mapOptional("AlignConsecutiveDeclarations",
Style.AlignConsecutiveDeclarations);
 IO.mapOptional("AlignEscapedNewlines", Style.Alig

[PATCH] D80144: [clang-format] @lefticus just taught the world how to use [[unlikely]] but we forgot to teach clang-format

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.
This revision is now accepted and ready to land.

LGTM assuming CI tests pass.


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

https://reviews.llvm.org/D80144



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

[PATCH] D80115: [clang-format] [PR45816] Add AlignConsecutiveBitFields

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

See follow-up in D80176 .


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

https://reviews.llvm.org/D80115



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

[PATCH] D80176: [clang-format][PR45816] Add AlignConsecutiveBitFields

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The following revision follows https://reviews.llvm.org/D80115 since
@MyDeveloperDay and I apparently both had the same idea at the same
time, for https://bugs.llvm.org/show_bug.cgi?id=45816 and my efforts
on tooling support for AMDVLK, respectively.

This option aligns adjacent bitfield separators across lines, in a
manner similar to AlignConsecutiveAssignments and friends.

Example:

  struct RawFloat {
uint32_t sign : 1;
uint32_t exponent : 8;
uint32_t mantissa : 23;
  };

would become

  struct RawFloat {
uint32_t sign : 1;
uint32_t exponent : 8;
uint32_t mantissa : 23;
  };

This also handles c++2a style bitfield-initializers with
AlignConsecutiveAssignments.

  struct RawFloat {
uint32_t sign : 1  = 0;
uint32_t exponent : 8  = 127;
uint32_t mantissa : 23 = 0;
  }; // defaults to 1.0f

Things this change does not do:

- Align multiple comma-chained bitfield variables. None of the other 
AlignConsecutive* options seem to implement that either.
- Detect bitfields that have a width specified with something other than a 
numeric literal (ie, 'int a : SOME_MACRO;'). That'd be fairly difficult to 
parse and is rare.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80176

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11955,6 +11955,48 @@
Alignment);
 }
 
+TEST_F(FormatTest, AlignConsecutiveBitFields) {
+  FormatStyle Alignment = getLLVMStyle();
+  Alignment.AlignConsecutiveBitFields = true;
+  verifyFormat("int const a : 5;\n"
+   "int oneTwoThree : 23;",
+   Alignment);
+
+  // Initializers are allowed starting with c++2a
+  verifyFormat("int const a : 5 = 1;\n"
+   "int oneTwoThree : 23 = 0;",
+   Alignment);
+
+  Alignment.AlignConsecutiveDeclarations = true;
+  verifyFormat("int const a   : 5;\n"
+   "int   oneTwoThree : 23;",
+   Alignment);
+
+  verifyFormat("int const a   : 5;  // comment\n"
+   "int   oneTwoThree : 23; // comment",
+   Alignment);
+
+  verifyFormat("int const a   : 5 = 1;\n"
+   "int   oneTwoThree : 23 = 0;",
+   Alignment);
+
+  Alignment.AlignConsecutiveAssignments = true;
+  verifyFormat("int const a   : 5  = 1;\n"
+   "int   oneTwoThree : 23 = 0;",
+   Alignment);
+  verifyFormat("int const a   : 5  = {1};\n"
+   "int   oneTwoThree : 23 = 0;",
+   Alignment);
+
+  // Known limitations: ':' is only recognized as a bitfield colon when
+  // followed by a number.
+  /*
+  verifyFormat("int oneTwoThree : SOME_CONSTANT;\n"
+   "int a   : 5;",
+   Alignment);
+  */
+}
+
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveMacros = true;
@@ -13369,6 +13411,7 @@
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
+  CHECK_PARSE_BOOL(AlignConsecutiveBitFields);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
   CHECK_PARSE_BOOL(AlignConsecutiveMacros);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
Index: clang/lib/Format/WhitespaceManager.h
===
--- clang/lib/Format/WhitespaceManager.h
+++ clang/lib/Format/WhitespaceManager.h
@@ -184,6 +184,9 @@
   /// Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
+  /// Align consecutive bitfields over all \c Changes.
+  void alignConsecutiveBitFields();
+
   /// Align consecutive declarations over all \c Changes.
   void alignConsecutiveDeclarations();
 
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -95,6 +95,7 @@
   calculateLineBreakInformation();
   alignConsecutiveMacros();
   alignConsecutiveDeclarations();
+  alignConsecutiveBitFields();
   alignConsecutiveAssignments();
   alignChainedConditionals();
   alignTrailingComments();
@@ -609,6 +610,25 @@
   Changes, /*StartAt=*/0);
 }
 
+void WhitespaceManager::alignConsecutiveBitFields() {
+  if (!Style.AlignConsecutiveBitFields)
+return;
+
+  AlignTokens(Style,
+  [&](Change const &C) {
+ 

[PATCH] D80144: [clang-format] @lefticus just taught the world how to use [[unlikely]] but we forgot to teach clang-format

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

This is a great improvement in readability. I think this will get in here 
before it does for clang proper :D

Likely/unlikely also seem to be supported on while, do-while, and for loops 
(case statements too, but that's not relevant). Should we also apply identical 
logic there? Far less useful/common than plain if's so it's not a blocker, but 
it's best to be consistent and I can imagine some decent use cases.


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

https://reviews.llvm.org/D80144



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

[PATCH] D80041: [clang-format] [PR45198] deletes whitespace inside of a noexcept specifier

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

I mainly have concerns with the search for noexcept being too brittle and not 
looking far enough back. Implementing that may have performance implications 
though, so I have no issue ignoring that problem if it's proven sufficiently 
rare.

Also: should this do the same thing for throw()? It has pretty much the same 
constraints/context and would be trivial to add, although it's deprecated 
and/or removed in recent C++ versions. I'm not particularly inclined to 
implement that, but the question ought to be raised since it theoretically has 
the same problem.




Comment at: clang/lib/Format/TokenAnnotator.cpp:1933
+if (PrevPrevToken) {
+  const FormatToken *PrevPrevPrevToken =
+  PrevPrevToken->getPreviousNonComment();

Doesn't this not handle nested templates? For example, 
`noexcept(Foo>::value && Baz)` would not work here. Ditto on expressions 
like `Foo`.

I'm having trouble thinking of a clean generic solution, but the most robust 
thing I can think of is to search the current line for the previous opening 
parens and check if noexcept is before it (if they use multiple sets of parens, 
the other heuristics seem to cover it). 


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

https://reviews.llvm.org/D80041



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

[PATCH] D80115: [clang-format] [PR45816] Add AlignConsecutiveBitFields

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Good idea, feel free to incorporate anything about the change (eg, tests and 
TT_BitFieldColon) or delegate to me if you like.


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

https://reviews.llvm.org/D80115



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

[PATCH] D79905: [clang-format] [PR44476] Add space between template and attribute

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD accepted this revision.
JakeMerdichAMD added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D79905



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

[PATCH] D80115: [clang-format] [PR45816] Add AlignConsecutiveBitFields

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Ironically, I independently made a near-identical change because my codebase 
needs this, and was going to upload it today. I won't post it now, but I stuck 
a copy on github if you're interested: 
https://github.com/JakeMerdichAMD/llvm-project/commit/6eec9ce0bb7732a519b765659422b72a9fa8aa67.
 I also have some more tests there, and will cross-check with this once the 
merge conflicts are fixed.




Comment at: clang/lib/Format/WhitespaceManager.cpp:98
   alignConsecutiveAssignments();
+  alignConsecutiveBitFields();
   alignTrailingComments();

This needs to be before assignments, because 'int foo : 3 = 1;' is valid in 
c++2a.



Comment at: clang/lib/Format/WhitespaceManager.cpp:588-590
+return C.Tok->Previous->startsSequence(tok::identifier, tok::colon,
+   tok::numeric_constant);
+  },

TT_BitFieldColon should match this criteria exactly. 


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

https://reviews.llvm.org/D80115



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

[PATCH] D79465: [clang-format] Fix line lengths w/ comments in align

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD updated this revision to Diff 264294.
JakeMerdichAMD added a comment.

Rebase to fix merge conflict


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79465

Files:
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11953,6 +11953,16 @@
"  x = 1;\n"
"y = 1;\n",
Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+"int yy  = 1; /// specificlennospace\n"
+"int zzz = 2;\n",
+format("int x   = 0;\n"
+   "int yy  = 1; ///specificlennospace\n"
+   "int zzz = 2;\n",
+   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -445,8 +445,16 @@
 
 unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
 int LineLengthAfter = Changes[i].TokenLength;
-for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j)
-  LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+  LineLengthAfter += Changes[j].Spaces;
+  // Changes are generally 1:1 with the tokens, but a change could also be
+  // inside of a token, in which case it's counted more than once: once for
+  // the whitespace surrounding the token (!IsInsideToken) and once for
+  // each whitespace change within it (IsInsideToken).
+  // Therefore, changes inside of a token should only count the space.
+  if (!Changes[j].IsInsideToken)
+LineLengthAfter += Changes[j].TokenLength;
+}
 unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
 // If we are restricted by the maximum column width, end the sequence.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11953,6 +11953,16 @@
"  x = 1;\n"
"y = 1;\n",
Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+"int yy  = 1; /// specificlennospace\n"
+"int zzz = 2;\n",
+format("int x   = 0;\n"
+   "int yy  = 1; ///specificlennospace\n"
+   "int zzz = 2;\n",
+   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -445,8 +445,16 @@
 
 unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
 int LineLengthAfter = Changes[i].TokenLength;
-for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j)
-  LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+  LineLengthAfter += Changes[j].Spaces;
+  // Changes are generally 1:1 with the tokens, but a change could also be
+  // inside of a token, in which case it's counted more than once: once for
+  // the whitespace surrounding the token (!IsInsideToken) and once for
+  // each whitespace change within it (IsInsideToken).
+  // Therefore, changes inside of a token should only count the space.
+  if (!Changes[j].IsInsideToken)
+LineLengthAfter += Changes[j].TokenLength;
+}
 unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
 // If we are restricted by the maximum column width, end the sequence.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79465: [clang-format] Fix line lengths w/ comments in align

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD updated this revision to Diff 264259.
JakeMerdichAMD added a comment.

Add a comment explaining why checking IsInsideToken is needed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79465

Files:
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11564,6 +11564,16 @@
"  x = 1;\n"
"y = 1;\n",
Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+"int yy  = 1; /// specificlennospace\n"
+"int zzz = 2;\n",
+format("int x   = 0;\n"
+   "int yy  = 1; ///specificlennospace\n"
+   "int zzz = 2;\n",
+   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -410,8 +410,16 @@
 
 unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
 int LineLengthAfter = -Changes[i].Spaces;
-for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
-  LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+  LineLengthAfter += Changes[j].Spaces;
+  // Changes are generally 1:1 with the tokens, but a change could also be
+  // inside of a token, in which case it's counted more than once: once for
+  // the whitespace surrounding the token (!IsInsideToken) and once for
+  // each whitespace change within it (IsInsideToken).
+  // Therefore, changes inside of a token should only count the space.
+  if (!Changes[j].IsInsideToken)
+LineLengthAfter += Changes[j].TokenLength;
+}
 unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
 // If we are restricted by the maximum column width, end the sequence.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11564,6 +11564,16 @@
"  x = 1;\n"
"y = 1;\n",
Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+"int yy  = 1; /// specificlennospace\n"
+"int zzz = 2;\n",
+format("int x   = 0;\n"
+   "int yy  = 1; ///specificlennospace\n"
+   "int zzz = 2;\n",
+   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -410,8 +410,16 @@
 
 unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
 int LineLengthAfter = -Changes[i].Spaces;
-for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
-  LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+  LineLengthAfter += Changes[j].Spaces;
+  // Changes are generally 1:1 with the tokens, but a change could also be
+  // inside of a token, in which case it's counted more than once: once for
+  // the whitespace surrounding the token (!IsInsideToken) and once for
+  // each whitespace change within it (IsInsideToken).
+  // Therefore, changes inside of a token should only count the space.
+  if (!Changes[j].IsInsideToken)
+LineLengthAfter += Changes[j].TokenLength;
+}
 unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
 // If we are restricted by the maximum column width, end the sequence.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79905: [clang-format] [PR44476] Add space between template and attribute

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

One spelling nit, but otherwise looks good to me.




Comment at: clang/lib/Format/TokenAnnotator.cpp:2900
+
+  // Space between tempalte and attribute
+  // e.g. template  [[nodiscard]] ...

Nit: template


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79905



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

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Thanks for the commit and review @MyDeveloperDay!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79388



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

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Hey, @MyDeveloperDay, can I get your assistance in committing this? It's 
probably been long enough for anyone to chime in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79388



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

[PATCH] D79465: Fix line lengths w/ comments in align

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Mind looking again? They did add one later on I think. It helped a lot since 
the exact settings and whitespace to trigger this are really finicky (though 
reproducible). You're right that it won't reproduce without it.

Here's a run log of the failing test I added, with the functional change 
commented out so it actually fails. Does this reproduce on your machine?

  jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ git log --oneline -n2
  9b146e8534b (HEAD -> pr43845) Fix line lengths w/ comments in align
  d05f8a38c54 (origin/master, origin/HEAD) [ARM] VMOVrh of VMOVhr
  jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ git diff
  diff --git a/clang/lib/Format/WhitespaceManager.cpp 
b/clang/lib/Format/WhitespaceManager.cpp
  index 5f6bde9f2d4..b2c6e6a16a2 100644
  --- a/clang/lib/Format/WhitespaceManager.cpp
  +++ b/clang/lib/Format/WhitespaceManager.cpp
  @@ -412,7 +412,7 @@ static unsigned AlignTokens(const FormatStyle &Style, F 
&&Matches,
   int LineLengthAfter = -Changes[i].Spaces;
   for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j) {
 LineLengthAfter += Changes[j].Spaces;
  -  if (!Changes[j].IsInsideToken)
  +  //if (!Changes[j].IsInsideToken)
   LineLengthAfter += Changes[j].TokenLength;
   }
   unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
  jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ ninja -j12 FormatTests
  ninja: no work to do.
  jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ 
tools/clang/unittests/Format/FormatTests --gtest_filter="*AlignConsecutiveAss*"
  Note: Google Test filter = *AlignConsecutiveAss*
  [==] Running 1 test from 1 test case.
  [--] Global test environment set-up.
  [--] 1 test from FormatTest
  [ RUN  ] FormatTest.AlignConsecutiveAssignments
  /opt/ws/llvm-project/clang/unittests/Format/FormatTest.cpp:11576: Failure
Expected: "int x   = 0;\n" "int yy  = 1; /// specificlennospace\n" "int 
zzz = 2;\n"
Which is: "int x   = 0;\nint yy  = 1; /// specificlennospace\nint zzz = 
2;\n"
  To be equal to: format("int x   = 0;\n" "int yy  = 1; 
///specificlennospace\n" "int zzz = 2;\n", Alignment)
Which is: "int x = 0;\nint yy = 1; /// specificlennospace\nint zzz = 
2;\n"
  With diff:
  @@ -1,3 +1,3 @@
  -int x   = 0;
  -int yy  = 1; /// specificlennospace
  +int x = 0;
  +int yy = 1; /// specificlennospace
   int zzz = 2;\n
  
  [  FAILED  ] FormatTest.AlignConsecutiveAssignments (126 ms)
  [--] 1 test from FormatTest (126 ms total)
  
  [--] Global test environment tear-down
  [==] 1 test from 1 test case ran. (127 ms total)
  [  PASSED  ] 0 tests.
  [  FAILED  ] 1 test, listed below:
  [  FAILED  ] FormatTest.AlignConsecutiveAssignments
  
   1 FAILED TEST
  jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79465



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

[PATCH] D79465: Fix line lengths w/ comments in align

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a project: clang-format.
JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, VelocityRa, 
sylvestre.ledru, sammccall, enyquist.
JakeMerdichAMD added a comment.

The failing case in this commit looks like the following after formatting (with 
alignconsecutiveassignments and a specific column limit)

  int x = 0;
  int yy = 1; ///specificlennospace
  int zzz = 2;

See PR43845 for more failing cases besides this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79465



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

[PATCH] D79465: Fix line lengths w/ comments in align

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
JakeMerdichAMD added a project: clang-format.
JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, VelocityRa, 
sylvestre.ledru, sammccall, enyquist.
JakeMerdichAMD added a comment.

The failing case in this commit looks like the following after formatting (with 
alignconsecutiveassignments and a specific column limit)

  int x = 0;
  int yy = 1; ///specificlennospace
  int zzz = 2;

See PR43845 for more failing cases besides this.


When a '//comment' trails a consecutive alignment, it adds a whitespace
replacement within the comment token. This wasn't handled correctly in
the alignment code, which treats it as a whole token and thus double
counts it.

This can wrongly trigger the "line too long, it'll wrap" alignment-break
condition with specific lengths, causing the alignment to break for
seemingly no reason.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79465

Files:
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11564,6 +11564,16 @@
"  x = 1;\n"
"y = 1;\n",
Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+"int yy  = 1; /// specificlennospace\n"
+"int zzz = 2;\n",
+format("int x   = 0;\n"
+   "int yy  = 1; ///specificlennospace\n"
+   "int zzz = 2;\n",
+   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -410,8 +410,11 @@
 
 unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
 int LineLengthAfter = -Changes[i].Spaces;
-for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
-  LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+  LineLengthAfter += Changes[j].Spaces;
+  if (!Changes[j].IsInsideToken)
+LineLengthAfter += Changes[j].TokenLength;
+}
 unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
 // If we are restricted by the maximum column width, end the sequence.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11564,6 +11564,16 @@
"  x = 1;\n"
"y = 1;\n",
Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+"int yy  = 1; /// specificlennospace\n"
+"int zzz = 2;\n",
+format("int x   = 0;\n"
+   "int yy  = 1; ///specificlennospace\n"
+   "int zzz = 2;\n",
+   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -410,8 +410,11 @@
 
 unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
 int LineLengthAfter = -Changes[i].Spaces;
-for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
-  LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+  LineLengthAfter += Changes[j].Spaces;
+  if (!Changes[j].IsInsideToken)
+LineLengthAfter += Changes[j].TokenLength;
+}
 unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
 // If we are restricted by the maximum column width, end the sequence.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Sure, I'll get started on that. It mainly comes from charging headfirst into 
the edge cases, but I think I have a decent grasp of clang-format internals 
now, and I'm definitely interested in both contributing to and reviewing for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79388



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

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

@MyDeveloperDay, you're right about what this issue addresses: it surprised me 
a lot when an unrelated edit caused something to 'randomly' add spaces 
elsewhere, since it's in a different ifdef block (whether or not it has the 
same condition). The code that's aligned should always be across lines that are 
visually consecutive, and those ones weren't (in most cases, they aren't even 
logically consecutive).

You're also right that I don't have commit access; this is my first commit in 
LLVM.

> I'm struggling with the simplicity of your solution ;-) (that's a compliment!)

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79388



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

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Currently the 'AlignConsecutive*' options incorrectly align across
elif and else statements, even if they are very far away and across
unrelated preprocessor macros.

This failed since on preprocessor run 2+, there is not enough context
about the #ifdefs to actually differentiate one block from another,
causing them to align across different blocks or even large sections of
the file.

Eg, with AlignConsecutiveAssignments:

  \#if FOO  // Run 1
  \#else// Run 1
  int a   = 1;  // Run 2, wrong
  \#endif   // Run 1
  
  \#if FOO  // Run 1
  \#else// Run 1
  int bar = 1;  // Run 2
  \#endif   // Run 1

is read as

  int a   = 1;  // Run 2, wrong
  int bar = 1;  // Run 2

The approach taken to fix this was to add a new flag to Token that
forces breaking alignment across groups of lines (MustBreakAlignBefore)
in a similar manner to the existing flag that forces a line break
(MustBreakBefore). This flag is set for the first Token after a
preprocessor statement or diff conflict marker.

Possible alternatives might be hashing preprocessor state to detect if
two lines come from the same block (but the way that ifdefs are
sometimes deferred makes that tricky) or showing the preprocessor
statements on all passes instead of just the first one (seems harder).

Fixes #25167,#31281


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79388

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -2780,6 +2780,27 @@
"   // line 2 about b\n"
"   long b;",
getLLVMStyleWithColumns(80)));
+
+  // Checks an edge case in preprocessor handling.
+  // These comments should *not* be aligned
+  EXPECT_EQ(
+  "#if FOO\n"
+  "#else\n"
+  "long a; // Line about a\n"
+  "#endif\n"
+  "#if BAR\n"
+  "#else\n"
+  "long b_long_name; // Line about b\n"
+  "#endif\n",
+  format("#if FOO\n"
+ "#else\n"
+ "long a;   // Line about a\n" // Previous (bad) behavior
+ "#endif\n"
+ "#if BAR\n"
+ "#else\n"
+ "long b_long_name; // Line about b\n"
+ "#endif\n",
+ getLLVMStyleWithColumns(80)));
 }
 
 TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11429,6 +11429,29 @@
   verifyFormat("int oneTwoThree = 123; // comment\n"
"int oneTwo  = 12;  // comment",
Alignment);
+
+  // Bug 25167
+  verifyFormat("#if A\n"
+   "#else\n"
+   "int  = 12;\n"
+   "#endif\n"
+   "#if B\n"
+   "#else\n"
+   "int a = 12;\n"
+   "#endif\n",
+   Alignment);
+  verifyFormat("enum foo {\n"
+   "#if A\n"
+   "#else\n"
+   "   = 12;\n"
+   "#endif\n"
+   "#if B\n"
+   "#else\n"
+   "  a = 12;\n"
+   "#endif\n"
+   "};\n",
+   Alignment);
+
   EXPECT_EQ("int a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -377,9 +377,11 @@
 if (Changes[i].NewlinesBefore != 0) {
   CommasBeforeMatch = 0;
   EndOfSequence = i;
-  // If there is a blank line, or if the last line didn't contain any
-  // matching token, the sequence ends here.
-  if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine)
+  // If there is a blank line, there is a forced-align-break (eg,
+  // preprocessor), or if the last line didn't contain any matching token,
+  // the sequence ends here.
+  if (Changes[i].NewlinesBefore > 1 ||
+  Changes[i].Tok->MustBreakAlignBefore || !FoundMatchOnLine)
 AlignCurrentSequence();
 
   FoundMatchOnLine = false;
@@ -618,6 +620,8 @@
 if (Changes[i].StartOfBlockComment)
   continue;
 Newlines += Changes[i].NewlinesBefore;
+if (Changes[i].Tok->MustBreakAlignBefore)
+  BreakBeforeNext = true;
 if (!Changes[i].IsTrailingComment)
   continue;
 
Index: clang/lib/Format/UnwrappedLineParser.