[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Any thoughts on this?


https://reviews.llvm.org/D37904



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

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

[Marek Kurdej via Phabricator via cfe-commits]

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

Thanks for the changes. LGTM.


https://reviews.llvm.org/D35743



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

[PATCH] D38357: [Support/Regex] Handle tabulators and escaped chars in square brackets.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.

This patch is motivated by bug #34739 
(https://bugs.llvm.org/show_bug.cgi?id=34739).
Clang-format misbehaves because escape sequence '\t' in regex is treated as 
literal 't'.
This patch handles escaped characters inside square brackets as well (so that 
somebody can write `R("^[\[\]\t]")` as a regex pattern).


https://reviews.llvm.org/D38357

Files:
  lib/Support/regcomp.c
  unittests/Support/RegexTest.cpp

Index: unittests/Support/RegexTest.cpp
===
--- unittests/Support/RegexTest.cpp
+++ unittests/Support/RegexTest.cpp
@@ -61,6 +61,76 @@
   EXPECT_TRUE(r5.match(String));
 }
 
+TEST_F(RegexTest, Tabulators) {
+  Regex r1("^\\t+$");
+  EXPECT_TRUE(r1.match("\t"));
+  EXPECT_TRUE(r1.match("\t\t\t"));
+  EXPECT_FALSE(r1.match(""));
+  EXPECT_FALSE(r1.match(" "));
+  EXPECT_FALSE(r1.match(" \t "));
+
+  Regex r2("^(\\t| )+$");
+  EXPECT_TRUE(r2.match("\t"));
+  EXPECT_TRUE(r2.match("\t\t\t"));
+  EXPECT_FALSE(r2.match(""));
+  EXPECT_TRUE(r2.match(" "));
+  EXPECT_TRUE(r2.match(" \t "));
+
+  Regex r3("^[\\t ]+$");
+  EXPECT_TRUE(r3.match("\t"));
+  EXPECT_TRUE(r3.match("\t\t\t"));
+  EXPECT_FALSE(r3.match(""));
+  EXPECT_TRUE(r3.match(" "));
+  EXPECT_TRUE(r3.match(" \t "));
+}
+
+TEST_F(RegexTest, EscapedBackslash) {
+  Regex r1("^+$");
+  EXPECT_TRUE(r1.match("\\"));
+  EXPECT_TRUE(r1.match("\\"));
+  EXPECT_FALSE(r1.match(""));
+  EXPECT_FALSE(r1.match(" "));
+  EXPECT_FALSE(r1.match(" \\ "));
+
+  Regex r2("^(| )+$");
+  EXPECT_TRUE(r2.match("\\"));
+  EXPECT_TRUE(r2.match("\\"));
+  EXPECT_FALSE(r2.match(""));
+  EXPECT_TRUE(r2.match(" "));
+  EXPECT_TRUE(r2.match(" \\ "));
+
+  Regex r3("^[ ]+$");
+  EXPECT_TRUE(r3.match("\\"));
+  EXPECT_TRUE(r3.match("\\"));
+  EXPECT_FALSE(r3.match(""));
+  EXPECT_TRUE(r3.match(" "));
+  EXPECT_TRUE(r3.match(" \\ "));
+}
+
+TEST_F(RegexTest, EscapedOrdinaryCharacters) {
+  Regex r1("^\\X+$");
+  EXPECT_TRUE(r1.match("X"));
+  EXPECT_TRUE(r1.match("XXX"));
+  EXPECT_FALSE(r1.match(""));
+  EXPECT_FALSE(r1.match(" "));
+  EXPECT_FALSE(r1.match(" X "));
+
+  Regex r2("^(\\X| )+$");
+  EXPECT_TRUE(r2.match("X"));
+  EXPECT_TRUE(r2.match("XXX"));
+  EXPECT_FALSE(r2.match(""));
+  EXPECT_TRUE(r2.match(" "));
+  EXPECT_TRUE(r2.match(" X "));
+
+  Regex r3("^[\\X ]+$");
+  EXPECT_TRUE(r3.match("X"));
+  EXPECT_TRUE(r3.match("XXX"));
+  EXPECT_FALSE(r3.match(""));
+  EXPECT_TRUE(r3.match(" "));
+  EXPECT_TRUE(r3.match(" X "));
+}
+
+
 TEST_F(RegexTest, Backreferences) {
   Regex r1("([a-z]+)_\\1");
   SmallVector Matches;
@@ -100,7 +170,7 @@
 
   EXPECT_EQ("aber", Regex("[0-9]+").sub("\\", "a1234ber", &Error));
   EXPECT_EQ(Error, "replacement string contained trailing backslash");
-  
+
   // Backreferences
   EXPECT_EQ("aa1234bber", Regex("a[0-9]+b").sub("a\\0b", "a1234ber", &Error));
   EXPECT_EQ("", Error);
Index: lib/Support/regcomp.c
===
--- lib/Support/regcomp.c
+++ lib/Support/regcomp.c
@@ -403,10 +403,17 @@
 			EMIT(O_BACK, backrefnum);
 			p->g->backrefs = 1;
 		} else {
-			/* Other chars are simply themselves when escaped with a backslash.
-			 */
-			ordinary(p, c);
-		}
+switch (c) {
+case 't':
+  ordinary(p, '\t');
+  break;
+default:
+  /* Other chars are simply themselves when escaped with a
+   * backslash. */
+  ordinary(p, c);
+  break;
+}
+}
 		break;
 	case '{':		/* okay as ordinary except if digit follows */
 		REQUIRE(!MORE() || !isdigit((uch)PEEK()), REG_BADRPT);
@@ -787,7 +794,19 @@
 	default:		/* symbol, ordinary character, or range */
 /* xxx revision needed for multichar stuff */
 		start = p_b_symbol(p);
-		if (SEE('-') && MORE2() && PEEK2() != ']') {
+if ((start == '\\')) {
+  /* escape */
+  REQUIRE(MORE(), REG_EESCAPE);
+  c = GETNEXT();
+  switch (c) {
+  case 't':
+start = finish = '\t';
+break;
+  default:
+start = finish = c;
+break;
+  }
+		} else if (SEE('-') && MORE2() && PEEK2() != ']') {
 			/* range */
 			NEXT();
 			if (EAT('-'))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
curdeius added reviewers: alexfh, aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun.

It fixes the false positive when using constexpr if and where else cannot be 
removed:

Example:

  if constexpr (sizeof(int) > 4)
// ...
return /* ... */;
  else // This else cannot be removed.
// ...
return /* ... */;


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372

Files:
  clang-tidy/readability/ElseAfterReturnCheck.cpp
  test/clang-tidy/readability-else-after-return-if-constexpr.cpp


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+  if (sizeof(int) > 4)
+return;
+  else
+return;
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else
+return;
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else if constexpr (sizeof(long) > 4)
+return;
+  else
+return;
+}
+// CHECK-NOT: warning:
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
  expr(ignoringImplicit(cxxThrowExpr().bind("throw");
   Finder->addMatcher(
   compoundStmt(forEach(
-  ifStmt(hasThen(stmt(
+  ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
  anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
  hasElse(stmt().bind("else")))


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+  if (sizeof(int) > 4)
+return;
+  else
+return;
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else
+return;
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else if constexpr (sizeof(long) > 4)
+return;
+  else
+return;
+}
+// CHECK-NOT: warning:
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
  expr(ignoringImplicit(cxxThrowExpr().bind("throw");
   Finder->addMatcher(
   compoundStmt(forEach(
-  ifStmt(hasThen(stmt(
+  ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
  anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
  hasElse(stmt().bind("else")))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

In https://reviews.llvm.org/D53372#1267728, @lebedev.ri wrote:

> I think it would be good to add some more explanation as to *why* that `else` 
> has to be kept.


Do you mean add a comment in the code or just an explanation for the review?

For the latter, e.g.:

  // unrelated types:
  struct type_a {};
  struct type_b {};
  
  auto f()
  {
if constexpr(condition)
{
  return type_a{};
}
else
{
  return type_b{};
}
  }

In this case removing else will just provoke a compilation error. There may be 
some cases where you may remove else though.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372



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

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Actually, I can make it an option for this check to skip or not constexpr ifs, 
WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372



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

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 170101.
curdeius added a comment.

Applied changes as per comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372

Files:
  test/clang-tidy/readability-else-after-return-if-constexpr.cpp


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- test/clang-tidy/readability-else-after-return-if-constexpr.cpp
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -6,7 +6,7 @@
 return;
   else
 return;
-  // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return'
 
   if constexpr (sizeof(int) > 4)
 return;
@@ -20,4 +20,3 @@
   else
 return;
 }
-// CHECK-NOT: warning:


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- test/clang-tidy/readability-else-after-return-if-constexpr.cpp
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -6,7 +6,7 @@
 return;
   else
 return;
-  // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return'
 
   if constexpr (sizeof(int) > 4)
 return;
@@ -20,4 +20,3 @@
   else
 return;
 }
-// CHECK-NOT: warning:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 170152.
curdeius added a comment.

Fixed diff.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372

Files:
  clang-tidy/readability/ElseAfterReturnCheck.cpp
  test/clang-tidy/readability-else-after-return-if-constexpr.cpp


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+  if (sizeof(int) > 4)
+return;
+  else
+return;
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return'
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else
+return;
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else if constexpr (sizeof(long) > 4)
+return;
+  else
+return;
+}
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
  expr(ignoringImplicit(cxxThrowExpr().bind("throw");
   Finder->addMatcher(
   compoundStmt(forEach(
-  ifStmt(hasThen(stmt(
+  ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
  anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
  hasElse(stmt().bind("else")))


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+  if (sizeof(int) > 4)
+return;
+  else
+return;
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return'
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else
+return;
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else if constexpr (sizeof(long) > 4)
+return;
+  else
+return;
+}
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
  expr(ignoringImplicit(cxxThrowExpr().bind("throw");
   Finder->addMatcher(
   compoundStmt(forEach(
-  ifStmt(hasThen(stmt(
+  ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
  anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
  hasElse(stmt().bind("else")))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Just a few minor remarks and a possible workaround for testing `CHECK-FIXES: 
[[nodiscard]]`.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+ return doesFunctionNameStartWithUnderScore(&Node);

If think that you should run clang-format over your patch.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:118
+  // 2. a const member function which return a variable which is ignored
+  // performs some externation io operation and the return value could be 
ignore
+

Please fix typos/grammar, externation -> external, io -> I/O, ignore -> ignored.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Users can use :option:`ReplacementString` to specify a macro to use instead of
+``[[nodiscard]]``. This is useful when maintaining source code that needs to

I would avoid repeating the option name in its description and state clearly 
that `[[nodiscard]]` is the default.

Cf. other clang-tidy checks with options: 
http://clang.llvm.org/extra/clang-tidy/checks/list.html, e.g. 
http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html#options.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:39
+``[[nodiscard]]``. This is useful when maintaining source code that needs to
+also compile with a non c++17 conforming compiler.
+

I'd suggest "needs to compile with a pre-C++17 compiler."



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()

"turn off the adding"? I'm not a native English speaker, but "turn off addition 
of" would sound better.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()

curdeius wrote:
> "turn off the adding"? I'm not a native English speaker, but "turn off 
> addition of" would sound better.
Same as for the other option, I wouldn't repeat the option name and give the 
default value explicitly.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

curdeius wrote:
> Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
It might have been discussed already, but is it a common convention to mark 
internal functions with a leading underscore?
If that comes from system headers, AFAIK clang-tidy already is able not to emit 
warnings from them.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;

I think you can use a regex as a workaround, i.e. using `{{\[\[nodiscard\]\]}}`.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:66
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+

So this line should become: `// CHECK-FIXES: {{\[\[nodiscard\]\] bool f11() 
const;`


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Some more minor remarks. I'll give this check a try to see how it behaves on 
some of my projects.
I agree that a high rate of false positives is possible (and is a bit of 
spoiler) but I wouldn't reject this IMO useful check because of that.
Anyway, everything looks pretty clean for me.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+const Type *ParType = Par->getType().getTypePtr();

Wouldn't something along the lines:
```
const auto ¶meters = Node.parameters();
return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto *Par) {
  const Type *ParType = Par->getType().getTypePtr();

  if (isNonConstReferenceType(Par->getType())) {
return true;
  }
  if (ParType->isPointerType()) {
return true;
  }
  return false;
});
```
be a little bit more expressive?

I would also refactor it differently:
```
  const Type &ParType = Par->getType(); // not sure about Type

  if (isNonConstReferenceType(ParType)) {
  // ...
  if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() 
possible?
  // ...
```
but that's a matter of taste.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:12
+or non constant references, and non pointer arguments, and of which the
+member functions are themselve const, have not means of altering any state
+or passing values other than the return type, unless they are using mutable 

themselve -> themselves, have not means -> have no means



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:3
+// RUN:   -config="{CheckOptions: [{key: 
modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+

I'm not sure what guidelines dictate, but I'd love to see another test file 
with `-std=c++14` for instance (or whatever minimal version to necessary to use 
`clang::WarnUnusedResult`, if any).
A very small test would be ok, you can also move some parts of this file into a 
new one.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;

You might want to get rid of this `TODO` note now, same for the one on the top 
of the file.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

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



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+const Type *ParType = Par->getType().getTypePtr();

curdeius wrote:
> Wouldn't something along the lines:
> ```
> const auto ¶meters = Node.parameters();
> return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto 
> *Par) {
>   const Type *ParType = Par->getType().getTypePtr();
> 
>   if (isNonConstReferenceType(Par->getType())) {
> return true;
>   }
>   if (ParType->isPointerType()) {
> return true;
>   }
>   return false;
> });
> ```
> be a little bit more expressive?
> 
> I would also refactor it differently:
> ```
>   const Type &ParType = Par->getType(); // not sure about Type
> 
>   if (isNonConstReferenceType(ParType)) {
>   // ...
>   if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() 
> possible?
>   // ...
> ```
> but that's a matter of taste.
I've just seen that you can use `.param_begin()` and `.param_end()` instead of 
`parameters.begin()/end()`.
I'd also reduce the use of `auto` here, because it's pretty hard to read it IMO.
So, I'd suggest

```
return std::any_of(Node.param_begin(), Node.param_end(), [](const ParmVarDecl 
*Par) {
  const QualType &ParType = Par->getType();

  if (isNonConstReferenceType(ParType)) {
return true;
  }
  if (ParType.getTypePtr()->isPointerType()) {
return true;
  }
  return false;
});
```


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM. But I'm not a code owner here and I don't know if you need an acceptance 
of one of them.
Great job.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

A few more ideas for enhancements.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:41
+  //bool foo(A*)
+  // then they may not care about the return value,  because of passing data
+  // via the arguments however functions with no arguments will fall through

Double space after comma. Please check all comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:9
+
+Member functions which return non void types, and take in only pass by value or
+non constant references, and non pointer arguments, and of which the member

`non-void`, `pass-by-value`, `non-const`, `non-pointer` with hyphens.
You may simplify `, and of which ...` (if you have an idea how to do it) as 
it's a bit hard to read.

The phrase starting with `Unless` is actually not a full phrase.

I'd suggest something along the lines:

```
Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions in 
order to
highlight at compile time which return values should not be ignored.
Member functions need to satisfy the following conditions to be considered by 
this check:
  - no ``[[nodiscard]]``, ``[[noreturn]]``, 
``__attribute__((warn_unused_result))``, ``[[clang::warn_unused_result]]`` nor 
`[[gcc::warn_unused_result]]``  attribute,
  - non-void return type,
  - no non-const reference parameters,
  - no non-const pointer parameters,
...
```



Comment at: test/clang-tidy/modernize-use-nodiscard-cxx11.cpp:24
+
+};
+

I think that it would be cool to test that the check doesn't warn on 
`[[clang::warn_unused_result]]` nor `[[gcc::warn_unused_result]]`.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

JonasToth wrote:
> I think the template tests should be improved.
> What happens for `T empty()`, `typename T::value_type empty()` and so on. THe 
> same for the parameters for functions/methods (including function templates).
> 
> Thinking of it, it might be a good idea to ignore functions where the 
> heuristic depends on the template-paramters.
It might be a good idea to add a note in the documentation about handling of 
function templates and functions in class templates.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:65
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  // If we are using C++17 attributes we are going to need c++17
+  if (NoDiscardMacro == "[[nodiscard]]") {

I'd suggets: ` // If we use [[nodiscard]] attribute, we require at least C++17.`



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:73
+  // c++17 compilers.
+  if (!getLangOpts().CPlusPlus)
+return;

I'd move this if to the bottom of the function as it's the most general one and 
fix the comment above: e.g. `// Ignore non-C++ code.`.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:92
+  SourceLocation Loc = MatchedDecl->getLocation();
+  if (!Loc.isValid() || Loc.isMacroID())
+return;

You can simplify `!Loc.isValid()` to `Loc.isInvalid()`.


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

https://reviews.llvm.org/D55433



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

[PATCH] D37132: [clang-format] Add support for C++17 structured bindings.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 113073.
curdeius added a comment.

Fix line endings.


https://reviews.llvm.org/D37132

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -30,11 +30,7 @@
 
 class FormatTest : public ::testing::Test {
 protected:
-  enum StatusCheck {
-SC_ExpectComplete,
-SC_ExpectIncomplete,
-SC_DoNotCheck
-  };
+  enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete, SC_DoNotCheck };
 
   std::string format(llvm::StringRef Code,
  const FormatStyle &Style = getLLVMStyle(),
@@ -280,7 +276,8 @@
 format("namespace {\n"
"int i;\n"
"\n"
-   "}", LLVMWithNoNamespaceFix));
+   "}",
+   LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
 "int i;\n"
 "}",
@@ -961,20 +958,26 @@
"#endif\n"
"}",
Style));
-  EXPECT_EQ("switch (a) {\n" "case 0:\n"
-"  return; // long long long long long long long long long long long long comment\n"
-"  // line\n" "}",
+  EXPECT_EQ("switch (a) {\n"
+"case 0:\n"
+"  return; // long long long long long long long long long long "
+"long long comment\n"
+"  // line\n"
+"}",
 format("switch (a) {\n"
-   "case 0: return; // long long long long long long long long long long long long comment line\n"
+   "case 0: return; // long long long long long long long long "
+   "long long long long comment line\n"
"}",
Style));
   EXPECT_EQ("switch (a) {\n"
 "case 0:\n"
-"  return; /* long long long long long long long long long long long long comment\n"
+"  return; /* long long long long long long long long long long "
+"long long comment\n"
 " line */\n"
 "}",
 format("switch (a) {\n"
-   "case 0: return; /* long long long long long long long long long long long long comment line */\n"
+   "case 0: return; /* long long long long long long long long "
+   "long long long long comment line */\n"
"}",
Style));
   verifyFormat("switch (a) {\n"
@@ -1395,8 +1398,7 @@
   // This code is more common than we thought; if we
   // layout this correctly the semicolon will go into
   // its own line, which is undesirable.
-  verifyFormat("namespace {};",
-   LLVMWithNoNamespaceFix);
+  verifyFormat("namespace {};", LLVMWithNoNamespaceFix);
   verifyFormat("namespace {\n"
"class A {};\n"
"};",
@@ -1465,8 +1467,8 @@
   Style.CompactNamespaces = true;
 
   verifyFormat("namespace A { namespace B {\n"
-			   "}} // namespace A::B",
-			   Style);
+   "}} // namespace A::B",
+   Style);
 
   EXPECT_EQ("namespace out { namespace in {\n"
 "}} // namespace out::in",
@@ -2332,7 +2334,8 @@
"\\\n"
"  public: \\\n"
"void baz(); \\\n"
-   "  };", DontAlign));
+   "  };",
+   DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
@@ -2614,16 +2617,19 @@
   Style.MacroBlockEnd = "^[A-Z_]+_END$";
   verifyFormat("FOO_BEGIN\n"
"  FOO_ENTRY\n"
-   "FOO_END", Style);
+   "FOO_END",
+   Style);
   verifyFormat("FOO_BEGIN\n"
"  NESTED_FOO_BEGIN\n"
"NESTED_FOO_ENTRY\n"
"  NESTED_FOO_END\n"
-   "FOO_END", Style);
+   "FOO_END",
+   Style);
   verifyFormat("FOO_BEGIN(Foo, Bar)\n"
"  int x;\n"
"  x = 1;\n"
-   "FOO_END(Baz)", Style);
+   "FOO_END(Baz)",
+   Style);
 }
 
 //===--===//
@@ -3101,22 +3107,22 @@
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), aaa() {}",
-	  Style);
+  Style);
 
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), a(aa),\n"
   "a(aa) {}",
-	  Style);
+  Style);
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "aa(aa),\n"
   "aaa() {}",
-	  Style);
+  Style);
   verifyFormat("Constructor(a

[PATCH] D37132: [clang-format] Add support for C++17 structured bindings.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 113077.
curdeius added a comment.

Fix line endings again.


https://reviews.llvm.org/D37132

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11217,6 +11217,31 @@
   EXPECT_EQ("auto c = u8'a';", format("auto c = u8'a';"));
 }
 
+TEST_F(FormatTest, StructuredBindings) {
+  // Structured bindings is a C++17 feature.
+  // all modes, including C++11, C++14 and C++17
+  verifyFormat("auto [a, b] = f();");
+  EXPECT_EQ("auto [a, b] = f();", format("auto[a, b] = f();"));
+  EXPECT_EQ("const auto [a, b] = f();", format("const   auto[a, b] = f();"));
+  EXPECT_EQ("auto const [a, b] = f();", format("auto  const[a, b] = f();"));
+  EXPECT_EQ("auto const volatile [a, b] = f();",
+format("auto  const   volatile[a, b] = f();"));
+  EXPECT_EQ("auto [a, b, c] = f();", format("auto   [  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto & [a, b, c] = f();",
+format("auto   &[  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto && [a, b, c] = f();",
+format("auto   &&[  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto const & [a, b] = f();", format("auto  const&[a, b] = f();"));
+  EXPECT_EQ("auto const volatile && [a, b] = f();",
+format("auto  const  volatile  &&[a, b] = f();"));
+  EXPECT_EQ("auto && [a, b] = f();", format("auto  &&[a, b] = f();"));
+
+  format::FormatStyle Spaces = format::getLLVMStyle();
+  Spaces.SpacesInSquareBrackets = true;
+  verifyFormat("auto [ a, b ] = f();", Spaces);
+  verifyFormat("auto && [ a, b ] = f();", Spaces);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -47,7 +47,7 @@
 if (NonTemplateLess.count(CurrentToken->Previous))
   return false;
 
-const FormatToken& Previous = *CurrentToken->Previous;
+const FormatToken &Previous = *CurrentToken->Previous;
 if (Previous.Previous) {
   if (Previous.Previous->Tok.isLiteral())
 return false;
@@ -152,11 +152,11 @@
   // export type X = (...);
   Contexts.back().IsExpression = false;
 } else if (Left->Previous &&
-(Left->Previous->isOneOf(tok::kw_static_assert, tok::kw_decltype,
- tok::kw_if, tok::kw_while, tok::l_paren,
- tok::comma) ||
- Left->Previous->endsSequence(tok::kw_constexpr, tok::kw_if) ||
- Left->Previous->is(TT_BinaryOperator))) {
+   (Left->Previous->isOneOf(tok::kw_static_assert, tok::kw_decltype,
+tok::kw_if, tok::kw_while, tok::l_paren,
+tok::comma) ||
+Left->Previous->endsSequence(tok::kw_constexpr, tok::kw_if) ||
+Left->Previous->is(TT_BinaryOperator))) {
   // static_assert, if and while usually contain expressions.
   Contexts.back().IsExpression = true;
 } else if (Style.Language == FormatStyle::LK_JavaScript && Left->Previous &&
@@ -325,8 +325,7 @@
 // In C++, this can happen either in array of templates (foo[10])
 // or when array is a nested template type (unique_ptr[]>).
 bool CppArrayTemplates =
-Style.isCpp() && Parent &&
-Parent->is(TT_TemplateCloser) &&
+Style.isCpp() && Parent && Parent->is(TT_TemplateCloser) &&
 (Contexts.back().CanBeExpression || Contexts.back().IsExpression ||
  Contexts.back().InTemplateArgument);
 
@@ -342,9 +341,22 @@
  getBinOpPrecedence(Parent->Tok.getKind(), true, true) > prec::Unknown);
 bool ColonFound = false;
 
+FormatToken *PreviousNoneOfConstVolatileReference = Parent;
+while (PreviousNoneOfConstVolatileReference &&
+   PreviousNoneOfConstVolatileReference->isOneOf(
+   tok::kw_const, tok::kw_volatile, tok::amp, tok::ampamp))
+  PreviousNoneOfConstVolatileReference =
+  PreviousNoneOfConstVolatileReference->getPreviousNonComment();
+
+bool CppStructuredBindings =
+Style.isCpp() && PreviousNoneOfConstVolatileReference &&
+PreviousNoneOfConstVolatileReference->is(tok::kw_auto);
+
 unsigned BindingIncrease = 1;
 if (Left->is(TT_Unknown)) {
-  if (StartsObjCMethodExpr) {
+  if (CppStructuredBindings) {
+Left->Type = TT_StructuredBindingLSquare;
+  } else if (StartsObjCMethodExpr) {
 Left->Type = TT_ObjCMethodExpr;
   } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
  Contexts.back().ContextKind == tok::l_brace &&
@@ -605,7 +617,8 @@
   break;
 case tok::kw_if:
 case tok::kw_while:
-  if (Tok->is(tok::kw_if) && 

[PATCH] D37132: [clang-format] Add support for C++17 structured bindings.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 113079.
curdeius added a comment.

Extract method.


https://reviews.llvm.org/D37132

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11217,6 +11217,31 @@
   EXPECT_EQ("auto c = u8'a';", format("auto c = u8'a';"));
 }
 
+TEST_F(FormatTest, StructuredBindings) {
+  // Structured bindings is a C++17 feature.
+  // all modes, including C++11, C++14 and C++17
+  verifyFormat("auto [a, b] = f();");
+  EXPECT_EQ("auto [a, b] = f();", format("auto[a, b] = f();"));
+  EXPECT_EQ("const auto [a, b] = f();", format("const   auto[a, b] = f();"));
+  EXPECT_EQ("auto const [a, b] = f();", format("auto  const[a, b] = f();"));
+  EXPECT_EQ("auto const volatile [a, b] = f();",
+format("auto  const   volatile[a, b] = f();"));
+  EXPECT_EQ("auto [a, b, c] = f();", format("auto   [  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto & [a, b, c] = f();",
+format("auto   &[  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto && [a, b, c] = f();",
+format("auto   &&[  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto const & [a, b] = f();", format("auto  const&[a, b] = f();"));
+  EXPECT_EQ("auto const volatile && [a, b] = f();",
+format("auto  const  volatile  &&[a, b] = f();"));
+  EXPECT_EQ("auto && [a, b] = f();", format("auto  &&[a, b] = f();"));
+
+  format::FormatStyle Spaces = format::getLLVMStyle();
+  Spaces.SpacesInSquareBrackets = true;
+  verifyFormat("auto [ a, b ] = f();", Spaces);
+  verifyFormat("auto && [ a, b ] = f();", Spaces);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -310,6 +310,16 @@
 return false;
   }
 
+  bool isCppStructuredBinding(const FormatToken *Tok) {
+if (!Style.isCpp() || !Tok->is(tok::l_square))
+  return false;
+while (Tok && Tok->isOneOf(tok::kw_const, tok::kw_volatile, tok::amp,
+   tok::ampamp)) {
+  Tok = Tok->getPreviousNonComment();
+}
+return Tok && Tok->is(tok::kw_auto);
+  }
+
   bool parseSquare() {
 if (!CurrentToken)
   return false;
@@ -344,7 +354,9 @@
 
 unsigned BindingIncrease = 1;
 if (Left->is(TT_Unknown)) {
-  if (StartsObjCMethodExpr) {
+  if (isCppStructuredBinding(Left)) {
+Left->Type = TT_StructuredBindingLSquare;
+  } else if (StartsObjCMethodExpr) {
 Left->Type = TT_ObjCMethodExpr;
   } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
  Contexts.back().ContextKind == tok::l_brace &&
@@ -2256,17 +2268,20 @@
   if (Left.is(tok::l_square))
 return (Left.is(TT_ArrayInitializerLSquare) &&
 Style.SpacesInContainerLiterals && Right.isNot(tok::r_square)) ||
-   (Left.is(TT_ArraySubscriptLSquare) && Style.SpacesInSquareBrackets &&
-Right.isNot(tok::r_square));
+   (Left.isOneOf(TT_ArraySubscriptLSquare,
+ TT_StructuredBindingLSquare) &&
+Style.SpacesInSquareBrackets && Right.isNot(tok::r_square));
   if (Right.is(tok::r_square))
 return Right.MatchingParen &&
((Style.SpacesInContainerLiterals &&
  Right.MatchingParen->is(TT_ArrayInitializerLSquare)) ||
 (Style.SpacesInSquareBrackets &&
- Right.MatchingParen->is(TT_ArraySubscriptLSquare)));
+ Right.MatchingParen->isOneOf(TT_ArraySubscriptLSquare,
+  TT_StructuredBindingLSquare)));
   if (Right.is(tok::l_square) &&
   !Right.isOneOf(TT_ObjCMethodExpr, TT_LambdaLSquare,
- TT_DesignatedInitializerLSquare) &&
+ TT_DesignatedInitializerLSquare,
+ TT_StructuredBindingLSquare) &&
   !Left.isOneOf(tok::numeric_constant, TT_DictLiteral))
 return false;
   if (Left.is(tok::l_brace) && Right.is(tok::r_brace))
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -84,6 +84,7 @@
   TYPE(RegexLiteral) \
   TYPE(SelectorName) \
   TYPE(StartOfName) \
+  TYPE(StructuredBindingLSquare) \
   TYPE(TemplateCloser) \
   TYPE(TemplateOpener) \
   TYPE(TemplateString) \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37132: [clang-format] Add support for C++17 structured bindings.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 113084.
curdeius added a comment.

Fix: use do-while loop.


https://reviews.llvm.org/D37132

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11217,6 +11217,31 @@
   EXPECT_EQ("auto c = u8'a';", format("auto c = u8'a';"));
 }
 
+TEST_F(FormatTest, StructuredBindings) {
+  // Structured bindings is a C++17 feature.
+  // all modes, including C++11, C++14 and C++17
+  verifyFormat("auto [a, b] = f();");
+  EXPECT_EQ("auto [a, b] = f();", format("auto[a, b] = f();"));
+  EXPECT_EQ("const auto [a, b] = f();", format("const   auto[a, b] = f();"));
+  EXPECT_EQ("auto const [a, b] = f();", format("auto  const[a, b] = f();"));
+  EXPECT_EQ("auto const volatile [a, b] = f();",
+format("auto  const   volatile[a, b] = f();"));
+  EXPECT_EQ("auto [a, b, c] = f();", format("auto   [  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto & [a, b, c] = f();",
+format("auto   &[  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto && [a, b, c] = f();",
+format("auto   &&[  a  ,  b,c   ] = f();"));
+  EXPECT_EQ("auto const & [a, b] = f();", format("auto  const&[a, b] = f();"));
+  EXPECT_EQ("auto const volatile && [a, b] = f();",
+format("auto  const  volatile  &&[a, b] = f();"));
+  EXPECT_EQ("auto && [a, b] = f();", format("auto  &&[a, b] = f();"));
+
+  format::FormatStyle Spaces = format::getLLVMStyle();
+  Spaces.SpacesInSquareBrackets = true;
+  verifyFormat("auto [ a, b ] = f();", Spaces);
+  verifyFormat("auto && [ a, b ] = f();", Spaces);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -310,6 +310,16 @@
 return false;
   }
 
+  bool isCppStructuredBinding(const FormatToken *Tok) {
+if (!Style.isCpp() || !Tok->is(tok::l_square))
+  return false;
+do {
+  Tok = Tok->getPreviousNonComment();
+} while (Tok && Tok->isOneOf(tok::kw_const, tok::kw_volatile, tok::amp,
+ tok::ampamp));
+return Tok && Tok->is(tok::kw_auto);
+  }
+
   bool parseSquare() {
 if (!CurrentToken)
   return false;
@@ -344,7 +354,9 @@
 
 unsigned BindingIncrease = 1;
 if (Left->is(TT_Unknown)) {
-  if (StartsObjCMethodExpr) {
+  if (isCppStructuredBinding(Left)) {
+Left->Type = TT_StructuredBindingLSquare;
+  } else if (StartsObjCMethodExpr) {
 Left->Type = TT_ObjCMethodExpr;
   } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
  Contexts.back().ContextKind == tok::l_brace &&
@@ -2256,17 +2268,20 @@
   if (Left.is(tok::l_square))
 return (Left.is(TT_ArrayInitializerLSquare) &&
 Style.SpacesInContainerLiterals && Right.isNot(tok::r_square)) ||
-   (Left.is(TT_ArraySubscriptLSquare) && Style.SpacesInSquareBrackets &&
-Right.isNot(tok::r_square));
+   (Left.isOneOf(TT_ArraySubscriptLSquare,
+ TT_StructuredBindingLSquare) &&
+Style.SpacesInSquareBrackets && Right.isNot(tok::r_square));
   if (Right.is(tok::r_square))
 return Right.MatchingParen &&
((Style.SpacesInContainerLiterals &&
  Right.MatchingParen->is(TT_ArrayInitializerLSquare)) ||
 (Style.SpacesInSquareBrackets &&
- Right.MatchingParen->is(TT_ArraySubscriptLSquare)));
+ Right.MatchingParen->isOneOf(TT_ArraySubscriptLSquare,
+  TT_StructuredBindingLSquare)));
   if (Right.is(tok::l_square) &&
   !Right.isOneOf(TT_ObjCMethodExpr, TT_LambdaLSquare,
- TT_DesignatedInitializerLSquare) &&
+ TT_DesignatedInitializerLSquare,
+ TT_StructuredBindingLSquare) &&
   !Left.isOneOf(tok::numeric_constant, TT_DictLiteral))
 return false;
   if (Left.is(tok::l_brace) && Right.is(tok::r_brace))
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -84,6 +84,7 @@
   TYPE(RegexLiteral) \
   TYPE(SelectorName) \
   TYPE(StartOfName) \
+  TYPE(StructuredBindingLSquare) \
   TYPE(TemplateCloser) \
   TYPE(TemplateOpener) \
   TYPE(TemplateString) \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31334: [clang-format] Add options for indenting preprocessor directives

[Marek Kurdej via Phabricator via cfe-commits]

curdeius abandoned this revision.
curdeius added a comment.

Superseded by https://reviews.llvm.org/rL312125.


https://reviews.llvm.org/D31334



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

[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
Herald added a subscriber: klimek.

NamespaceEndCommentsFixer did not fix namespace comments when the brace opening 
the namespace was not on the same line as the "namespace" keyword.
It occurs in Allman, GNU and Linux styles and whenever 
BraceWrapping.AfterNamespace is true.

Before:

  namespace a
  {
  void f();
  void g();
  }

After:

  namespace a
  {
  void f();
  void g();
  } // namespace a


https://reviews.llvm.org/D37904

Files:
  lib/Format/NamespaceEndCommentsFixer.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1413,7 +1413,7 @@
   verifyFormat("typedef enum {} EmptyEnum;");
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum {\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -1425,7 +1425,7 @@
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum\n"
"{\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -9323,7 +9323,7 @@
"struct B {\n"
"  int x;\n"
"};\n"
-   "}\n",
+   "} // namespace a\n",
LinuxBraceStyle);
   verifyFormat("enum X {\n"
"  Y = 0,\n"
@@ -9453,6 +9453,19 @@
 TEST_F(FormatTest, AllmanBraceBreaking) {
   FormatStyle AllmanBraceStyle = getLLVMStyle();
   AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
+
+  EXPECT_EQ("namespace a\n"
+"{\n"
+"void f();\n"
+"void g();\n"
+"} // namespace a\n",
+format("namespace a\n"
+   "{\n"
+   "void f();\n"
+   "void g();\n"
+   "}\n",
+   AllmanBraceStyle));
+
   verifyFormat("namespace a\n"
"{\n"
"class A\n"
@@ -9471,7 +9484,7 @@
"{\n"
"  int x;\n"
"};\n"
-   "}",
+   "} // namespace a",
AllmanBraceStyle);
 
   verifyFormat("void f()\n"
@@ -9677,7 +9690,7 @@
"  }\n"
"  void g() { return; }\n"
"}\n"
-   "}",
+   "} // namespace a",
GNUBraceStyle);
 
   verifyFormat("void f()\n"
Index: lib/Format/NamespaceEndCommentsFixer.cpp
===
--- lib/Format/NamespaceEndCommentsFixer.cpp
+++ lib/Format/NamespaceEndCommentsFixer.cpp
@@ -118,6 +118,12 @@
 return nullptr;
   assert(StartLineIndex < AnnotatedLines.size());
   const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First;
+  if (NamespaceTok->is(tok::l_brace)) {
+// "namespace" keyword can be on the line preceding '{', e.g. in styles
+// where BraceWrapping.AfterNamespace is true.
+if (StartLineIndex > 0)
+  NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
+  }
   // Detect "(inline)? namespace" in the beginning of a line.
   if (NamespaceTok->is(tok::kw_inline))
 NamespaceTok = NamespaceTok->getNextNonComment();


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1413,7 +1413,7 @@
   verifyFormat("typedef enum {} EmptyEnum;");
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum {\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -1425,7 +1425,7 @@
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum\n"
"{\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -9323,7 +9323,7 @@
"struct B {\n"
"  int x;\n"
"};\n"
-   "}\n",
+   "} // namespace a\n",
LinuxBraceStyle);
   verifyFormat("enum X {\n"
"  Y = 0,\n"
@@ -9453,6 +9453,19 @@
 TEST_F(FormatTest, AllmanBraceBreaking) {
   FormatStyle AllmanBraceStyle = getLLVMStyle();
   AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
+
+  EXPECT_EQ("namespace a\n"
+"{\n"
+"void f();\n"
+"void g();\n"
+"} // namespace a\n",
+format("namespace a\n"
+   "{\n"
+   "void f();\n"
+   "void g(

[PATCH] D37980: [clang-format] Better parsing of lambda captures with initializer expressions.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.

This fixes bug #19986 (https://bugs.llvm.org/show_bug.cgi?id=19986).
The code was incorrectly formatted to (mind additional spaces inside brackets) 
when lambda capture contained an initializer expression.
This patch does not handle all possible initializers, but the most common once.
To handle these resting cases, we'd have to parse the whole initializer 
expressions or handle lambda detection differently.

Before:

  int i = 100;
  auto f1 = [ i, value = 23 ]() { return i + value; };
  auto f2 = [ i, value{23} ]() { return i + value; };

After:

  int i = 100;
  auto f1 = [i, value = 23]() { return i + value; };
  auto f2 = [i, value{23}]() { return i + value; };


https://reviews.llvm.org/D37980

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1413,7 +1413,7 @@
   verifyFormat("typedef enum {} EmptyEnum;");
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum {\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -1425,7 +1425,7 @@
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum\n"
"{\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -1574,8 +1574,8 @@
   Style.CompactNamespaces = true;
 
   verifyFormat("namespace A { namespace B {\n"
-			   "}} // namespace A::B",
-			   Style);
+   "}} // namespace A::B",
+   Style);
 
   EXPECT_EQ("namespace out { namespace in {\n"
 "}} // namespace out::in",
@@ -3428,22 +3428,22 @@
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), aaa() {}",
-	  Style);
+  Style);
 
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), a(aa),\n"
   "a(aa) {}",
-	  Style);
+  Style);
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "aa(aa),\n"
   "aaa() {}",
-	  Style);
+  Style);
   verifyFormat("Constructor(aa ,\n"
"aa ) :\n"
"aa(aa) {}",
-			   Style);
+   Style);
 
   verifyFormat("Constructor() :\n"
"(aaa),\n"
@@ -3450,17 +3450,17 @@
"(aaa,\n"
" aaa),\n"
"aaa() {}",
-			   Style);
+   Style);
 
   verifyFormat("Constructor() :\n"
"(\n"
"a) {}",
-			   Style);
+   Style);
 
   verifyFormat("Constructor(int Parameter = 0) :\n"
"aa(a),\n"
"(a) {}",
-			   Style);
+   Style);
   verifyFormat("Constructor() :\n"
"aa(a), (b) {\n"
"}",
@@ -3468,7 +3468,7 @@
   verifyFormat("Constructor() :\n"
"a(\n"
"a(, )) {}",
-			   Style);
+   Style);
 
   // Here a line could be saved by splitting the second initializer onto two
   // lines, but that is not desirable.
@@ -3476,7 +3476,7 @@
"(),\n"
"aaa(aaa),\n"
"at() {}",
-			   Style);
+   Style);
 
   FormatStyle OnePerLine = Style;
   OnePerLine.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
@@ -3526,7 +3526,7 @@
 format("Constructor() :\n"
"// Comment forcing unwanted break.\n"
"() {}",
-   Style));
+   Style));
 
   Style.ColumnLimit = 0;
   verifyFormat("SomeClass::Constructor() :\n"
@@ -3536,7 +3536,7 @@
"a(a) {}",
Style);
   verifyFormat("SomeClass::Constructor() :\n"
-			   "a(a), b(b), c(c) {}",
+   "a(a), b(b), c

[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

In https://reviews.llvm.org/D37904#873860, @krasimir wrote:

> Could you please move the test that adds a namespace end comment to 
> `unittests/Format/NamespaceEndCommentsFixerTest.cpp`?


That's what I wanted to do initially, but the very same tests there passed 
surprisingly. I think that there are some differences related to "messing up" 
the code between both test suites.
I'll have another look at it however.


https://reviews.llvm.org/D37904



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

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

There is one big missing thing here: `PointerAlignment`. Actually only 
`PAS_Left` is taken into account.
There are 3 possible options:
Left: `auto& [a, b] = f();`
Middle: `auto & [a, b] = f();`
Right: `auto &[a, b] = f();`


https://reviews.llvm.org/D35743



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

[PATCH] D37980: [clang-format] Better parsing of lambda captures with initializer expressions.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 115814.
curdeius added a comment.

Minor: use `FormatToken::isNot` instead of `!FormatToken::is`.


https://reviews.llvm.org/D37980

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1413,7 +1413,7 @@
   verifyFormat("typedef enum {} EmptyEnum;");
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum {\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -1425,7 +1425,7 @@
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum\n"
"{\n"
-   "  ZERO = 0,\n" 
+   "  ZERO = 0,\n"
"  ONE = 1,\n"
"  TWO = 2,\n"
"  THREE = 3\n"
@@ -1574,8 +1574,8 @@
   Style.CompactNamespaces = true;
 
   verifyFormat("namespace A { namespace B {\n"
-			   "}} // namespace A::B",
-			   Style);
+   "}} // namespace A::B",
+   Style);
 
   EXPECT_EQ("namespace out { namespace in {\n"
 "}} // namespace out::in",
@@ -3428,22 +3428,22 @@
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), aaa() {}",
-	  Style);
+  Style);
 
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), a(aa),\n"
   "a(aa) {}",
-	  Style);
+  Style);
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "aa(aa),\n"
   "aaa() {}",
-	  Style);
+  Style);
   verifyFormat("Constructor(aa ,\n"
"aa ) :\n"
"aa(aa) {}",
-			   Style);
+   Style);
 
   verifyFormat("Constructor() :\n"
"(aaa),\n"
@@ -3450,17 +3450,17 @@
"(aaa,\n"
" aaa),\n"
"aaa() {}",
-			   Style);
+   Style);
 
   verifyFormat("Constructor() :\n"
"(\n"
"a) {}",
-			   Style);
+   Style);
 
   verifyFormat("Constructor(int Parameter = 0) :\n"
"aa(a),\n"
"(a) {}",
-			   Style);
+   Style);
   verifyFormat("Constructor() :\n"
"aa(a), (b) {\n"
"}",
@@ -3468,7 +3468,7 @@
   verifyFormat("Constructor() :\n"
"a(\n"
"a(, )) {}",
-			   Style);
+   Style);
 
   // Here a line could be saved by splitting the second initializer onto two
   // lines, but that is not desirable.
@@ -3476,7 +3476,7 @@
"(),\n"
"aaa(aaa),\n"
"at() {}",
-			   Style);
+   Style);
 
   FormatStyle OnePerLine = Style;
   OnePerLine.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
@@ -3526,7 +3526,7 @@
 format("Constructor() :\n"
"// Comment forcing unwanted break.\n"
"() {}",
-   Style));
+   Style));
 
   Style.ColumnLimit = 0;
   verifyFormat("SomeClass::Constructor() :\n"
@@ -3536,7 +3536,7 @@
"a(a) {}",
Style);
   verifyFormat("SomeClass::Constructor() :\n"
-			   "a(a), b(b), c(c) {}",
+   "a(a), b(b), c(c) {}",
Style);
   verifyFormat("SomeClass::Constructor() :\n"
"a(a) {\n"
@@ -3547,12 +3547,12 @@
 
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   verifyFormat("SomeClass::Constructor() :\n"
-			   "a(a), b(b), c(c) {\n"
-			   "}",
+   "a(a), b(b), c(c) {\n"
+   "}",
Style);
   verifyFormat("SomeClass::Constructor() :\n"
"a(a) {\n"
-			   "}",
+   "}",
Style);
 
   Style.ColumnLimit = 80;
@@ -6519,7 +6519,7 @@

[PATCH] D37980: [clang-format] Better parsing of lambda captures with initializer expressions.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius abandoned this revision.
curdeius added a comment.

Ok. Nice patch. You can close https://bugs.llvm.org/show_bug.cgi?id=19986 now.


https://reviews.llvm.org/D37980



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

[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I confirm what I observed before: when invoking tests in 
`unittests/Format/NamespaceEndCommentsFixerTest.cpp`, the `const AnnotatedLine 
*line` parameter in `getNamespaceToken` gets one big line that includes both 
`namespace` and `{` (something like `namespace\n{\n...`, whereas in tests 
invoked from `unittests/Format/FormatTests.cpp`, there is a separate line with 
`namespace\n` and another one with `{\n`.

I haven't looked at what provokes this behavior, but I imagine that there are 
additional passes in `FormatTests`.


https://reviews.llvm.org/D37904



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

[PATCH] D13811: [clang-format] AllowShortFunctionsOnASingleLine: true/Empty didn't work with BreakBeforeBraces: Linux/Allman.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius abandoned this revision.
curdeius added a comment.

This was fixed by https://reviews.llvm.org/rL312904 and other commits.


https://reviews.llvm.org/D13811



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

[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

That's precisely what I've written, but, as I'd said before, such tests pass 
already without any modification in `NamespaceEndCommentsFixer`.


https://reviews.llvm.org/D37904



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

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Could you add just one more test for `PAS_Middle` please? A single line will be 
just enough.
Otherwise, LGTM.


https://reviews.llvm.org/D35743



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:22
+
+// Find a better way of detecting const-reference-template type
+// parameters via using alias not detected by ``isTemplateTypeParamType()``.

You can write `TODO: ` or `FIXME: ` in front of this comment to make it well 
visible.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType &ParamType) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+  std::string::npos);

This indeed looks a bit ugly. Is there no check that skips const-ref template 
params and handles `using`s / `typedef`s?



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+
+// Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType &ParamType) {

`TODO: ` too.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:31
+static bool isFunctionTemplate(const QualType &ParamType) {
+  return (ParamType.getAsString().find("std::function") != std::string::npos);
+}

I'm not sure if you can find a better way to find parameters of type 
`std::function` than this... Unless we find the rules that distinguish function 
types from others.
Why is `std::function` so different? How could we match `boost::function` and 
alike types?

Just setting the questions, I have no answers.

Anyway, I think that this might be left for later to be improved.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+AST_MATCHER(CXXMethodDecl, hasTemplateReturnType) {
+  // Don't put [[nodiscard]] int front functions return T
+  return (Node.getReturnType()->isTemplateTypeParmType() ||

int front ... -> in front of functions that return T.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:23
+via the return type. Unless the member functions are using mutable member
+variables or alteringing state via some external call (e.g. I/O).
+

Typo: alteringing



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:162
+using reference = value_type&;
+using const_reference = const value_type&;
+

Could you use similar test cases for `typedef`s, please?


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

LGTM.
Any ideas for improvement, @JonasToth?
I'd rather have it merged and improve on it later if there are ideas on how to 
do it better.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32
+  // Try to catch both std::function and boost::function
+  return (ParamType.getAsString().find("::function") != std::string::npos);
+}

I see that you check for any `::function` but you only test `std::function`. 
Could you add a test case for `boost::function` or any other `xyz::function` 
please?



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:127
+  auto &Context = *Result.Context;
+  // check for the existance of the keyword being used as the ``[[nodiscard]]``
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {

existance -> existence



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:231
+
+// don't mark typical ``[[nodisscard]]`` candidates if the class
+// has mutable member variables

Single 's' in `[[nodiscard]]`.


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

https://reviews.llvm.org/D55433



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

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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80309



___
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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I like these changes.
I have mixed feelings about `isCpp()` & co.
As @MyDeveloperDay said, I'd like it mean C++ only. I find it confusing that it 
means C++ or ObjC (even if the latter is a superset of the former). I'd rather 
see it spelt as `isCppOrObjC()` even if it's verbose but at least removes all 
confusion IMO.




Comment at: clang/include/clang/Format/Format.h:1632
+  bool isCppOnly() const { return Language == LK_Cpp; }
+  bool isObjectiveC() const { return Language == LK_ObjC; }
+  bool isCpp() const { return isCppOnly() || isObjectiveC(); }

sammccall wrote:
> Just my 2c - I find the current meaning of isCpp easier to understand, and 
> would prefer isObjectiveC to mean objective-C/C++. h if it exists.
> 
> Reasons:
>  - this is consistent with LangOptions::CPlusPlus and LangOptions::ObjC
>  - when checking for C++, also applying these rules to ObjC++ should be the 
> common/default case, and excluding ObjC++ the special case that justifies 
> more precise syntax (and honestly, I'd find `isCpp && !isObjC` to carry the 
> clearest intent in that case). IOW, this seems like it will attract bugs.
> 
> > perhaps a better name for isCpp() is isCStyleLanguages()
> 
> Clang uses the term "C family languages", and this includes C, C++, ObjC, 
> ObjC++.
> If you really want to avoid the conflict between C++ the boolean language 
> option and C++ the precise language mode, I'd suggest `isPlusPlus()` and 
> `isObjective()`. But I think consistency with LangOptions is worth more.
I'd rather go for coherence with `LanguageKind` options and spell it `isObjC()`.



Comment at: clang/include/clang/Format/Format.h:1635
   bool isCSharp() const { return Language == LK_CSharp; }
+  bool isProtoBuf() const { return Language == LK_Proto; }
+  bool isTableGen() const { return Language == LK_TableGen; }

sammccall wrote:
> These functions that don't *even in principle* do more than compare to an 
> enum seem like extra indirection that hurts understanding of the code (have 
> to look up what isObjectiveC() does, or have subtle bugs).
> 
> I suspect isCSharp() was added due to a misunderstanding of what isCpp() was 
> for.
Ditto, maybe `isProto` and `isTextProto`?


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] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Minor remarks.




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2346
 
+// look to see if we have [[ by looking ahead if
+// its not then rewind to the original position

Nit: shouldn't comments be "Full phrases."?



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2348
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();

MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > This so makes me feel like we need a peekNextToken()
> which is like all the time I have to write
> 
> ```
> Tok->Next && Tok->Next->is(tok::XXX)
> Tok->Previous && Tok->Previous ->is(tok::XXX)
> ```
> 
> would be so much smaller code if we had
> 
> ```
> Tok->isNext(tok::XXX)
> Tok->isPrevious(tok::XXX)
> ```
`peekNextToken()` probably should be done in a separate revision, nope?
It would be good to have it IMO.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2351
+  FormatToken *Tok = Tokens->getNextToken();
+  FormatTok = Tokens->setPosition(StoredPosition);
+  if (Tok->is(tok::l_square)) {

Maybe add `assert(Tok);`. How can you be know for sure that there's a next 
token?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80547



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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:359
 
+- Option ``ConstPlacement`` has been added auto-arrange the positioning of 
const
+  in variable and parameter declarations to be ``Right/East`` const or 
``Left/West`` 

It sounds strange as if you wanted to write "[in order] to auto-arrange".



Comment at: clang/lib/Format/EastWestConstFixer.cpp:83
+  // Change `const int` to be `int const`.
+  std::string NewType;
+  NewType += Second->TokenText;

Nit: `NewType` may be misleading when reading. Why not `NewText` as above?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:130
+
+static void swapFourTokens(const SourceManager &SourceMgr,
+ tooling::Replacements &Fixes,

These functions seem a bit ugly... and very specific. And they both look like 
rotate left/right. Couldn't it be a single function taking a 
range/span/collection of FormatTokens?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:136
+ const FormatToken *Fourth, 
+ bool West) {
+  // e.g. Change `const unsigned long long` to be `unsigned long long const`.

Nit: West doesn't seem appropriate as a name at the level of this function as 
you rather don't move elements west/east but left/right. Of course, that 
applies only if you share my view that it's sort of rotate algorithm.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:171
+return false;
+  return (Tok->isSimpleTypeSpecifier() ||
+  Tok->isOneOf(tok::kw_volatile, tok::kw_auto));

Parentheses seem to be unnecessary.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:171
+return false;
+  return (Tok->isSimpleTypeSpecifier() ||
+  Tok->isOneOf(tok::kw_volatile, tok::kw_auto));

curdeius wrote:
> Parentheses seem to be unnecessary.
Stupid question, are both const and restrict handled here?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:175
+
+// If a token is an identifier and its upper case it could
+// be a macro and hence we need to be able to ignore it

Typo: its -> it's.
Missing comma before "it could".



Comment at: clang/lib/Format/EastWestConstFixer.cpp:176
+// If a token is an identifier and its upper case it could
+// be a macro and hence we need to be able to ignore it
+static bool isPossibleMacro(const FormatToken *Tok)

Missing dot at the end.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {

Why? What about `unsigned const int`?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:208
+
+  if (isCVQualifierOrType(Tok->Next) && isCVQualifierOrType(Tok->Next->Next) 
&& isCVQualifierOrType(Tok->Next->Next->Next)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next->Next, 
Tok->Next->Next->Next,

I think that this code asks for a cleanup. And if we needed to look for five 
tokens...?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:213
+  }
+  if (isCVQualifierOrType(Tok->Next) && Tok->Next->Next &&
+  isCVQualifierOrType(Tok->Next->Next)) {

Shouldn't it be `else if`?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:223
+ Tok->Next->is(tok::kw_auto)) {
+// The basic case  `const int` -> `int const`
+swapTwoTokens(SourceMgr, Fixes, Tok, Tok->Next);

Nits: double space, missing ending dot.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:225
+swapTwoTokens(SourceMgr, Fixes, Tok, Tok->Next);
+
+  } else if (Tok->startsSequence(tok::kw_const, tok::identifier,

Nit: unnecessary blank line.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:228
+ TT_TemplateOpener)) {
+// Read from to the end of the TemplateOpener to
+// TemplateCloser const ArrayRef a; const ArrayRef &a;

"from to the end"?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:244
+  removeToken(SourceMgr, Fixes, Tok);
+  //return EndTemplate->Next;
+  return Tok;

Forgotten remnants?



Comment at: clang/lib/Format/EastWestConstFixer.cpp:383
+while (Tok && Tok != Last) {
+  if (!Tok->Next) {
+break;

It's a matter of taste, but this condition could be moved into the while 
condition above.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:387
+  if (Tok->is(tok::comment)) {
+Tok = Tok->Next;
+continue;
--

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

One last thought, how about making the config a bit more future-proof and 
instead of `ConstPlacement` accept what was discussed some time ago:

  QualifierStyle:
 -   const: Right

and restrict it to `const` for the moment?
Maybe renaming to `QualifierPlacement` or something more appropriate.


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

https://reviews.llvm.org/D69764



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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Marek Kurdej via Phabricator via cfe-commits]

curdeius marked an inline comment as done.
curdeius added inline comments.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:195
+FormatToken *Tok) {
+  // We only need to think about streams that begin with const.
+  if (!Tok->is(tok::kw_const)) {

MyDeveloperDay wrote:
> curdeius wrote:
> > Why? What about `unsigned const int`?
> @curdeius would you help me understand your expectation here?
> 
>  - east: `unsigned int const`
>  - west: `const unsigned int`
> 
> ?
Yes, precisely this. And as for all other cases, I would only move `const`, 
nothing else.



Comment at: clang/lib/Format/EastWestConstFixer.cpp:293
+  Tok->Next->Next->Next && Tok->Next->Next->Next->is(tok::kw_const)) {
+swapFourTokens(SourceMgr, Fixes, Tok, Tok->Next, Tok->Next, 
Tok->Next->Next->Next,
+ /*West=*/true);

MyDeveloperDay wrote:
> rsmith wrote:
> > There can be more than four type-specifiers / cv-qualifiers in a row. Eg: 
> > `unsigned long long int volatile const` -> `const volatile unsigned long 
> > long int`.
> you have the volatile moving too? if you had the choice would it become:
> 
> - `const unsigned long long int volatile`
> - `const volatile unsigned long long int`
> - `volatile const unsigned long long int`
> 
> Any reason why? or is that personal taste? what would be the ideal?
> 
> 
Given the size of this revision, it would be probably wiser not to touch 
anything else than `const`.


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

https://reviews.llvm.org/D69764



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

[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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/tools/generate_formatted_state.py:17
+with open(DOC_FILE, 'wb') as output:
+output.write(".. raw:: html\n")
+output.write("\n")

It will still not work with Python 3, you need to pass `bytes` to `write()` 
method.


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] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

First of all, very nice idea. :)
Second, could you please make sure that the script is compatible with Python 3?
Also, in order to clean up a bit the generation of the RST (to avoid the 
repetition of all this `output.write()` stuff), you can use multiline strings 
`"""`, or in general, take inspiration from e.g. 
https://github.com/llvm/llvm-project/blob/master/libcxx/utils/generate_feature_test_macro_components.py#L720.
Lastly, it would be nice to add last update date somewhere.


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] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

@MyDeveloperDay , I know it's a strange request, but could you change (or 
remove) the background color for 100% case.
I'm partially color-blind and having red and green background in the same table 
is really hard to distinguish. I guess I'm not alone.
I'd suggest using something like light blue, it doesn't need to stand out 
anyway.




Comment at: clang/docs/tools/generate_formatted_state.py:50
+totalFilesFail = 0
+for root, subdirs, files in os.walk(rootdir):
+path = os.path.relpath(root, LLVM_DIR)

Unused `subdirs` variable: change to `_`.



Comment at: clang/docs/tools/generate_formatted_state.py:52
+path = os.path.relpath(root, LLVM_DIR)
+if "/test/" in path:
+continue

That doesn't work on Windows because of slashes. You doesn't skip `unittests` 
(present at least in clang and llvm).



Comment at: clang/docs/tools/generate_formatted_state.py:56
+while head:
+fileCount = 0
+filePass = 0

curdeius wrote:
> You can move it outside the loop.
Here you use camelCase, but in other places you use snake_case (e.g. 
`file_path`). Please make that consistent.



Comment at: clang/docs/tools/generate_formatted_state.py:56-58
+fileCount = 0
+filePass = 0
+fileFail = 0

You can move it outside the loop.



Comment at: clang/docs/tools/generate_formatted_state.py:80
+if (fileCount > 0):
+output.write("   * - " + path + "\n")
+output.write(" - " + str(fileCount) + "\n")

Writing `path` directly on Windows will put backslashes that are not rendered 
in rst, so you should either change backslashes to forward slashes (that's what 
I'd suggest) or double the backslashes. You can just `path.replace('\\', '/')`.


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] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

@MyDeveloperDay, I've played around with the script, you can take as much as 
you judge useful from here: 
https://github.com/mkurdej/llvm-project/tree/arcpatch-D80627.


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] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

That looks nicer indeed. Thanks.
Just some minor nitty-gritty comments.




Comment at: clang/docs/tools/generate_formatted_state.py:52
+path = os.path.relpath(root, LLVM_DIR)
+if "/test/" in path:
+continue

MyDeveloperDay wrote:
> curdeius wrote:
> > That doesn't work on Windows because of slashes. You doesn't skip 
> > `unittests` (present at least in clang and llvm).
> So unit tests is something that I I think needs to be clang-formatted, this 
> is because often we are actively editing in there, (I use format on save) and 
> so having clean tests is super important
> 
> The tests directories normally have 100's of small snippets of code and some 
> may even be testing unformatted code deliberately, these files are often made 
> once and not continuously edited, (whilst it would be good to have them 
> clean, I wanted to give ourselves a fighting chance!)
> 
> Point taken about Windows, whilst I develop myself on Windows I use cygwin 
> which is why it probably worked.
OK, I agree for unittests. But then one could argue that the same should apply 
for test, nope?



Comment at: clang/docs/tools/generate_formatted_state.py:30
+.good {{ background-color: #2CCCFF }}
+
+

Nit: wrong indentation.



Comment at: clang/docs/tools/generate_formatted_state.py:74
+continue
+head, tail = os.path.split(root)
+while head:

Both `tail` and `_tail` seem unused. You can change to `_`.


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] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/tools/generate_formatted_state.py:54
+
+table_row = """   * - {path}
+ - {count}

Another nit: I prefer writing `"""\` as it nicely aligns with subsequent lines.


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] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM! Thanks!


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] D80830: [clang-format] [PR46130] When editing a file with unbalance {} the namespace comment fixer can incorrectly comment the wrong closing brace

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM.
It may be silly, but the fact that this code runs over all the tokens makes me 
think: do we have any performance-related non-regression tests for clang-format?




Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:1093
+
+TEST_F(NamespaceEndCommentsFixerTest, IgnoreUnbalanced) {
+  EXPECT_EQ("namespace A {\n"

Nit: You may add PR46130 in the test name *if* this is what is done usually (I 
see it in the test  above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80830



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

[PATCH] D80830: [clang-format] [PR46130] When editing a file with unbalance {} the namespace comment fixer can incorrectly comment the wrong closing brace

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

OK, great. Thanks for all the information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80830



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

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

The change seems to me technically sound, but I'm not sure of the scope of its 
effects. There might be users that rely on this behaviour. On the other hand, 
adding an option to keep the old behaviour doesn't seem appropriate, and 
personally I consider the old behaviour as a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80950



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

[PATCH] D80933: [clang-format] [PR46157] Wrong spacing of negative literals with use of operator

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:981
 consumeToken();
+if (CurrentToken->is(tok::comma) &&
+CurrentToken->Previous->isNot(tok::kw_operator))

Shouldn't you check `CurrentToken` for not being null after a call to 
`consumeToken`?? It may happen if `operator` is at the end of input (even if 
it's a degenerated case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80933



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

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I would have nothing against if you'd added this option and we kept current 
behaviour by default.
The only drawback is the (bit of) complexity it adds bit that seems justified 
to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80950



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

[PATCH] D80940: [clang-format] [PR46159] Linux kernel 'C' code uses 'try' as a variable name, allow clang-format to handle such cases

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Nice. Should we test other non-C keywords as well?
Out of curiosity, where does "@try" come from?




Comment at: clang/lib/Format/FormatTokenLexer.cpp:392
+  auto &Try = *(Tokens.end() - 2);
+  auto &Brace = *(Tokens.end() - 1);
+  if (!Try->is(tok::kw_try))

Please rename to something that includes colon or is more general. Just using 
Brace is misleading.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:393
+  auto &Brace = *(Tokens.end() - 1);
+  if (!Try->is(tok::kw_try))
+return false;

Nit: You can move this if above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80940



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

[PATCH] D80933: [clang-format] [PR46157] Wrong spacing of negative literals with use of operator

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM.
Keep in mind I'm not the code owner, I don't know if another approval is 
required.


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

https://reviews.llvm.org/D80933



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

[PATCH] D84090: [clang-format] Add SpaceAroundBitFieldColon option

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

The changes look good to me in general. I share your doubt though about whether 
a book flag is sufficient here. We've seen in the past a few times that at some 
time a false/true flag is not enough. I'd rather go for a 
Before/After/Both/None flag (or similar, naming probably should be coherent 
with other flags). But I'm not really aware of the projects/coding styles that 
use bit fields. Maybe a small research on this would be good to confirm or 
infirm a necessity of higher complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84090



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

[PATCH] D83296: [clang-format] Add a MacroExpander.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/MacroExpander.cpp:46
+
+  // Parse the token stream and return the corresonding Defintion object.
+  // Returns an empty definition object with a null-Name on error.

Nit: typo "corresponding Definition".



Comment at: clang/lib/Format/MacroExpander.cpp:110
+
+MacroExpander::~MacroExpander() {}
+

Why isn't it defaulted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83296



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

[PATCH] D80940: [clang-format] [PR46159] Linux kernel 'C' code uses 'try' as a variable name, allow clang-format to handle such cases

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM.

For a second, I thought that you could simplify the code by removing this @try 
condition (and calling the function 
`FormatTokenLexer::tryTransformTryUsageForC()` only if `isCppOnly`, but given 
that Objective is a superset of C, it's probably safer to keep it the way it's 
done now.


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

https://reviews.llvm.org/D80940



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

[PATCH] D82016: [clang-format] [PR462254] fix indentation of default and break correctly in whitesmiths style

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

Great!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82016



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

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Marek Kurdej via Phabricator via cfe-commits]

curdeius marked an inline comment as done.
curdeius added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:13540
   CHECK_PARSE_BOOL(AlignConsecutiveMacros);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeConceptDeclarations);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);

MyDeveloperDay wrote:
> curdeius wrote:
> > Format: leading whitespace. Should it be in the same "group" as 
> > `AlignConsecutiveMacros`?
> I might be misunderstanding your comment, but the list should alphabetic but 
> now I realize it should be after the `Allows`
I just wanted to point out that when you don't put a blank line after 
CHECK_PARSE_BOOL, then the next one gets indented. And yes, the order isn't ok. 
 BTW, us "Always" in the name necessary? Cf. other "Break" options.



Comment at: clang/unittests/Format/FormatTest.cpp:16745
+   "::std::is_copy_constructable and "
+   "::std::is_move_constructable and\n"
+   "requires (T c) {\n"

MyDeveloperDay wrote:
> curdeius wrote:
> > Isn't it a strange indentation?
> there is no newline on the line above, but I clang-format the tests so it 
> wraps it, I know visually its confusing
O, ok, thanks for the explanation. It's indeed confusing in the review.


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

https://reviews.llvm.org/D79773



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

[PATCH] D83564: [clang-format] PR46609 clang-format does not obey `PointerAlignment: Right` for ellipsis in declarator for pack

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM! Thanks for fixing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83564



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

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/include/clang/Format/Format.h:533
 
+  /// If ``true``, always break before concept declarations
+  bool AlwaysBreakBeforeConceptDeclarations;

It would be nice to have an example here in the doc.



Comment at: clang/unittests/Format/FormatTest.cpp:13540
   CHECK_PARSE_BOOL(AlignConsecutiveMacros);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeConceptDeclarations);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);

Format: leading whitespace. Should it be in the same "group" as 
`AlignConsecutiveMacros`?



Comment at: clang/unittests/Format/FormatTest.cpp:16745
+   "::std::is_copy_constructable and "
+   "::std::is_move_constructable and\n"
+   "requires (T c) {\n"

Isn't it a strange indentation?


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

https://reviews.llvm.org/D79773



___
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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



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

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




Comment at: clang/lib/Format/FormatTokenLexer.cpp:46-49
+  for (const std::string &WhitespaceSensitiveMacro :
+   Style.WhitespaceSensitiveMacros)
+Macros.insert(
+{&IdentTable.get(WhitespaceSensitiveMacro), TT_UntouchableMacroFunc});

Personally I would add braces around the loop body.



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

Shouldn't that be:
`CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",`
as in other options that take vector of strings?



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

Ditto: apostrophes around strings.



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

How about a test with escaped parentheses `\(` inside the macro argument?


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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM. Thank you for taking my comments into account.


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] D78982: [clang-format] Fix Microsoft style for enums

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

In D78982#2013221 , @RKSimon wrote:

> @asmith I'm seeing MSVC warnings from this patch:
>
> E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1475): warning 
> C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'
>  E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1828): warning 
> C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'


That's caused by the changed signature of `parseBracedList` and this change 
hasn't been taken into account at the above mentioned locations:

  - bool parseBracedList(bool ContinueOnSemicolons = false,
  +  bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = 
false, tok::TokenKind ClosingBraceKind = tok::r_brace);

BTW, it's a bit strange that a new parameter had been added in the middle of 
parameter list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78982



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

[PATCH] D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

> This fix tries to avoid that issue by determining that a ";" between the < 
> and > would not be a template (I couldn't think of an example where that 
> would be the case, but I'm sure there are..

For what is worth, with lambdas in unevaluated context (C++20), you can get a 
semicolon between template opener/closer easily:

  some_templated_type

But I'm not sure if it's something that may break anything here.

Another idea in which a semicolon occurs between <> would be a macro X(;).




Comment at: clang/lib/Format/TokenAnnotator.cpp:40
 /// keyword as a potential Objective-C selector component.
 static bool canBeObjCSelectorComponent(const FormatToken &Tok) {
   return Tok.Tok.getIdentifierInfo() != nullptr;

I know you haven't changed this, but...
Static function (internal linkage) in anonymous namespace?



Comment at: clang/lib/Format/TokenAnnotator.cpp:46
 /// `[...]<...>(`, where the [ opens a lambda capture list.
 static bool isLambdaParameterList(const FormatToken *Left) {
   // Skip <...> if present.

Ditto (static).



Comment at: clang/lib/Format/TokenAnnotator.cpp:1987
   llvm::SmallPtrSet NonTemplateLess;
-};
+}; // namespace
 

There should be no comment, it's the ending brace of a class.


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

https://reviews.llvm.org/D79293



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

[PATCH] D85600: [clang-format] use spaces for alignment of binary/ternary expressions with UT_AlignWithSpaces

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1362-1367
+if (Style.AlignOperands != FormatStyle::OAS_DontAlign && Previous &&
+(Previous->getPrecedence() == prec::Assignment ||
+ Previous->is(tok::kw_return) ||
+ (*I == prec::Conditional && Previous->is(tok::question) &&
+  Previous->is(TT_ConditionalExpr))) &&
+!Newline)

You could factor out the part of the condition that doesn't concern 
`AlignOperands` to get rid of the repetition wrt. the previous `if`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85600

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

[PATCH] D85600: [clang-format] use spaces for alignment of binary/ternary expressions with UT_AlignWithSpaces

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM. Thank you for the patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85600

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

[PATCH] D85600: [clang-format] use spaces for alignment of binary/ternary expressions with UT_AlignWithSpaces

[Marek Kurdej via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb18c63e85aa8: [clang-format] use spaces for alignment of 
binary/ternary expressions with… (authored by fickert, committed by curdeius).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85600

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11259,6 +11259,23 @@
"\t}\n"
"};",
Tab);
+  Tab.AlignOperands = FormatStyle::OAS_Align;
+  verifyFormat("int aa =  +\n"
+   " ;",
+   Tab);
+  // no alignment
+  verifyFormat("int aa =\n"
+   "\t;",
+   Tab);
+  verifyFormat("return  ? 111\n"
+   "   : bb ? 222\n"
+   ": 333;",
+   Tab);
+  Tab.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Tab.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+  verifyFormat("int aa = \n"
+   "   + ;",
+   Tab);
 }
 
 TEST_F(FormatTest, ZeroTabWidth) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1348,16 +1348,20 @@
State.Stack.back().LastSpace);
 }
 
-// If BreakBeforeBinaryOperators is set, un-indent a bit to account for
-// the operator and keep the operands aligned
-if (Style.AlignOperands == FormatStyle::OAS_AlignAfterOperator &&
-Previous &&
+if (Previous &&
 (Previous->getPrecedence() == prec::Assignment ||
  Previous->is(tok::kw_return) ||
  (*I == prec::Conditional && Previous->is(tok::question) &&
   Previous->is(TT_ConditionalExpr))) &&
-!Newline)
-  NewParenState.UnindentOperator = true;
+!Newline) {
+  // If BreakBeforeBinaryOperators is set, un-indent a bit to account for
+  // the operator and keep the operands aligned
+  if (Style.AlignOperands == FormatStyle::OAS_AlignAfterOperator)
+NewParenState.UnindentOperator = true;
+  // Mark indentation as alignment if the expression is aligned.
+  if (Style.AlignOperands != FormatStyle::OAS_DontAlign)
+NewParenState.IsAligned = true;
+}
 
 // Do not indent relative to the fake parentheses inserted for "." or "->".
 // This is a special case to make the following to statements consistent:


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11259,6 +11259,23 @@
"\t}\n"
"};",
Tab);
+  Tab.AlignOperands = FormatStyle::OAS_Align;
+  verifyFormat("int aa =  +\n"
+   " ;",
+   Tab);
+  // no alignment
+  verifyFormat("int aa =\n"
+   "\t;",
+   Tab);
+  verifyFormat("return  ? 111\n"
+   "   : bb ? 222\n"
+   ": 333;",
+   Tab);
+  Tab.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
+  Tab.AlignOperands = FormatStyle::OAS_AlignAfterOperator;
+  verifyFormat("int aa = \n"
+   "   + ;",
+   Tab);
 }
 
 TEST_F(FormatTest, ZeroTabWidth) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1348,16 +1348,20 @@
State.Stack.back().LastSpace);
 }
 
-// If BreakBeforeBinaryOperators is set, un-indent a bit to account for
-// the operator and keep the operands aligned
-if (Style.AlignOperands == FormatStyle::OAS_AlignAfterOperator &&
-Previous &&
+if (Previous &&
 (Previous->getPrecedence() == prec::Assignment ||
  Previous->is(tok::kw_return) ||
  (*I == prec::Conditional && Previous->is(tok::question) &&
   Previous->is(TT_ConditionalExpr))) &&
-!Newline)
-  NewParenState.UnindentOperator = true;
+!Newline) {
+  // If Break

[PATCH] D79935: [clang-format] [PR44345] Long namespace closing comment is duplicated endlessly

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:129
+  // } // namespace
+  //   // verylongnamespacenamethatdidnotfitonthepreviouscommenline
+  if (!(Comment->Next && Comment->Next->is(TT_LineComment)))

Nit: typo commenline -> commentline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79935



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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

@eoanermine, please provide your name and email address for the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D131978: [clang-format] Concepts: allow identifiers after negation

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

@rymiel, please provide your name and email address for the commit message, so 
that it can land it for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131978

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

[PATCH] D126365: [git-clang-format] Stop ignoring changes for files with space in path

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

@Eitot, please provide your name and email address for the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126365

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

[PATCH] D132189: [clang-format] Don't put `noexcept` on empty line following constructor

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

@rymiel, please provide your name and email address for the commit message, so 
that we can land it for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132189

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

[PATCH] D132295: [clang-format] Change heuristic for locating lambda template arguments

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

@rymiel, please provide your name and email address for the commit message, so 
that we can land it for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132295

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

[PATCH] D132762: [clang-format] Allow `throw` to be a keyword in front of casts

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

@rymiel, please provide your name and email address for the commit message, so 
that we can land it for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132762

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

[PATCH] D133571: [clang-format] Introduce NoFallThrough option into AllowShortCaseLabelsOnASingleLine

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:935
+**AllowShortCaseLabelsOnASingleLine** (``ShortCaseLabelStyle``) 
:versionbadge:`clang-format 3.6`
+  Determine if Short case labels will be contracted to a single line.
+





Comment at: clang/docs/ClangFormatStyleOptions.rst:3596
 
-  QualifierOrder: ['inline', 'static', 'type', 'const']
+  QualifierOrder: ['inline', 'static' , 'type', 'const']
 

Unrelated change.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:643-657
+bool NoFallThrough = Style.AllowShortCaseLabelsOnASingleLine ==
+ FormatStyle::SCLS_NoFallThrough;
+// If the last thing on the line before was just case X:  then don't merge.
+if (NoFallThrough && PreviousLine && PreviousLine->Last &&
+PreviousLine->Last->is(tok::colon))
+  return 0;
+// Check if the last thing on the line before was an attribute and if so

Can we merge the conditions like this?


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

https://reviews.llvm.org/D133571

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

[PATCH] D133571: [clang-format] Introduce NoFallThrough option into AllowShortCaseLabelsOnASingleLine

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM after fixing the last comment.




Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:650
+  // Don't merge if the last thing on the line before was an attribute.
+  if (PreviousLine->Last->Previous && PreviousLine->Last->Previous) {
+auto *PrevPrev = PreviousLine->Last->Previous->Previous;

Haven't seen this before.


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

https://reviews.llvm.org/D133571

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

[PATCH] D133589: [clang-format] JSON formatting add new option for controlling newlines in json arrays

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3115
 
+**JsonMultilineArrays** (``Boolean``) :versionbadge:`clang-format 16`
+  If ``true``, clang-format will always break after a Json array `[`

Why limiting to JSON only?
Could we name it in a general fashion (we comment that it's JSON only for the 
time being). I believe it may be an interesting option for various languages.

How about BreakMultilineArrays, or just BreakArrays to follow the naming of 
existing options a bit?



Comment at: clang/lib/Format/TokenAnnotator.cpp:4413
+  while (Tok) {
+if (Tok->isOneOf(tok::l_brace, tok::l_square)) {
+  return true;

Please elide braces in the ifs.



Comment at: clang/lib/Format/TokenAnnotator.cpp:4427
+if (Left.is(tok::comma)) {
+  if (Right.is(tok::l_brace)) {
+return true;

You can elide braces here.



Comment at: clang/lib/Format/TokenAnnotator.cpp:4434
+  while (Tok) {
+if (Tok->isOneOf(tok::l_brace, tok::l_square)) {
+  return true;

You might elide braces in both ifs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133589

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

[PATCH] D129057: [clang-format] Break on AfterColon only if not followed by comment

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:7099
OnePerLine);
-  verifyFormat("SomeClass::Constructor() :\n"
+  verifyFormat("SomeClass::Constructor() : // NOLINT\n"
"a(aa), // Some comment\n"

You should add a test without modifying an existing one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129057

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

[PATCH] D129057: [clang-format] Break on AfterColon only if not followed by comment

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:7136
Style);
+  verifyFormat("Constructor() : // NOLINT\n"
+   "() {}",

How about very long comments? They don't get split now? Please add a test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129057

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

[PATCH] D129057: [clang-format] Break on AfterColon only if not followed by comment

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM. Thanks for addressing my comments.


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

https://reviews.llvm.org/D129057

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

[PATCH] D129064: [clang-format] Avoid crash in LevelIndentTracker.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
curdeius added reviewers: HazardyKnusperkeks, owenpan, MyDeveloperDay.
Herald added a project: All.
curdeius requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes https://github.com/llvm/llvm-project/issues/56352.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129064

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -609,6 +609,14 @@
  "  return a == 8 ? 32 : 16;\n"
  "}\n";
   EXPECT_EQ(Code, format(Code, 40, 0));
+
+  // https://llvm.org/PR56352
+  Style.CompactNamespaces = true;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Code = "\n"
+ "namespace ns1 { namespace ns2 {\n"
+ "}}";
+  EXPECT_EQ(Code, format(Code, 0, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -66,7 +66,8 @@
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
   Indent = Line.Level * IndentWidth + AdditionalIndent;
 } else {
-  IndentForLevel.resize(Line.Level + 1);
+  if (IndentForLevel.size() < Line.Level + 1)
+IndentForLevel.resize(Line.Level + 1);
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -91,6 +92,7 @@
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
+assert(Line.Level < IndentForLevel.size());
 if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
 !Line.InPPDirective) {
   IndentForLevel[Line.Level] = LevelIndent;


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -609,6 +609,14 @@
  "  return a == 8 ? 32 : 16;\n"
  "}\n";
   EXPECT_EQ(Code, format(Code, 40, 0));
+
+  // https://llvm.org/PR56352
+  Style.CompactNamespaces = true;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Code = "\n"
+ "namespace ns1 { namespace ns2 {\n"
+ "}}";
+  EXPECT_EQ(Code, format(Code, 0, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -66,7 +66,8 @@
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
   Indent = Line.Level * IndentWidth + AdditionalIndent;
 } else {
-  IndentForLevel.resize(Line.Level + 1);
+  if (IndentForLevel.size() < Line.Level + 1)
+IndentForLevel.resize(Line.Level + 1);
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -91,6 +92,7 @@
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
+assert(Line.Level < IndentForLevel.size());
 if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
 !Line.InPPDirective) {
   IndentForLevel[Line.Level] = LevelIndent;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129064: [clang-format] Avoid crash in LevelIndentTracker.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius updated this revision to Diff 442482.
curdeius marked an inline comment as done.
curdeius added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129064

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -609,6 +609,14 @@
  "  return a == 8 ? 32 : 16;\n"
  "}\n";
   EXPECT_EQ(Code, format(Code, 40, 0));
+
+  // https://llvm.org/PR56352
+  Style.CompactNamespaces = true;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Code = "\n"
+ "namespace ns1 { namespace ns2 {\n"
+ "}}";
+  EXPECT_EQ(Code, format(Code, 0, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -66,7 +66,6 @@
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
   Indent = Line.Level * IndentWidth + AdditionalIndent;
 } else {
-  IndentForLevel.resize(Line.Level + 1);
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -91,6 +90,7 @@
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
+assert(Line.Level < IndentForLevel.size());
 if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
 !Line.InPPDirective) {
   IndentForLevel[Line.Level] = LevelIndent;


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -609,6 +609,14 @@
  "  return a == 8 ? 32 : 16;\n"
  "}\n";
   EXPECT_EQ(Code, format(Code, 40, 0));
+
+  // https://llvm.org/PR56352
+  Style.CompactNamespaces = true;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Code = "\n"
+ "namespace ns1 { namespace ns2 {\n"
+ "}}";
+  EXPECT_EQ(Code, format(Code, 0, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -66,7 +66,6 @@
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
   Indent = Line.Level * IndentWidth + AdditionalIndent;
 } else {
-  IndentForLevel.resize(Line.Level + 1);
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -91,6 +90,7 @@
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
+assert(Line.Level < IndentForLevel.size());
 if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
 !Line.InPPDirective) {
   IndentForLevel[Line.Level] = LevelIndent;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129064: [clang-format] Avoid crash in LevelIndentTracker.

[Marek Kurdej via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14c30c70c459: [clang-format] Avoid crash in 
LevelIndentTracker. (authored by curdeius).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129064

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -609,6 +609,14 @@
  "  return a == 8 ? 32 : 16;\n"
  "}\n";
   EXPECT_EQ(Code, format(Code, 40, 0));
+
+  // https://llvm.org/PR56352
+  Style.CompactNamespaces = true;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Code = "\n"
+ "namespace ns1 { namespace ns2 {\n"
+ "}}";
+  EXPECT_EQ(Code, format(Code, 0, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -66,7 +66,6 @@
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
   Indent = Line.Level * IndentWidth + AdditionalIndent;
 } else {
-  IndentForLevel.resize(Line.Level + 1);
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -91,6 +90,7 @@
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
+assert(Line.Level < IndentForLevel.size());
 if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
 !Line.InPPDirective) {
   IndentForLevel[Line.Level] = LevelIndent;


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -609,6 +609,14 @@
  "  return a == 8 ? 32 : 16;\n"
  "}\n";
   EXPECT_EQ(Code, format(Code, 40, 0));
+
+  // https://llvm.org/PR56352
+  Style.CompactNamespaces = true;
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Code = "\n"
+ "namespace ns1 { namespace ns2 {\n"
+ "}}";
+  EXPECT_EQ(Code, format(Code, 0, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -66,7 +66,6 @@
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
   Indent = Line.Level * IndentWidth + AdditionalIndent;
 } else {
-  IndentForLevel.resize(Line.Level + 1);
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -91,6 +90,7 @@
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
+assert(Line.Level < IndentForLevel.size());
 if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
 !Line.InPPDirective) {
   IndentForLevel[Line.Level] = LevelIndent;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129348: [clang-format] Fix an assertion failure on -lines=0:n

[Marek Kurdej via Phabricator via cfe-commits]

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

👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129348

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

[PATCH] D121584: [clang-format] Correctly recognize arrays in template parameter list.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Yes, let's revert this. I should have a fix soon though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121584

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

[PATCH] D122468: [clang-format] Fix SeparateDefinitionBlocks breaking up function-try-block.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
curdeius added reviewers: MyDeveloperDay, HazardyKnusperkeks, owenpan.
Herald added a project: All.
curdeius requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes https://github.com/llvm/llvm-project/issues/54536.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122468

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/DefinitionBlockSeparatorTest.cpp


Index: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
===
--- clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
+++ clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
@@ -67,6 +67,25 @@
 EXPECT_EQ(ExpectedCode, Result) << "Test failed. Formatted:\n" << Result;
   }
 
+  static void _verifyFormatNoInverse(const char *File, int Line,
+ llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ llvm::StringRef ExpectedCode = "") {
+::testing::ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+bool HasOriginalCode = true;
+if (ExpectedCode == "") {
+  ExpectedCode = Code;
+  HasOriginalCode = false;
+}
+
+EXPECT_EQ(ExpectedCode, separateDefinitionBlocks(ExpectedCode, Style))
+<< "Expected code is not stable";
+std::string CodeToFormat =
+HasOriginalCode ? Code.str() : removeEmptyLines(Code);
+std::string Result = separateDefinitionBlocks(CodeToFormat, Style);
+EXPECT_EQ(ExpectedCode, Result) << "Test failed. Formatted:\n" << Result;
+  }
+
   static std::string removeEmptyLines(llvm::StringRef Code) {
 std::string Result = "";
 for (auto Char : Code.str()) {
@@ -83,6 +102,8 @@
 };
 
 #define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+#define verifyFormatNoInverse(...) 
\
+  _verifyFormatNoInverse(__FILE__, __LINE__, __VA_ARGS__)
 
 TEST_F(DefinitionBlockSeparatorTest, Basic) {
   FormatStyle Style = getLLVMStyle();
@@ -448,6 +469,32 @@
Style);
 }
 
+TEST_F(DefinitionBlockSeparatorTest, TryBlocks) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+  verifyFormatNoInverse("void FunctionWithInternalTry()\n"
+"{\n"
+"  try\n"
+"  {\n"
+"return;\n"
+"  }\n"
+"  catch (const std::exception &)\n"
+"  {\n"
+"  }\n"
+"}",
+Style);
+  verifyFormatNoInverse("void FunctionWithTryBlock()\n"
+"try\n"
+"{\n"
+"  return;\n"
+"}\n"
+"catch (const std::exception &)\n"
+"{\n"
+"}",
+Style);
+}
+
 TEST_F(DefinitionBlockSeparatorTest, Leave) {
   FormatStyle Style = getLLVMStyle();
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Leave;
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2603,6 +2603,7 @@
   nextToken();
 }
 NeedsUnwrappedLine = false;
+Line->MustBeDeclaration = false;
 CompoundStatementIndenter Indenter(this, Style, Line->Level);
 parseBlock();
 if (Style.BraceWrapping.BeforeCatch)


Index: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
===
--- clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
+++ clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
@@ -67,6 +67,25 @@
 EXPECT_EQ(ExpectedCode, Result) << "Test failed. Formatted:\n" << Result;
   }
 
+  static void _verifyFormatNoInverse(const char *File, int Line,
+ llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ llvm::StringRef ExpectedCode = "") {
+::testing::ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+bool HasOriginalCode = true;
+if (ExpectedCode == "") {
+  ExpectedCode = Code;
+  HasOriginalCode = false;
+}
+
+EXPECT_EQ(ExpectedCode, separateDefinitionBlocks(ExpectedCode, Style))
+<< "Expected code is not stable";
+std::string CodeToFormat =
+HasOriginalCode ? Code.str() : removeEmptyLines(Code);
+std::string Result = separateDefinitionBlocks(CodeToFormat, Style);
+EXPECT_EQ(ExpectedCode, Result) << "Test failed. For

[PATCH] D106112: [clang-format] Break an unwrapped line at a K&R C parameter decl

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Looks okay, but I was wondering if we don't want to guard all K&R-related 
changes behind e.g. ```Standard: Cpp78``, so as not to possibly introduce 
strange bugs in newer modes.
It may be an overkill if there are 2 patches like this, but if there are more, 
that might become sensible to do so.
WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106112

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

[PATCH] D106112: [clang-format] Break an unwrapped line at a K&R C parameter decl

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

In D106112#2885604 , @owenpan wrote:

> I just reviewed the differences between K&R C (circa 1978) and ANSI/ISO C 
> again and didn't see anything else that would impact clang-format, so a new 
> Standard enum value for C78 is not needed. Nevertheless, we can add a boolean 
> option e.g. C78ParameterDecl in the future if this patch causes regressions 
> for some users. WDYT?

Sounds reasonable.
Any way, you can go forward and land this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106112

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

[PATCH] D106773: [clang-format] Fix aligning with linebreaks #2

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM! That was quick! Thanks.
Please wait for @baramin to validate the fix before landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106773

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Marek Kurdej via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG71616722d409: [clang-format] Correctly attach enum braces 
with ShortEnums disabled (authored by lunasorcery, committed by curdeius).

Changed prior to commit:
  https://reviews.llvm.org/D99840?vs=362220&id=362302#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -402,8 +402,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2451,6 +2451,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -22123,8 +22131,7 @@
Style);
   // Enumerations are not records and should be unaffected.
   Style.AllowShortEnumsOnASingleLine = false;
-  verifyFormat("enum class E\n"
-   "{\n"
+  verifyFormat("enum class E {\n"
"  A,\n"
"  B\n"
"};\n",
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2515,6 +2515,8 @@
   if (FormatTok->Tok.is(tok::kw_enum))
 nextToken();
 
+  const FormatToken &InitialToken = *FormatTok;
+
   // In TypeScript, "enum" can also be used as property name, e.g. in interface
   // declarations. An "enum" keyword followed by a colon would be a syntax
   // error and thus assume it is just an identifier.
@@ -2561,7 +2563,8 @@
 return true;
   }
 
-  if (!Style.AllowShortEnumsOnASingleLine)
+  if (!Style.AllowShortEnumsOnASingleLine &&
+  ShouldBreakBeforeBrace(Style, InitialToken))
 addUnwrappedLine();
   // Parse enum body.
   nextToken();
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -544,8 +544,7 @@
   ///   enum { A, B } myEnum;
   ///
   ///   false:
-  ///   enum
-  ///   {
+  ///   enum {
   /// A,
   /// B
   ///   } myEnum;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -153,7 +153,8 @@
 clang-format
 
 
-- ...
+- Option ``AllowShortEnumsOnASingleLine: false`` has been improved, it now
+  correctly places the opening brace according to ``BraceWrapping.AfterEnum``.
 
 libclang
 


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -402,8 +402,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2451,6 +2451,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -22123,8 +22131,7 @@
Style);
   // Enumerations are 

[PATCH] D106773: [clang-format] Fix aligning with linebreaks #2

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

This bug fix should probably be cherry-picked into 13.x branch. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106773

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

[PATCH] D106773: [clang-format] Fix aligning with linebreaks #2

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a subscriber: tstellar.
curdeius added a comment.

In D106773#2912925 , 
@HazardyKnusperkeks wrote:

> In D106773#2912732 , @curdeius 
> wrote:
>
>> This bug fix should probably be cherry-picked into 13.x branch. WDYT?
>
> Yeah, I wrote already an email about it. Or can (and should) I push it myself?

I think we should ask @tstellar for permission before cherry-picking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106773

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

[PATCH] D121269: [clang-format] Fix namespace format when the name is followed by a macro

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:192-211
+  EXPECT_EQ("#define M(x) x##x\n"
+"namespace A M(x) {\n"
+"int i;\n"
+"int j;\n"
+"}// namespace A M(x)",
+fixNamespaceEndComments("#define M(x) x##x\n"
+"namespace A M(x) {\n"

MyDeveloperDay wrote:
> Can you test the A B case? We can’t have a space right?
What's the rationale behind keeping `M(x)` in one case and not the other?
How can you decide?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121269

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

[PATCH] D121269: [clang-format] Fix namespace format when the name is followed by a macro

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:72
 
-Tok = FirstNSTok;
-while (Tok && !Tok->is(tok::l_brace)) {
+bool IsPrevColoncolon = false;
+bool HasColoncolon = false;

Nit: Personally, I'd put these bools further down, closer to the place where 
they are used and after AddMacro lambda.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:85
+  Tok = Tok->getNextNonComment();
+  for (int NestLevel = 1; Tok && NestLevel > 0;) {
+if (Tok->is(tok::l_paren))

This loop seems very similar to the one that skips the attributes on lines 
45-53, could you please refactor?
A lambda taking a pair of token kinds (l_paren/r_paren, l_square/r_square) 
should be fine.



Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:192
 "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+"namespace A M(x) {\n"

zequanwu wrote:
> zequanwu wrote:
> > curdeius wrote:
> > > MyDeveloperDay wrote:
> > > > Can you test the A B case? We can’t have a space right?
> > > What's the rationale behind keeping `M(x)` in one case and not the other?
> > > How can you decide?
> > In the first one, we keep `M(x)` because we don't know that is `A` or 
> > `M(x)` the name. 
> > In the second one, because we see a `::`, so we know that's the name and 
> > discard `M(x)`.
> Added a test with `A B`. 
> In that case, we will have `// namespace A B`, since we don't know which one 
> is the real name. Either one could be a macro that expands to an attribute.
In a different test case, could you add A to AttributeMacros and test that we 
skip it like other attributes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121269

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

[PATCH] D121269: [clang-format] Fix namespace format when the name is followed by a macro

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:192
 "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+"namespace A M(x) {\n"

curdeius wrote:
> zequanwu wrote:
> > zequanwu wrote:
> > > curdeius wrote:
> > > > MyDeveloperDay wrote:
> > > > > Can you test the A B case? We can’t have a space right?
> > > > What's the rationale behind keeping `M(x)` in one case and not the 
> > > > other?
> > > > How can you decide?
> > > In the first one, we keep `M(x)` because we don't know that is `A` or 
> > > `M(x)` the name. 
> > > In the second one, because we see a `::`, so we know that's the name and 
> > > discard `M(x)`.
> > Added a test with `A B`. 
> > In that case, we will have `// namespace A B`, since we don't know which 
> > one is the real name. Either one could be a macro that expands to an 
> > attribute.
> In a different test case, could you add A to AttributeMacros and test that we 
> skip it like other attributes?
Euh, actually, it's out of scope of this patch. Please ignore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121269

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

[PATCH] D121269: [clang-format] Fix namespace format when the name is followed by a macro

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM % nits. Thanks for working on this!




Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:54-55
+Tok = processTokens(Tok, tok::l_paren, tok::r_paren, nullptr);
+  } else if (Tok->is(tok::l_square))
+Tok = processTokens(Tok, tok::l_square, tok::r_square, nullptr);
+  return Tok;

Nit: add braces around `else if` block to match the `if`.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:77
   } else {
 // Skip attributes.
+Tok = skipAttribute(Tok);

Comment not necessary anymore. Please remove.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:130
   Tok = Tok->getNextNonComment();
+  // Skip attribute
+  const FormatToken *TokAfterAttr = skipAttribute(Tok);

Comment not necessary anymore. Please remove.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:138
+if (!FirstNSName.empty() && !HasColoncolon)
+  name = FirstNSName + (!name.empty() ? " " + name : "");
   }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121269

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

[PATCH] D119599: [clang-format] Add option to align compound assignments like `+=`

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM. Thanks a lot!




Comment at: clang/lib/Format/Format.cpp:1181-1184
+  LLVMStyle.AlignConsecutiveAssignments = {
+  /*Enabled=*/false, /*AcrossEmptyLines=*/false,
+  /*AcrossComments=*/false, /*AlignCompound=*/false,
+  /*PadOperators=*/true};

That should be more future-proof.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119599

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

[PATCH] D121370: [clang-format] SortIncludes should support "@import" lines in Objective-C

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/Format.cpp:2685
 const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+R"(^[\t\ ]*[@#]?[\t\ ]*(import|include)[^"<]*[\t\n\ 
\\]*("[^"]+"|<[^>]+>|[^"<>;]+;))";
 

I'd rather see handling separately the `#import`/`#include` case on one hand 
and `@import` on the other.
IIUC, `@import` is followed by `ns1.ns2.something` without quotes nor `<>`. And 
it ends with a `;`.
Trying to put everything together is IMO error-prone and not very readable.

You could just do (pseudo-code):
```
CppIncludeRegexPattern = (current_regex) | "@\s*import\s+[^;]+;"
```
(`\s` is probably not supported, but we could use character class `[:space:]` 
for clarity, cf. llvm\lib\Support\regcomp.c)

Not sure whether whitespace is allowed after `@`.
Could you please post a link to the specification of `@import` syntax?

Not sure fixing `<"` / `">` issue is worth it, but in all cases I'd prefer 
seeing it in a different patch.



Comment at: clang/lib/Format/Format.cpp:2759
+  IncludeName =
+  StringRef(Twine("<", IncludeName).concat(Twine(">")).str());
+}

HazardyKnusperkeks wrote:
> How does that not go out of scope and I have a dangling reference?
Why not doing it the other way round, i.e. trimming `<>"` from include name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121370

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

[PATCH] D121370: [clang-format] SortIncludes should support "@import" lines in Objective-C

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/Format.cpp:2685
 const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+R"(^[\t\ ]*[@#]?[\t\ ]*(import|include)[^"<]*[\t\n\ 
\\]*("[^"]+"|<[^>]+>|[^"<>;]+;))";
 

curdeius wrote:
> I'd rather see handling separately the `#import`/`#include` case on one hand 
> and `@import` on the other.
> IIUC, `@import` is followed by `ns1.ns2.something` without quotes nor `<>`. 
> And it ends with a `;`.
> Trying to put everything together is IMO error-prone and not very readable.
> 
> You could just do (pseudo-code):
> ```
> CppIncludeRegexPattern = (current_regex) | "@\s*import\s+[^;]+;"
> ```
> (`\s` is probably not supported, but we could use character class `[:space:]` 
> for clarity, cf. llvm\lib\Support\regcomp.c)
> 
> Not sure whether whitespace is allowed after `@`.
> Could you please post a link to the specification of `@import` syntax?
> 
> Not sure fixing `<"` / `">` issue is worth it, but in all cases I'd prefer 
> seeing it in a different patch.
`[:space:]` would match newlines and we don't want this in all cases, but 
`[:blank:]` should be a good replacement for `[\t ]`.

On a different note, why there's a new line `\n` and backslash `\\` in `[\t\n\ 
\\]`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121370

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

[PATCH] D121450: [clang-format] Handle attributes before case label.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
curdeius added reviewers: MyDeveloperDay, HazardyKnusperkeks, owenpan.
Herald added a project: All.
curdeius requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes https://github.com/llvm/llvm-project/issues/53110.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121450

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2602,6 +2602,52 @@
"}",
getLLVMStyleWithColumns(34));
 
+  verifyFormat("switch (a) {\n"
+   "[[likely]] case 1:\n"
+   "  return;\n"
+   "}");
+  verifyFormat("switch (a) {\n"
+   "[[likely]] [[other::likely]] case 1:\n"
+   "  return;\n"
+   "}");
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "  return;\n"
+   "[[likely]] case 2:\n"
+   "  return;\n"
+   "}");
+  verifyFormat("switch (a) {\n"
+   "case 1:\n"
+   "[[likely]] case 2:\n"
+   "  return;\n"
+   "}");
+  FormatStyle Attributes = getLLVMStyle();
+  Attributes.AttributeMacros.push_back("LIKELY");
+  Attributes.AttributeMacros.push_back("OTHER_LIKELY");
+  verifyFormat("switch (a) {\n"
+   "LIKELY case b:\n"
+   "  return;\n"
+   "}",
+   Attributes);
+  verifyFormat("switch (a) {\n"
+   "LIKELY OTHER_LIKELY() case b:\n"
+   "  return;\n"
+   "}",
+   Attributes);
+  verifyFormat("switch (a) {\n"
+   "case 1:\n"
+   "  return;\n"
+   "LIKELY case 2:\n"
+   "  return;\n"
+   "}",
+   Attributes);
+  verifyFormat("switch (a) {\n"
+   "case 1:\n"
+   "LIKELY case 2:\n"
+   "  return;\n"
+   "}",
+   Attributes);
+
   FormatStyle Style = getLLVMStyle();
   Style.IndentCaseLabels = true;
   Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -122,6 +122,8 @@
   void keepAncestorBraces();
   void parseUnbracedBody(bool CheckEOF = false);
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
+  void handleAttributes();
+  bool handleCppAttributes();
   void parseTryCatch();
   void parseForOrWhileLoop();
   void parseDoWhile();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -480,6 +480,10 @@
   unsigned StatementCount = 0;
   bool SwitchLabelEncountered = false;
   do {
+if (FormatTok->getType() == TT_AttributeMacro) {
+  nextToken();
+  continue;
+}
 tok::TokenKind kind = FormatTok->Tok.getKind();
 if (FormatTok->getType() == TT_MacroBlockBegin)
   kind = tok::l_brace;
@@ -569,6 +573,9 @@
 parseCSharpAttribute();
 break;
   }
+  if (handleCppAttributes())
+break;
+
   LLVM_FALLTHROUGH;
 default:
   ParseDefault();
@@ -1390,9 +1397,11 @@
 // e.g. "default void f() {}" in a Java interface.
 break;
   case tok::kw_case:
-if (Style.isJavaScript() && Line->MustBeDeclaration)
+if (Style.isJavaScript() && Line->MustBeDeclaration) {
   // 'case: string' field declaration.
+  nextToken();
   break;
+}
 parseCaseLabel();
 return;
   case tok::kw_try:
@@ -1813,6 +1822,12 @@
 case tok::kw_new:
   parseNew();
   break;
+case tok::kw_case:
+  if (Style.isJavaScript() && Line->MustBeDeclaration)
+// 'case: string' field declaration.
+break;
+  parseCaseLabel();
+  break;
 default:
   nextToken();
   break;
@@ -2376,17 +2391,24 @@
   RightBrace->Optional = true;
 }
 
+void UnwrappedLineParser::handleAttributes() {
+  // Handle AttributeMacro, e.g. `if (x) UNLIKELY`.
+  if (FormatTok->is(TT_AttributeMacro))
+nextToken();
+  handleCppAttributes();
+}
+
+bool UnwrappedLineParser::handleCppAttributes() {
+  // Handle [[likely]] / [[unlikely]] attributes.
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute()) {
+parseSquare();
+return true;
+  }
+  return false;
+}
+
 FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
   bool KeepBraces) {
-  auto HandleAttributes = [this]() {
-// Hand

[PATCH] D121451: [clang-format] Add space to comments starting with '#'.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
curdeius added reviewers: MyDeveloperDay, HazardyKnusperkeks, owenpan.
Herald added a project: All.
curdeius requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes https://github.com/llvm/llvm-project/issues/35116.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121451

Files:
  clang/lib/Format/BreakableToken.cpp
  clang/unittests/Format/FormatTestComments.cpp


Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -91,6 +91,9 @@
"// line 2\n"
"void f() {}\n");
 
+  EXPECT_EQ("// comment\n", format("//comment\n"));
+  EXPECT_EQ("// #comment\n", format("//#comment\n"));
+
   EXPECT_EQ("// comment\n"
 "// clang-format on\n",
 format("//comment\n"
Index: clang/lib/Format/BreakableToken.cpp
===
--- clang/lib/Format/BreakableToken.cpp
+++ clang/lib/Format/BreakableToken.cpp
@@ -779,11 +779,14 @@
 const char FirstCommentChar = Lines[i][IndentPrefix.size()];
 const unsigned FirstCharByteSize =
 encoding::getCodePointNumBytes(FirstCommentChar, Encoding);
-return encoding::columnWidth(
-   Lines[i].substr(IndentPrefix.size(), FirstCharByteSize),
-   Encoding) == 1 &&
-   (FirstCommentChar == '\\' || isPunctuation(FirstCommentChar) ||
-isHorizontalWhitespace(FirstCommentChar));
+if (encoding::columnWidth(
+Lines[i].substr(IndentPrefix.size(), FirstCharByteSize),
+Encoding) != 1)
+  return false;
+if (FirstCommentChar == '#')
+  return false;
+return FirstCommentChar == '\\' || isPunctuation(FirstCommentChar) ||
+   isHorizontalWhitespace(FirstCommentChar);
   };
 
   // On the first line of the comment section we calculate how many spaces


Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -91,6 +91,9 @@
"// line 2\n"
"void f() {}\n");
 
+  EXPECT_EQ("// comment\n", format("//comment\n"));
+  EXPECT_EQ("// #comment\n", format("//#comment\n"));
+
   EXPECT_EQ("// comment\n"
 "// clang-format on\n",
 format("//comment\n"
Index: clang/lib/Format/BreakableToken.cpp
===
--- clang/lib/Format/BreakableToken.cpp
+++ clang/lib/Format/BreakableToken.cpp
@@ -779,11 +779,14 @@
 const char FirstCommentChar = Lines[i][IndentPrefix.size()];
 const unsigned FirstCharByteSize =
 encoding::getCodePointNumBytes(FirstCommentChar, Encoding);
-return encoding::columnWidth(
-   Lines[i].substr(IndentPrefix.size(), FirstCharByteSize),
-   Encoding) == 1 &&
-   (FirstCommentChar == '\\' || isPunctuation(FirstCommentChar) ||
-isHorizontalWhitespace(FirstCommentChar));
+if (encoding::columnWidth(
+Lines[i].substr(IndentPrefix.size(), FirstCharByteSize),
+Encoding) != 1)
+  return false;
+if (FirstCommentChar == '#')
+  return false;
+return FirstCommentChar == '\\' || isPunctuation(FirstCommentChar) ||
+   isHorizontalWhitespace(FirstCommentChar);
   };
 
   // On the first line of the comment section we calculate how many spaces
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121451: [clang-format] Add space to comments starting with '#'.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I'm not sure whether we should consider this a breaking change, there were no 
tests at all verifying this behaviour.
I think that not adding a space in comments starting with a punctuation is a 
mistake unless for some special comments like `///`, `//!` etc. (which are 
handled separately anyway).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121451

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