[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog marked an inline comment as done.
kesyog added a comment.

Thanks for the suggestions. I removed `-no-use-color` and modified `-use-color` 
to follow the behavior of LLVM's CLI parser. I also fixed the (very 
unintentional) file mode change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog updated this revision to Diff 410132.
kesyog added a comment.

Use argument style more consistent with clang-tidy & LLVM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -62,6 +62,21 @@
 import queue as queue
 
 
+def strtobool(val):
+  """Convert a string representation of truth to a bool following LLVM's CLI 
argument parsing."""
+
+  val = val.lower()
+  if val in ['', 'true', '1']:
+return True
+  elif val in ['false', '0']:
+return False
+
+  # Return ArgumentTypeError so that argparse does not substitute its own 
error message
+  raise argparse.ArgumentTypeError(
+"'{}' is invalid value for boolean argument! Try 0 or 1.".format(val)
+  )
+
+
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
@@ -82,15 +97,20 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 extra_arg, extra_arg_before, quiet, config,
-line_filter):
+line_filter, use_color):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if line_filter is not None:
 start.append('-line-filter=' + line_filter)
+  if use_color is not None:
+if use_color:
+  start.append('--use-color')
+else:
+  start.append('--use-color=false')
   if checks:
 start.append('-checks=' + checks)
   if tmpdir is not None:
@@ -168,7 +188,8 @@
  tmpdir, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 
 proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
 output, err = proc.communicate()
@@ -231,6 +252,10 @@
   'after applying fixes')
   parser.add_argument('-style', default='file', help='The style of reformat '
   'code after applying fixes')
+  parser.add_argument('-use-color', type=strtobool, nargs='?', const=True,
+  help='Use colors in diagnostics, overriding 
clang-tidy\'s'
+  ' default behavior. This option overrides the \'UseColor'
+  '\' option in .clang-tidy file, if any.')
   parser.add_argument('-p', dest='build_path',
   help='Path used to read a compile command database.')
   parser.add_argument('-extra-arg', dest='extra_arg',
@@ -258,7 +283,8 @@
  None, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 invocation.append('-list-checks')
 invocation.append('-')
 if args.quiet:


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -62,6 +62,21 @@
 import queue as queue
 
 
+def strtobool(val):
+  """Convert a string representation of truth to a bool following LLVM's CLI argument parsing."""
+
+  val = val.lower()
+  if val in ['', 'true', '1']:
+return True
+  elif val in ['false', '0']:
+return False
+
+  # Return ArgumentTypeError so that argparse does not substitute its own error message
+  raise argparse.ArgumentTypeError(
+"'{}' is invalid value for boolean argument! Try 0 or 1.".format(val)
+  )
+
+
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
@@ -82,15 +97,20 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 

[PATCH] D120200: [WIP][Clang][OpenMP] Add Sema support for atomic compare capture

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 updated this revision to Diff 410124.
tianshilei1992 added a comment.

reorg code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120200

Files:
  clang/lib/Sema/SemaOpenMP.cpp

Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -10974,6 +10974,16 @@
 NotScalar,
 /// Not an integer.
 NotInteger,
+/// Not an equality operator.
+NotEQ,
+/// Invalid assignment (not v == x).
+InvalidAssignment,
+/// Not if statement
+NotIfStmt,
+/// More than two statements in a compund statement.
+MoreThanTwoStmts,
+/// Not a compound statement.
+NotCompoundStmt,
 /// No error.
 NoError,
   };
@@ -10997,7 +11007,7 @@
   Expr *getCond() const { return C; }
   bool isXBinopExpr() const { return IsXBinopExpr; }
 
-private:
+protected:
   /// Reference to ASTContext
   ASTContext 
   /// 'x' lvalue part of the source atomic expression.
@@ -11024,6 +11034,35 @@
 
   /// Check if all captured values have right type.
   bool checkType(ErrorInfoTy ) const;
+
+  static bool CheckValue(const Expr *E, ErrorInfoTy ,
+ bool ShouldBeLValue) {
+if (ShouldBeLValue && !E->isLValue()) {
+  ErrorInfo.Error = ErrorTy::XNotLValue;
+  ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+  ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+  return false;
+}
+
+if (!E->isInstantiationDependent()) {
+  QualType QTy = E->getType();
+  if (!QTy->isScalarType()) {
+ErrorInfo.Error = ErrorTy::NotScalar;
+ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+return false;
+  }
+
+  if (!QTy->isIntegerType()) {
+ErrorInfo.Error = ErrorTy::NotInteger;
+ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+return false;
+  }
+}
+
+return true;
+  }
 };
 
 bool OpenMPAtomicCompareChecker::checkCondUpdateStmt(IfStmt *S,
@@ -11206,41 +11245,13 @@
   // 'x' and 'e' cannot be nullptr
   assert(X && E && "X and E cannot be nullptr");
 
-  auto CheckValue = [](const Expr *E, bool ShouldBeLValue) {
-if (ShouldBeLValue && !E->isLValue()) {
-  ErrorInfo.Error = ErrorTy::XNotLValue;
-  ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
-  ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
-  return false;
-}
-
-if (!E->isInstantiationDependent()) {
-  QualType QTy = E->getType();
-  if (!QTy->isScalarType()) {
-ErrorInfo.Error = ErrorTy::NotScalar;
-ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
-ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
-return false;
-  }
-
-  if (!QTy->isIntegerType()) {
-ErrorInfo.Error = ErrorTy::NotInteger;
-ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
-ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
-return false;
-  }
-}
-
-return true;
-  };
-
-  if (!CheckValue(X, true))
+  if (!CheckValue(X, ErrorInfo, true))
 return false;
 
-  if (!CheckValue(E, false))
+  if (!CheckValue(E, ErrorInfo, false))
 return false;
 
-  if (D && !CheckValue(D, false))
+  if (D && !CheckValue(D, ErrorInfo, false))
 return false;
 
   return true;
@@ -11288,6 +11299,265 @@
 
   return checkType(ErrorInfo);
 }
+
+class OpenMPAtomicCompareCaptureChecker final
+: public OpenMPAtomicCompareChecker {
+public:
+  OpenMPAtomicCompareCaptureChecker(Sema ) : OpenMPAtomicCompareChecker(S) {}
+
+  Expr *getV() const { return V; }
+  Expr *getR() const { return R; }
+  bool isFailOnly() const { return IsFailOnly; }
+
+  /// Check if statement \a S is valid for atomic compare.
+  bool checkStmt(Stmt *S, ErrorInfoTy );
+
+private:
+  bool checkType(ErrorInfoTy );
+
+  /// Check if it is valid 'if(x == e) { x = d; } else { v = x; }' (form 3)
+  bool checkForm3(IfStmt *IS, ErrorInfoTy );
+
+  /// 'v' lvalue part of the source atomic expression.
+  Expr *V = nullptr;
+  /// 'r' lvalue part of the source atomic expression.
+  Expr *R = nullptr;
+  ///
+  bool IsFailOnly = false;
+};
+
+bool OpenMPAtomicCompareCaptureChecker::checkType(ErrorInfoTy ) {
+  if (!OpenMPAtomicCompareChecker::checkType(ErrorInfo))
+return false;
+
+  assert(V && "V cannot be nullptr");
+
+  if (!CheckValue(V, ErrorInfo, true))
+return false;
+
+  if (R && !CheckValue(R, ErrorInfo, true))
+return false;
+
+  return true;
+}
+
+bool OpenMPAtomicCompareCaptureChecker::checkForm3(IfStmt *S,
+   ErrorInfoTy ) {
+  IsFailOnly = true;
+
+  auto 

[PATCH] D120200: [WIP][Clang][OpenMP] Add Sema support for atomic compare capture

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 created this revision.
tianshilei1992 added reviewers: jdoerfert, ABataev.
Herald added subscribers: guansong, yaxunl.
tianshilei1992 requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

This patch adds Sema support for `atomic compare capture`.

NOTE: Tests will be added soon.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120200

Files:
  clang/lib/Sema/SemaOpenMP.cpp

Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -10974,6 +10974,16 @@
 NotScalar,
 /// Not an integer.
 NotInteger,
+/// Not an equality operator.
+NotEQ,
+/// Invalid assignment (not v == x).
+InvalidAssignment,
+/// Not if statement
+NotIfStmt,
+/// More than two statements in a compund statement.
+MoreThanTwoStmts,
+/// Not a compound statement.
+NotCompoundStmt,
 /// No error.
 NoError,
   };
@@ -10997,7 +11007,7 @@
   Expr *getCond() const { return C; }
   bool isXBinopExpr() const { return IsXBinopExpr; }
 
-private:
+protected:
   /// Reference to ASTContext
   ASTContext 
   /// 'x' lvalue part of the source atomic expression.
@@ -11024,6 +11034,35 @@
 
   /// Check if all captured values have right type.
   bool checkType(ErrorInfoTy ) const;
+
+  static bool CheckValue(const Expr *E, ErrorInfoTy ,
+ bool ShouldBeLValue) {
+if (ShouldBeLValue && !E->isLValue()) {
+  ErrorInfo.Error = ErrorTy::XNotLValue;
+  ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+  ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+  return false;
+}
+
+if (!E->isInstantiationDependent()) {
+  QualType QTy = E->getType();
+  if (!QTy->isScalarType()) {
+ErrorInfo.Error = ErrorTy::NotScalar;
+ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+return false;
+  }
+
+  if (!QTy->isIntegerType()) {
+ErrorInfo.Error = ErrorTy::NotInteger;
+ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+return false;
+  }
+}
+
+return true;
+  }
 };
 
 bool OpenMPAtomicCompareChecker::checkCondUpdateStmt(IfStmt *S,
@@ -11206,41 +11245,13 @@
   // 'x' and 'e' cannot be nullptr
   assert(X && E && "X and E cannot be nullptr");
 
-  auto CheckValue = [](const Expr *E, bool ShouldBeLValue) {
-if (ShouldBeLValue && !E->isLValue()) {
-  ErrorInfo.Error = ErrorTy::XNotLValue;
-  ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
-  ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
-  return false;
-}
-
-if (!E->isInstantiationDependent()) {
-  QualType QTy = E->getType();
-  if (!QTy->isScalarType()) {
-ErrorInfo.Error = ErrorTy::NotScalar;
-ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
-ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
-return false;
-  }
-
-  if (!QTy->isIntegerType()) {
-ErrorInfo.Error = ErrorTy::NotInteger;
-ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
-ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
-return false;
-  }
-}
-
-return true;
-  };
-
-  if (!CheckValue(X, true))
+  if (!CheckValue(X, ErrorInfo, true))
 return false;
 
-  if (!CheckValue(E, false))
+  if (!CheckValue(E, ErrorInfo, false))
 return false;
 
-  if (D && !CheckValue(D, false))
+  if (D && !CheckValue(D, ErrorInfo, false))
 return false;
 
   return true;
@@ -11288,6 +11299,262 @@
 
   return checkType(ErrorInfo);
 }
+
+class OpenMPAtomicCompareCaptureChecker final
+: public OpenMPAtomicCompareChecker {
+public:
+  OpenMPAtomicCompareCaptureChecker(Sema ) : OpenMPAtomicCompareChecker(S) {}
+
+  Expr *getV() const { return V; }
+  Expr *getR() const { return R; }
+  bool isFailOnly() const { return IsFailOnly; }
+
+  /// Check if statement \a S is valid for atomic compare.
+  bool checkStmt(Stmt *S, ErrorInfoTy );
+
+private:
+  bool checkType(ErrorInfoTy );
+
+  /// Check if it is valid 'if(x == e) { x = d; } else { v = x; }' (form 3)
+  bool checkForm3(IfStmt *IS, ErrorInfoTy );
+
+  /// 'v' lvalue part of the source atomic expression.
+  Expr *V = nullptr;
+  /// 'r' lvalue part of the source atomic expression.
+  Expr *R = nullptr;
+  ///
+  bool IsFailOnly = false;
+};
+
+bool OpenMPAtomicCompareCaptureChecker::checkType(ErrorInfoTy ) {
+  if (!OpenMPAtomicCompareChecker::checkType(ErrorInfo))
+return false;
+
+  assert(V && "V cannot be nullptr");
+
+  if (!CheckValue(V, ErrorInfo, true))
+return false;
+
+  if (R && !CheckValue(R, ErrorInfo, true))
+return 

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Why have you removed execute perms form the scripts, can you add them back 
please
`$ chmod +x run-clang-tidy.py'`
Unless you are using windows in which case you will have to manually edit the 
diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:240-249
+  color_group = parser.add_mutually_exclusive_group()
+  color_group.add_argument('-use-color', action='store_true', dest='use_color',
+   help='Use colors in diagnostics, overriding 
clang-tidy\'s default '
+   'behavior. This option overrides the \'UseColor\' 
option in'
+   '.clang-tidy file, if any.')
+  color_group.add_argument('-no-use-color', action='store_false', 
dest='use_color',
+   help='Do not use colors in diagnostics, overriding 
clang-tidy\'s default'

All the arguments to this script mirror clang-tidy arguments, therefore I'm 
against introducing gnc style command line flags for controlling colour output. 
I'd suggest something like this which mirrors clang-tidys behaviour much more 
closely,
This requires an import
```lang=py
from distutils.util import strtobool```
Tbh it may be better writing a strtobool that is in sync with llvms command 
line bool parser
```
{ "true", "TRUE", "True", "1"` -> true }
{ "false", "FALSE", "False", "0" -> false }
{ ... -> Error }

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Luis Penagos via Phabricator via cfe-commits]

penagos marked an inline comment as done.
penagos added a comment.

In D120188#674 , 
@HazardyKnusperkeks wrote:

> Thanks for working on it.

Happy to help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120188

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

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Luis Penagos via Phabricator via cfe-commits]

penagos marked 3 inline comments as done.
penagos added inline comments.



Comment at: clang/lib/Format/BreakableToken.cpp:818-822
 const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+(!LineTok || !switchesFormatting(*LineTok)) &&
+(SpacesInPrefix != 0 ||
+ (!NoSpaceBeforeFirstCommentChar() ||
+  (FirstNonSpace == '}' && FirstLineSpaceChange != 0)));

curdeius wrote:
> curdeius wrote:
> > HazardyKnusperkeks wrote:
> > > This slowly gets hard to read. Could you maybe split it up? Either in 
> > > giving the multiple parts their own name. Or to keep the short circuiting 
> > > have something like:
> > > ```
> > > auto AllowsSpaceChange = ...;
> > > AllowsSpaceChange = AllowsSpaceChange || ...;
> > > AllowsSpaceChange = AllowsSpaceChange || ...;
> > > ```
> > Or use a lambda (with a series of ifs for instance) to initialize it.
> While here, please change `auto` to `bool`.
I've split the conditional over a couple of smaller boolean conditionals with 
the intent of enhancing readability.



Comment at: clang/unittests/Format/FormatTestComments.cpp:98
+   "// clang-format on\n",
+   getLLVMStyleWithColumns(20)));
+

HazardyKnusperkeks wrote:
> Why the limit?
This was a copy-paste artifact and wasn't intended to be included in the 
original diff; the column limit has no bearing on this specific test addition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120188

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

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Luis Penagos via Phabricator via cfe-commits]

penagos updated this revision to Diff 410115.
penagos added a comment.

- Split AllowsSpaceChange conditional over multiple variables to enhance 
readability.
- Remove unnecessary column limit from unittest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120188

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,11 @@
"// line 2\n"
"void f() {}\n");
 
+  EXPECT_EQ("// comment\n"
+"// clang-format on\n",
+format("//comment\n"
+   "// clang-format on\n"));
+
   verifyFormat("void f() {\n"
"  // Doesn't do anything\n"
"}");
Index: clang/lib/Format/BreakableToken.cpp
===
--- clang/lib/Format/BreakableToken.cpp
+++ clang/lib/Format/BreakableToken.cpp
@@ -815,10 +815,13 @@
 
 assert(Lines[i].size() > IndentPrefix.size());
 const auto FirstNonSpace = Lines[i][IndentPrefix.size()];
-const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+const bool IsFormatComment = LineTok && switchesFormatting(*LineTok);
+const bool LineRequiresLeadingSpace =
+!NoSpaceBeforeFirstCommentChar() ||
+(FirstNonSpace == '}' && FirstLineSpaceChange != 0);
+const bool AllowsSpaceChange =
+!IsFormatComment &&
+(SpacesInPrefix != 0 || LineRequiresLeadingSpace);
 
 if (PrefixSpaceChange[i] > 0 && AllowsSpaceChange) {
   Prefix[i] = IndentPrefix.str();


Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -91,6 +91,11 @@
"// line 2\n"
"void f() {}\n");
 
+  EXPECT_EQ("// comment\n"
+"// clang-format on\n",
+format("//comment\n"
+   "// clang-format on\n"));
+
   verifyFormat("void f() {\n"
"  // Doesn't do anything\n"
"}");
Index: clang/lib/Format/BreakableToken.cpp
===
--- clang/lib/Format/BreakableToken.cpp
+++ clang/lib/Format/BreakableToken.cpp
@@ -815,10 +815,13 @@
 
 assert(Lines[i].size() > IndentPrefix.size());
 const auto FirstNonSpace = Lines[i][IndentPrefix.size()];
-const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+const bool IsFormatComment = LineTok && switchesFormatting(*LineTok);
+const bool LineRequiresLeadingSpace =
+!NoSpaceBeforeFirstCommentChar() ||
+(FirstNonSpace == '}' && FirstLineSpaceChange != 0);
+const bool AllowsSpaceChange =
+!IsFormatComment &&
+(SpacesInPrefix != 0 || LineRequiresLeadingSpace);
 
 if (PrefixSpaceChange[i] > 0 && AllowsSpaceChange) {
   Prefix[i] = IndentPrefix.str();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp:100
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}

SimplyDanny wrote:
> SimplyDanny wrote:
> > njames93 wrote:
> > > This seems like a shortcoming in llvms YAML parser. Isn't the fold 
> > > character `>` supposed to replace newlines with spaces and strip trailing 
> > > ws
> > > ```lang=c++
> > > "-*,misc-* llvm-* -clang-* google-*"
> > > ```
> > > Using the pipe `|` instead should yield the output you are currently 
> > > expecting,
> > That's true. `>` should actually not work without a comma after each check.
> `YAMLParser.h` mentions that "Multi-line literal folding" is not yet 
> implemented.
So it is, D102590 has been up addressing this very issue (Though its rather 
stale ATM).
In any case for now we shouldn't depend on the broken behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120187

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

[PATCH] D120070: [HIP] Support linking archive of bundled bitcode

[Yaxun Liu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfa0f90bc55ed: [HIP] Support linking archive of bundled 
bitcode (authored by yaxunl).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120070

Files:
  clang/lib/Driver/ToolChains/HIPAMD.cpp
  clang/test/Driver/clang-offload-bundler.c
  clang/test/Driver/hip-link-bundle-archive.hip


Index: clang/test/Driver/hip-link-bundle-archive.hip
===
--- /dev/null
+++ clang/test/Driver/hip-link-bundle-archive.hip
@@ -0,0 +1,14 @@
+// REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
+
+// RUN: touch %T/libhipBundled.a
+
+// Check clang unbundle the archive and link them by lld.
+
+// RUN: %clang -### --offload-arch=gfx906 --offload-arch=gfx1030 \
+// RUN:   -nogpulib %s -fgpu-rdc -L%T -lhipBundled \
+// RUN:   2>&1 | FileCheck -check-prefix=CHECK %s
+
+// CHECK: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a" 
"-inputs={{.*}}libhipBundled.a" "-targets=hip-amdgcn-amd-amdhsa-gfx1030" 
"-outputs=[[A1030:.*\.a]]" "-allow-missing-bundles"
+// CHECK: "{{.*}}lld" {{.*}}"-plugin-opt=mcpu=gfx1030" {{.*}} "[[A1030]]"
+// CHECK: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a" 
"-inputs={{.*}}libhipBundled.a" "-targets=hip-amdgcn-amd-amdhsa-gfx906" 
"-outputs=[[A906:.*\.a]]" "-allow-missing-bundles"
+// CHECK: "{{.*}}lld" {{.*}}"-plugin-opt=mcpu=gfx906" {{.*}} "[[A906]]"
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -365,6 +365,28 @@
 // CKLST2-NOT: openmp-powerpc64le-ibm-linux-gnu
 // CKLST2-NOT: openmp-x86_64-pc-linux-gnu
 
+//
+// Check unbundling archive for HIP.
+//
+// When the input to clang-offload-bundler is an archive of bundled bitcodes,
+// for each target, clang-offload-bundler extracts the bitcode from each
+// bundle and archives them. Therefore for each target, the output is an
+// archive of unbundled bitcodes.
+//
+// RUN: clang-offload-bundler -type=bc 
-targets=hip-amdgcn-amd-amdhsa--gfx900,hip-amdgcn-amd-amdhsa--gfx906 \
+// RUN:   -inputs=%t.tgt1,%t.tgt2 -outputs=%T/hip_bundle1.bc
+// RUN: clang-offload-bundler -type=bc 
-targets=hip-amdgcn-amd-amdhsa--gfx900,hip-amdgcn-amd-amdhsa--gfx906 \
+// RUN:   -inputs=%t.tgt1,%t.tgt2 -outputs=%T/hip_bundle2.bc
+// RUN: llvm-ar cr %T/hip_archive.a %T/hip_bundle1.bc %T/hip_bundle2.bc
+// RUN: clang-offload-bundler -unbundle -type=a 
-targets=hip-amdgcn-amd-amdhsa--gfx900,hip-amdgcn-amd-amdhsa--gfx906 \
+// RUN:   -outputs=%T/hip_900.a,%T/hip_906.a -inputs=%T/hip_archive.a
+// RUN: llvm-ar t %T/hip_900.a | FileCheck -check-prefix=HIP-AR-900 %s
+// RUN: llvm-ar t %T/hip_906.a | FileCheck -check-prefix=HIP-AR-906 %s
+// HIP-AR-900-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx900
+// HIP-AR-900-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx900
+// HIP-AR-906-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx906
+// HIP-AR-906-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx906
+
 //
 // Check bundling without host target is allowed for HIP.
 //
Index: clang/lib/Driver/ToolChains/HIPAMD.cpp
===
--- clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -121,6 +121,14 @@
   for (auto Input : Inputs)
 LldArgs.push_back(Input.getFilename());
 
+  // Look for archive of bundled bitcode in arguments, and add temporary files
+  // for the extracted archive of bitcode to inputs.
+  auto TargetID = Args.getLastArgValue(options::OPT_mcpu_EQ);
+  AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, LldArgs, "amdgcn",
+ TargetID,
+ /*IsBitCodeSDL=*/true,
+ /*PostClangLink=*/false);
+
   const char *Lld = Args.MakeArgString(getToolChain().GetProgramPath("lld"));
   C.addCommand(std::make_unique(JA, *this, 
ResponseFileSupport::None(),
  Lld, LldArgs, Inputs, Output));


Index: clang/test/Driver/hip-link-bundle-archive.hip
===
--- /dev/null
+++ clang/test/Driver/hip-link-bundle-archive.hip
@@ -0,0 +1,14 @@
+// REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
+
+// RUN: touch %T/libhipBundled.a
+
+// Check clang unbundle the archive and link them by lld.
+
+// RUN: %clang -### --offload-arch=gfx906 --offload-arch=gfx1030 \
+// RUN:   -nogpulib %s -fgpu-rdc -L%T -lhipBundled \
+// RUN:   2>&1 | FileCheck -check-prefix=CHECK %s
+
+// CHECK: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a" "-inputs={{.*}}libhipBundled.a" "-targets=hip-amdgcn-amd-amdhsa-gfx1030" 

[clang] fa0f90b - [HIP] Support linking archive of bundled bitcode

[Yaxun Liu via cfe-commits]

Author: Yaxun (Sam) Liu
Date: 2022-02-19T18:37:44-05:00
New Revision: fa0f90bc55ed536e1488648255278ce9029cfa59

URL: 
https://github.com/llvm/llvm-project/commit/fa0f90bc55ed536e1488648255278ce9029cfa59
DIFF: 
https://github.com/llvm/llvm-project/commit/fa0f90bc55ed536e1488648255278ce9029cfa59.diff

LOG: [HIP] Support linking archive of bundled bitcode

HIP programs compiled with -c -fgpu-rdc generate clang-offload-bundler
bundles which contain bitcode for different GPU's.

Such files can be archived to an archive file which can be linked with
HIP programs with -fgpu-rdc.

This patch adds suppor of linking archive of bundled bitcode.

When an archive of bundled bitcode is passed to clang by -l, for each
GPU specified through --offload-arch, clang extracts bitcode from
the archive and creates a new archive for that GPU and pass it
to lld.

Reviewed by: Artem Belevich

Differential Revision: https://reviews.llvm.org/D120070

Fixes: SWDEV-321741, SWDEV-315773

Added: 
clang/test/Driver/hip-link-bundle-archive.hip

Modified: 
clang/lib/Driver/ToolChains/HIPAMD.cpp
clang/test/Driver/clang-offload-bundler.c

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp 
b/clang/lib/Driver/ToolChains/HIPAMD.cpp
index 6d553791b394..4a952530993e 100644
--- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -121,6 +121,14 @@ void AMDGCN::Linker::constructLldCommand(Compilation , 
const JobAction ,
   for (auto Input : Inputs)
 LldArgs.push_back(Input.getFilename());
 
+  // Look for archive of bundled bitcode in arguments, and add temporary files
+  // for the extracted archive of bitcode to inputs.
+  auto TargetID = Args.getLastArgValue(options::OPT_mcpu_EQ);
+  AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, LldArgs, "amdgcn",
+ TargetID,
+ /*IsBitCodeSDL=*/true,
+ /*PostClangLink=*/false);
+
   const char *Lld = Args.MakeArgString(getToolChain().GetProgramPath("lld"));
   C.addCommand(std::make_unique(JA, *this, 
ResponseFileSupport::None(),
  Lld, LldArgs, Inputs, Output));

diff  --git a/clang/test/Driver/clang-offload-bundler.c 
b/clang/test/Driver/clang-offload-bundler.c
index a307af4473a1..eab4dbc7e3be 100644
--- a/clang/test/Driver/clang-offload-bundler.c
+++ b/clang/test/Driver/clang-offload-bundler.c
@@ -365,6 +365,28 @@
 // CKLST2-NOT: openmp-powerpc64le-ibm-linux-gnu
 // CKLST2-NOT: openmp-x86_64-pc-linux-gnu
 
+//
+// Check unbundling archive for HIP.
+//
+// When the input to clang-offload-bundler is an archive of bundled bitcodes,
+// for each target, clang-offload-bundler extracts the bitcode from each
+// bundle and archives them. Therefore for each target, the output is an
+// archive of unbundled bitcodes.
+//
+// RUN: clang-offload-bundler -type=bc 
-targets=hip-amdgcn-amd-amdhsa--gfx900,hip-amdgcn-amd-amdhsa--gfx906 \
+// RUN:   -inputs=%t.tgt1,%t.tgt2 -outputs=%T/hip_bundle1.bc
+// RUN: clang-offload-bundler -type=bc 
-targets=hip-amdgcn-amd-amdhsa--gfx900,hip-amdgcn-amd-amdhsa--gfx906 \
+// RUN:   -inputs=%t.tgt1,%t.tgt2 -outputs=%T/hip_bundle2.bc
+// RUN: llvm-ar cr %T/hip_archive.a %T/hip_bundle1.bc %T/hip_bundle2.bc
+// RUN: clang-offload-bundler -unbundle -type=a 
-targets=hip-amdgcn-amd-amdhsa--gfx900,hip-amdgcn-amd-amdhsa--gfx906 \
+// RUN:   -outputs=%T/hip_900.a,%T/hip_906.a -inputs=%T/hip_archive.a
+// RUN: llvm-ar t %T/hip_900.a | FileCheck -check-prefix=HIP-AR-900 %s
+// RUN: llvm-ar t %T/hip_906.a | FileCheck -check-prefix=HIP-AR-906 %s
+// HIP-AR-900-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx900
+// HIP-AR-900-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx900
+// HIP-AR-906-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx906
+// HIP-AR-906-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx906
+
 //
 // Check bundling without host target is allowed for HIP.
 //

diff  --git a/clang/test/Driver/hip-link-bundle-archive.hip 
b/clang/test/Driver/hip-link-bundle-archive.hip
new file mode 100644
index ..4b97844faf46
--- /dev/null
+++ b/clang/test/Driver/hip-link-bundle-archive.hip
@@ -0,0 +1,14 @@
+// REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
+
+// RUN: touch %T/libhipBundled.a
+
+// Check clang unbundle the archive and link them by lld.
+
+// RUN: %clang -### --offload-arch=gfx906 --offload-arch=gfx1030 \
+// RUN:   -nogpulib %s -fgpu-rdc -L%T -lhipBundled \
+// RUN:   2>&1 | FileCheck -check-prefix=CHECK %s
+
+// CHECK: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a" 
"-inputs={{.*}}libhipBundled.a" "-targets=hip-amdgcn-amd-amdhsa-gfx1030" 
"-outputs=[[A1030:.*\.a]]" "-allow-missing-bundles"
+// CHECK: "{{.*}}lld" {{.*}}"-plugin-opt=mcpu=gfx1030" {{.*}} "[[A1030]]"
+// CHECK: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a" 
"-inputs={{.*}}libhipBundled.a" 

[PATCH] D120196: [clang-tidy][NFC] Remove Tristate from CachedGlobList

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: alexfh, aaron.ballman, whisperity, salman-javed-nz.
Herald added subscribers: rnkovacs, xazax.hun.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The tristate is a little redundant as we can determine if the item was already 
in the cache based on the return from try_emplace.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120196

Files:
  clang-tools-extra/clang-tidy/GlobList.cpp
  clang-tools-extra/clang-tidy/GlobList.h


Index: clang-tools-extra/clang-tidy/GlobList.h
===
--- clang-tools-extra/clang-tidy/GlobList.h
+++ clang-tools-extra/clang-tidy/GlobList.h
@@ -59,8 +59,7 @@
   bool contains(StringRef S) const override;
 
 private:
-  enum Tristate { None, Yes, No };
-  mutable llvm::StringMap Cache;
+  mutable llvm::StringMap Cache;
 };
 
 } // namespace tidy
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===
--- clang-tools-extra/clang-tidy/GlobList.cpp
+++ clang-tools-extra/clang-tidy/GlobList.cpp
@@ -65,16 +65,12 @@
 }
 
 bool CachedGlobList::contains(StringRef S) const {
-  switch (auto  = Cache[S]) {
-  case Yes:
-return true;
-  case No:
-return false;
-  case None:
-Result = GlobList::contains(S) ? Yes : No;
-return Result == Yes;
-  }
-  llvm_unreachable("invalid enum");
+  auto Entry = Cache.try_emplace(S);
+  bool  = Entry.first->getValue();
+  // If the entry was just inserted, determine its required value.
+  if (Entry.second)
+Value = GlobList::contains(S);
+  return Value;
 }
 
 } // namespace tidy


Index: clang-tools-extra/clang-tidy/GlobList.h
===
--- clang-tools-extra/clang-tidy/GlobList.h
+++ clang-tools-extra/clang-tidy/GlobList.h
@@ -59,8 +59,7 @@
   bool contains(StringRef S) const override;
 
 private:
-  enum Tristate { None, Yes, No };
-  mutable llvm::StringMap Cache;
+  mutable llvm::StringMap Cache;
 };
 
 } // namespace tidy
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===
--- clang-tools-extra/clang-tidy/GlobList.cpp
+++ clang-tools-extra/clang-tidy/GlobList.cpp
@@ -65,16 +65,12 @@
 }
 
 bool CachedGlobList::contains(StringRef S) const {
-  switch (auto  = Cache[S]) {
-  case Yes:
-return true;
-  case No:
-return false;
-  case None:
-Result = GlobList::contains(S) ? Yes : No;
-return Result == Yes;
-  }
-  llvm_unreachable("invalid enum");
+  auto Entry = Cache.try_emplace(S);
+  bool  = Entry.first->getValue();
+  // If the entry was just inserted, determine its required value.
+  if (Entry.second)
+Value = GlobList::contains(S);
+  return Value;
 }
 
 } // namespace tidy
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Was there any real use case for making this polymorphic, The overhead for a 
virtual function call seems a little unnecessary IMO?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422

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

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/BreakableToken.cpp:818
 const auto FirstNonSpace = Lines[i][IndentPrefix.size()];
 const auto AllowsSpaceChange =
+(!LineTok || !switchesFormatting(*LineTok)) &&

curdeius wrote:
> HazardyKnusperkeks wrote:
> > This slowly gets hard to read. Could you maybe split it up? Either in 
> > giving the multiple parts their own name. Or to keep the short circuiting 
> > have something like:
> > ```
> > auto AllowsSpaceChange = ...;
> > AllowsSpaceChange = AllowsSpaceChange || ...;
> > AllowsSpaceChange = AllowsSpaceChange || ...;
> > ```
> Or use a lambda (with a series of ifs for instance) to initialize it.
While here, please change `auto` to `bool`.



Comment at: clang/lib/Format/BreakableToken.cpp:818-822
 const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+(!LineTok || !switchesFormatting(*LineTok)) &&
+(SpacesInPrefix != 0 ||
+ (!NoSpaceBeforeFirstCommentChar() ||
+  (FirstNonSpace == '}' && FirstLineSpaceChange != 0)));

HazardyKnusperkeks wrote:
> This slowly gets hard to read. Could you maybe split it up? Either in giving 
> the multiple parts their own name. Or to keep the short circuiting have 
> something like:
> ```
> auto AllowsSpaceChange = ...;
> AllowsSpaceChange = AllowsSpaceChange || ...;
> AllowsSpaceChange = AllowsSpaceChange || ...;
> ```
Or use a lambda (with a series of ifs for instance) to initialize it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120188

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

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[Danny Mösch via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp:100
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}

SimplyDanny wrote:
> njames93 wrote:
> > This seems like a shortcoming in llvms YAML parser. Isn't the fold 
> > character `>` supposed to replace newlines with spaces and strip trailing ws
> > ```lang=c++
> > "-*,misc-* llvm-* -clang-* google-*"
> > ```
> > Using the pipe `|` instead should yield the output you are currently 
> > expecting,
> That's true. `>` should actually not work without a comma after each check.
`YAMLParser.h` mentions that "Multi-line literal folding" is not yet 
implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120187

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

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[Danny Mösch via Phabricator via cfe-commits]

SimplyDanny marked 2 inline comments as done.
SimplyDanny added inline comments.



Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp:100
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}

njames93 wrote:
> This seems like a shortcoming in llvms YAML parser. Isn't the fold character 
> `>` supposed to replace newlines with spaces and strip trailing ws
> ```lang=c++
> "-*,misc-* llvm-* -clang-* google-*"
> ```
> Using the pipe `|` instead should yield the output you are currently 
> expecting,
That's true. `>` should actually not work without a comma after each check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120187

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

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[Danny Mösch via Phabricator via cfe-commits]

SimplyDanny updated this revision to Diff 410102.
SimplyDanny added a comment.

Update according to comments in review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120187

Files:
  clang-tools-extra/clang-tidy/GlobList.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
@@ -104,5 +104,18 @@
   EXPECT_TRUE(Filter.contains("asdfqwEasdf"));
 }
 
+TYPED_TEST(GlobListTest, NewlineCharactersAsSeparators) {
+  TypeParam Filter("a*  \n b,\n-c*,dd");
+
+  EXPECT_FALSE(Filter.contains(""));
+  EXPECT_TRUE(Filter.contains("aaa"));
+  EXPECT_TRUE(Filter.contains("b"));
+  EXPECT_FALSE(Filter.contains("c"));
+  EXPECT_FALSE(Filter.contains("ccc"));
+  EXPECT_FALSE(Filter.contains("d"));
+  EXPECT_TRUE(Filter.contains("dd"));
+  EXPECT_FALSE(Filter.contains("ddd"));
+}
+
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -86,6 +86,20 @@
   EXPECT_EQ("some.user", *Options->User);
 }
 
+TEST(ParseConfiguration, ChecksSeparatedByNewlines) {
+  auto MemoryBuffer = llvm::MemoryBufferRef("Checks: |\n"
+"  -*,misc-*\n"
+"  llvm-*\n"
+"  -clang-*,\n"
+"  google-*",
+"Options");
+
+  auto Options = parseConfiguration(MemoryBuffer);
+
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}
+
 TEST(ParseConfiguration, MergeConfigurations) {
   llvm::ErrorOr Options1 =
   parseConfiguration(llvm::MemoryBufferRef(R"(
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===
--- clang-tools-extra/clang-tidy/GlobList.cpp
+++ clang-tools-extra/clang-tidy/GlobList.cpp
@@ -27,7 +27,7 @@
 // Converts first glob from the comma-separated list of globs to Regex and
 // removes it and the trailing comma from the GlobList.
 static llvm::Regex consumeGlob(StringRef ) {
-  StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find(','));
+  StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find_first_of(",\n"));
   StringRef Glob = UntrimmedGlob.trim();
   GlobList = GlobList.substr(UntrimmedGlob.size() + 1);
   SmallString<128> RegexText("^");
@@ -44,7 +44,7 @@
 }
 
 GlobList::GlobList(StringRef Globs, bool KeepNegativeGlobs /* =true */) {
-  Items.reserve(Globs.count(',') + 1);
+  Items.reserve(Globs.count(',') + Globs.count('\n') + 1);
   do {
 GlobListItem Item;
 Item.IsPositive = !consumeNegativeIndicator(Globs);


Index: clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
@@ -104,5 +104,18 @@
   EXPECT_TRUE(Filter.contains("asdfqwEasdf"));
 }
 
+TYPED_TEST(GlobListTest, NewlineCharactersAsSeparators) {
+  TypeParam Filter("a*  \n b,\n-c*,dd");
+
+  EXPECT_FALSE(Filter.contains(""));
+  EXPECT_TRUE(Filter.contains("aaa"));
+  EXPECT_TRUE(Filter.contains("b"));
+  EXPECT_FALSE(Filter.contains("c"));
+  EXPECT_FALSE(Filter.contains("ccc"));
+  EXPECT_FALSE(Filter.contains("d"));
+  EXPECT_TRUE(Filter.contains("dd"));
+  EXPECT_FALSE(Filter.contains("ddd"));
+}
+
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -86,6 +86,20 @@
   EXPECT_EQ("some.user", *Options->User);
 }
 
+TEST(ParseConfiguration, ChecksSeparatedByNewlines) {
+  auto MemoryBuffer = llvm::MemoryBufferRef("Checks: |\n"
+"  -*,misc-*\n"
+"  llvm-*\n"
+"  -clang-*,\n"
+"  google-*",
+"Options");
+
+  auto Options = parseConfiguration(MemoryBuffer);
+
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}
+
 TEST(ParseConfiguration, 

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

Thanks for working on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120188

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

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/BreakableToken.cpp:818-822
 const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+(!LineTok || !switchesFormatting(*LineTok)) &&
+(SpacesInPrefix != 0 ||
+ (!NoSpaceBeforeFirstCommentChar() ||
+  (FirstNonSpace == '}' && FirstLineSpaceChange != 0)));

This slowly gets hard to read. Could you maybe split it up? Either in giving 
the multiple parts their own name. Or to keep the short circuiting have 
something like:
```
auto AllowsSpaceChange = ...;
AllowsSpaceChange = AllowsSpaceChange || ...;
AllowsSpaceChange = AllowsSpaceChange || ...;
```



Comment at: clang/unittests/Format/FormatTestComments.cpp:98
+   "// clang-format on\n",
+   getLLVMStyleWithColumns(20)));
+

Why the limit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120188

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

[PATCH] D117973: [cmake] Support custom package install paths

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

Could one of you review this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117973

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

[PATCH] D117977: [cmake] Don't export `LLVM_TOOLS_INSTALL_DIR` anymore

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

Could one of you review this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117977

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

[PATCH] D120188: Fix extraneous whitespace addition in line comments on clang-format directives

[Luis Penagos via Phabricator via cfe-commits]

penagos created this revision.
penagos requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120188

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,12 @@
"// line 2\n"
"void f() {}\n");
 
+  EXPECT_EQ("// comment\n"
+"// clang-format on\n",
+format("//comment\n"
+   "// clang-format on\n",
+   getLLVMStyleWithColumns(20)));
+
   verifyFormat("void f() {\n"
"  // Doesn't do anything\n"
"}");
Index: clang/lib/Format/BreakableToken.cpp
===
--- clang/lib/Format/BreakableToken.cpp
+++ clang/lib/Format/BreakableToken.cpp
@@ -816,9 +816,10 @@
 assert(Lines[i].size() > IndentPrefix.size());
 const auto FirstNonSpace = Lines[i][IndentPrefix.size()];
 const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+(!LineTok || !switchesFormatting(*LineTok)) &&
+(SpacesInPrefix != 0 ||
+ (!NoSpaceBeforeFirstCommentChar() ||
+  (FirstNonSpace == '}' && FirstLineSpaceChange != 0)));
 
 if (PrefixSpaceChange[i] > 0 && AllowsSpaceChange) {
   Prefix[i] = IndentPrefix.str();


Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -91,6 +91,12 @@
"// line 2\n"
"void f() {}\n");
 
+  EXPECT_EQ("// comment\n"
+"// clang-format on\n",
+format("//comment\n"
+   "// clang-format on\n",
+   getLLVMStyleWithColumns(20)));
+
   verifyFormat("void f() {\n"
"  // Doesn't do anything\n"
"}");
Index: clang/lib/Format/BreakableToken.cpp
===
--- clang/lib/Format/BreakableToken.cpp
+++ clang/lib/Format/BreakableToken.cpp
@@ -816,9 +816,10 @@
 assert(Lines[i].size() > IndentPrefix.size());
 const auto FirstNonSpace = Lines[i][IndentPrefix.size()];
 const auto AllowsSpaceChange =
-SpacesInPrefix != 0 ||
-(!NoSpaceBeforeFirstCommentChar() ||
- (FirstNonSpace == '}' && FirstLineSpaceChange != 0));
+(!LineTok || !switchesFormatting(*LineTok)) &&
+(SpacesInPrefix != 0 ||
+ (!NoSpaceBeforeFirstCommentChar() ||
+  (FirstNonSpace == '}' && FirstLineSpaceChange != 0)));
 
 if (PrefixSpaceChange[i] > 0 && AllowsSpaceChange) {
   Prefix[i] = IndentPrefix.str();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I can also confirm that `main` and my branch (containing 
https://reviews.llvm.org/D119477 and https://reviews.llvm.org/D119476) both 
assert on the same line:

  $ cat bug-53244.cc 
  struct A {   
  consteval A() {}
  A(const A&) = delete;
  consteval void f() {}
  };
  
  int main() {
  A{A{}}.f();
  }
  
  $ bin/clang -std=c++20 ../../bug-53244.cc
  clang-14: /home/kimgr/code/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1453: 
clang::ExprResult clang::Sema::BuildCXXTypeConstructExpr(clang::TypeSourceInfo 
*, clang::SourceLocation, clang::MultiExprArg, clang::SourceLocation, bool): 
Assertion `(!ListInitialization || (Exprs.size() == 1 && 
isa(Exprs[0]))) && "List initialization must have initializer 
list as expression."' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.  Program arguments: 
/home/kimgr/code/llvm-project/out/main/bin/clang-14 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations 
-disable-free -clear-ast-before-backend -main-file-name bug-53244.cc 
-mrelocation-model static -mframe-pointer=all -fmath-errno -ffp-contract=on 
-fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 
-tune-cpu generic -mllvm -treat-scalable-fixed-error-as-warning 
-debugger-tuning=gdb 
-fcoverage-compilation-dir=/home/kimgr/code/llvm-project/out/main -resource-dir 
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/x86_64-linux-gnu/c++/9 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/backward 
-internal-isystem 
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0/include 
-internal-isystem /usr/local/include -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../x86_64-linux-gnu/include 
-internal-externc-isystem /usr/include/x86_64-linux-gnu 
-internal-externc-isystem /include -internal-externc-isystem /usr/include 
-std=c++20 -fdeprecated-macro 
-fdebug-compilation-dir=/home/kimgr/code/llvm-project/out/main -ferror-limit 19 
-fgnuc-version=4.2.1 -fno-implicit-modules -fcxx-exceptions -fexceptions 
-fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o 
/tmp/bug-53244-192e65.o -x c++ ../../bug-53244.cc
  1.   parser at end of file
  2.  ../../bug-53244.cc:7:12: parsing function body 'main'
   #0 0x0698e43a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:11
   #1 0x0698e5eb PrintStackTraceSignalHandler(void*) 
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
   #2 0x0698cc9a llvm::sys::RunSignalHandlers() 
/home/kimgr/code/llvm-project/llvm/lib/Support/Signals.cpp:97:5
   #3 0x0698ed15 SignalHandler(int) 
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
   #4 0x7f8769c493c0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x153c0)
   #5 0x7f87696dd18b raise 
/build/glibc-eX1tMB/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
   #6 0x7f87696bc859 abort 
/build/glibc-eX1tMB/glibc-2.31/stdlib/abort.c:81:7
   #7 0x7f87696bc729 get_sysdep_segment_value 
/build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:509:8
   #8 0x7f87696bc729 _nl_load_domain 
/build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:970:34
   #9 0x7f87696cdf36 (/lib/x86_64-linux-gnu/libc.so.6+0x36f36)
  #10 0x0af7d719 
clang::Sema::BuildCXXTypeConstructExpr(clang::TypeSourceInfo*, 
clang::SourceLocation, llvm::MutableArrayRef, 
clang::SourceLocation, bool) 
/home/kimgr/code/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1454:39
  #11 0x0ade829d 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::RebuildCXXFunctionalCastExpr(clang::TypeSourceInfo*, 
clang::SourceLocation, clang::Expr*, clang::SourceLocation, bool) 
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:3019:22
  #12 0x0adac3d5 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::TransformCXXFunctionalCastExpr(clang::CXXFunctionalCastExpr*)
 /home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:11728:23
  #13 0x0ada4a8f 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::TransformExpr(clang::Expr*) 
/home/kimgr/code/llvm-project/out/main/tools/clang/include/clang/AST/StmtNodes.inc:929:1
  #14 0x0adcd51c 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 
>*>)::ComplexRemove>::TransformExprs(clang::Expr* const*, unsigned int, bool, 
llvm::SmallVectorImpl&, bool*) 
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:4047:29
  #15 0x0adae285 
clang::TreeTransform, 
llvm::PointerIntPairInfo > 

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Thank you so much Carlos, I hope they can come to see the value of having first 
class support for this in LLVM/Clang.  I really appreciate you pushing for this,

-Chris


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

https://reviews.llvm.org/D112730

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

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: .clang-tidy:1-10
+Checks: |
+  -*
+  clang-diagnostic-*
+  llvm-*
+  misc-*
+  -misc-unused-parameters
+  -misc-non-private-member-variables-in-classes

This change doesn't belong in this patch.



Comment at: clang-tools-extra/clang-tidy/GlobList.cpp:30-31
 static llvm::Regex consumeGlob(StringRef ) {
-  StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find(','));
+  auto NextSplitIndex = std::min(GlobList.find(','), GlobList.find('\n'));
+  StringRef UntrimmedGlob = GlobList.substr(0, NextSplitIndex);
   StringRef Glob = UntrimmedGlob.trim();





Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp:100
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}

This seems like a shortcoming in llvms YAML parser. Isn't the fold character 
`>` supposed to replace newlines with spaces and strip trailing ws
```lang=c++
"-*,misc-* llvm-* -clang-* google-*"
```
Using the pipe `|` instead should yield the output you are currently expecting,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120187

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

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[Danny Mösch via Phabricator via cfe-commits]

SimplyDanny created this revision.
SimplyDanny added a reviewer: alexfh.
Herald added subscribers: carlosgalvezp, xazax.hun.
SimplyDanny requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added a project: clang-tools-extra.

This is a fix for #53737. In addition to commas, newline characters are 
considered as separators of checks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120187

Files:
  .clang-tidy
  clang-tools-extra/clang-tidy/GlobList.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
@@ -104,5 +104,18 @@
   EXPECT_TRUE(Filter.contains("asdfqwEasdf"));
 }
 
+TYPED_TEST(GlobListTest, NewlineCharactersAsSeparators) {
+  TypeParam Filter("a*  \n b,\n-c*,dd");
+
+  EXPECT_FALSE(Filter.contains(""));
+  EXPECT_TRUE(Filter.contains("aaa"));
+  EXPECT_TRUE(Filter.contains("b"));
+  EXPECT_FALSE(Filter.contains("c"));
+  EXPECT_FALSE(Filter.contains("ccc"));
+  EXPECT_FALSE(Filter.contains("d"));
+  EXPECT_TRUE(Filter.contains("dd"));
+  EXPECT_FALSE(Filter.contains("ddd"));
+}
+
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -86,6 +86,20 @@
   EXPECT_EQ("some.user", *Options->User);
 }
 
+TEST(ParseConfiguration, ChecksSeparatedByNewlines) {
+  auto MemoryBuffer = llvm::MemoryBufferRef("Checks: >\n"
+"  -*,misc-*\n"
+"  llvm-*\n"
+"  -clang-*,\n"
+"  google-*",
+"Options");
+
+  auto Options = parseConfiguration(MemoryBuffer);
+
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}
+
 TEST(ParseConfiguration, MergeConfigurations) {
   llvm::ErrorOr Options1 =
   parseConfiguration(llvm::MemoryBufferRef(R"(
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===
--- clang-tools-extra/clang-tidy/GlobList.cpp
+++ clang-tools-extra/clang-tidy/GlobList.cpp
@@ -27,7 +27,8 @@
 // Converts first glob from the comma-separated list of globs to Regex and
 // removes it and the trailing comma from the GlobList.
 static llvm::Regex consumeGlob(StringRef ) {
-  StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find(','));
+  auto NextSplitIndex = std::min(GlobList.find(','), GlobList.find('\n'));
+  StringRef UntrimmedGlob = GlobList.substr(0, NextSplitIndex);
   StringRef Glob = UntrimmedGlob.trim();
   GlobList = GlobList.substr(UntrimmedGlob.size() + 1);
   SmallString<128> RegexText("^");
@@ -44,7 +45,7 @@
 }
 
 GlobList::GlobList(StringRef Globs, bool KeepNegativeGlobs /* =true */) {
-  Items.reserve(Globs.count(',') + 1);
+  Items.reserve(Globs.count(',') + Globs.count('\n') + 1);
   do {
 GlobListItem Item;
 Item.IsPositive = !consumeNegativeIndicator(Globs);
Index: .clang-tidy
===
--- .clang-tidy
+++ .clang-tidy
@@ -1,4 +1,13 @@
-Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,readability-identifier-naming'
+Checks: |
+  -*
+  clang-diagnostic-*
+  llvm-*
+  misc-*
+  -misc-unused-parameters
+  -misc-non-private-member-variables-in-classes
+  -misc-no-recursion
+  readability-identifier-naming
+
 CheckOptions:
   - key: readability-identifier-naming.ClassCase
 value:   CamelCase


Index: clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
@@ -104,5 +104,18 @@
   EXPECT_TRUE(Filter.contains("asdfqwEasdf"));
 }
 
+TYPED_TEST(GlobListTest, NewlineCharactersAsSeparators) {
+  TypeParam Filter("a*  \n b,\n-c*,dd");
+
+  EXPECT_FALSE(Filter.contains(""));
+  EXPECT_TRUE(Filter.contains("aaa"));
+  EXPECT_TRUE(Filter.contains("b"));
+  EXPECT_FALSE(Filter.contains("c"));
+  EXPECT_FALSE(Filter.contains("ccc"));
+  EXPECT_FALSE(Filter.contains("d"));
+  EXPECT_TRUE(Filter.contains("dd"));
+  EXPECT_FALSE(Filter.contains("ddd"));
+}
+
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

[Itay Bookstein via Phabricator via cfe-commits]

ibookstein added a comment.

I now realize that the type check isn't correct for the platforms which pass 
arguments to the resolver. Unfortunate that the glibc wiki doesn't mention this 
(as far as I can tell)...
I thought that the bitcast-to-"expected"-type should shield from that error, 
but maybe something drops the bitcast along the way. That reminded me of 
https://github.com/llvm/llvm-project/blob/f6ee45e94391ef8cee67e2a4ad6d61c614985de9/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L388-L391.
In addition, it sounds to me that the resolver type check will be made 
redundant by the opaque pointer work, so maybe it makes sense to remove it 
altogether now? I'm not in the details enough with respect to the migration 
plan to know.
Also, I recall there are some outstanding issues with respect to thinlto+ifunc: 
https://reviews.llvm.org/D82745 which may be of interest to your use-case as 
well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112349

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

[PATCH] D119927: [Clang] [P2025] More exhaustive tests for NRVO

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/test/CodeGenCXX/nrvo.cpp:1537
+  }
+  return x; // FIXME: NRVO could happen if B == false, but doesn't
+}

Quuxplusone wrote:
> Nit: `s/if/when/`
Serendipitously, I just ran into almost this exact scenario in D120180, where 
C++20's `reverse_iterator` wants to do basically
```
{
  _Iter __tmp = current;
  --__tmp;
  if constexpr (is_pointer_v<_Iter>) {
return __tmp;
  } else {
return std::move(__tmp).operator->();
  }
}
```
and so we want NRVO on `__tmp` in the former case but URVO in the latter case. 
Of course in that specific case, we "want NRVO" only when `__tmp` is a pointer 
and thus NRVO doesn't apply anyway because pointers are returned in registers. 
But it would be nice to have a test case for as-close-as-possible to that 
pattern, if you don't mind adding one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119927

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

[PATCH] D119829: [Driver] Support Solaris/amd64 GetTls

[Rainer Orth via Phabricator via cfe-commits]

ro updated this revision to Diff 410083.
ro added a comment.

Testcase formatting improvements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119829

Files:
  clang/lib/Driver/ToolChains/Solaris.cpp
  clang/test/Driver/solaris-ld-sanitizer.c


Index: clang/test/Driver/solaris-ld-sanitizer.c
===
--- /dev/null
+++ clang/test/Driver/solaris-ld-sanitizer.c
@@ -0,0 +1,51 @@
+/// General tests that the ld -z relax=transtls workaround is only applied
+/// on Solaris/amd64. Note that we use sysroot to make these tests
+/// independent of the host system.
+
+/// Check sparc-sun-solaris2.11, 32bit
+// RUN: %clang --target=sparc-sun-solaris2.11 %s -### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32 %s
+// CHECK-LD-SPARC32-NOT: -zrelax=transtls
+
+/// Check sparc-sun-solaris2.11, 32bit
+// RUN: %clang -fsanitize=undefined --target=sparc-sun-solaris2.11 %s -### 
2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32 %s
+// CHECK-LD-SPARC32-NOT: -zrelax=transtls
+
+/// Check sparc-sun-solaris2.11, 64bit
+// RUN: %clang -m64 --target=sparc-sun-solaris2.11 %s -### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC64 %s
+// CHECK-LD-SPARC64-NOT: -zrelax=transtls
+
+/// Check sparc-sun-solaris2.11, 64bit
+// RUN: %clang -m64 -fsanitize=undefined --target=sparc-sun-solaris2.11 %s 
-### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC64 %s
+// CHECK-LD-SPARC64-NOT: -zrelax=transtls
+
+/// Check i386-pc-solaris2.11, 32bit
+// RUN: %clang --target=i386-pc-solaris2.11 %s -### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-X32 %s
+// CHECK-LD-X32-NOT: -zrelax=transtls
+
+/// Check i386-pc-solaris2.11, 32bit
+// RUN: %clang -fsanitize=undefined --target=i386-pc-solaris2.11 %s -### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-X32 %s
+// CHECK-LD-X32-NOT: -zrelax=transtls
+
+/// Check i386-pc-solaris2.11, 64bit
+// RUN: %clang -m64 --target=i386-pc-solaris2.11 %s -### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-X64 %s
+// CHECK-LD-X64-NOT: -zrelax=transtls
+
+/// Check i386-pc-solaris2.11, 64bit
+// RUN: %clang -m64 -fsanitize=undefined --target=i386-pc-solaris2.11 %s -### 
2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-X64-UBSAN %s
+// CHECK-LD-X64-UBSAN: -zrelax=transtls
Index: clang/lib/Driver/ToolChains/Solaris.cpp
===
--- clang/lib/Driver/ToolChains/Solaris.cpp
+++ clang/lib/Driver/ToolChains/Solaris.cpp
@@ -14,6 +14,8 @@
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
+#include "clang/Driver/SanitizerArgs.h"
+#include "clang/Driver/ToolChain.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
@@ -145,8 +147,18 @@
   CmdArgs.push_back("-lgcc");
   CmdArgs.push_back("-lm");
 }
-if (NeedsSanitizerDeps)
+if (NeedsSanitizerDeps) {
   linkSanitizerRuntimeDeps(getToolChain(), CmdArgs);
+
+  // Work around Solaris/amd64 ld bug when calling __tls_get_addr directly.
+  // However, ld -z relax=transtls is available since Solaris 11.2, but not
+  // in Illumos.
+  const SanitizerArgs  = getToolChain().getSanitizerArgs(Args);
+  if (getToolChain().getTriple().getArch() == llvm::Triple::x86_64 &&
+  (SA.needsAsanRt() || SA.needsStatsRt() ||
+   (SA.needsUbsanRt() && !SA.requiresMinimalRuntime(
+CmdArgs.push_back("-zrelax=transtls");
+}
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {


Index: clang/test/Driver/solaris-ld-sanitizer.c
===
--- /dev/null
+++ clang/test/Driver/solaris-ld-sanitizer.c
@@ -0,0 +1,51 @@
+/// General tests that the ld -z relax=transtls workaround is only applied
+/// on Solaris/amd64. Note that we use sysroot to make these tests
+/// independent of the host system.
+
+/// Check sparc-sun-solaris2.11, 32bit
+// RUN: %clang --target=sparc-sun-solaris2.11 %s -### 2>&1 \
+// RUN: --gcc-toolchain="" --sysroot=%S/Inputs/solaris_sparc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-LD-SPARC32 %s
+// CHECK-LD-SPARC32-NOT: -zrelax=transtls
+
+/// Check sparc-sun-solaris2.11, 32bit

[PATCH] D119829: [Driver] Support Solaris/amd64 GetTls

[Rainer Orth via Phabricator via cfe-commits]

ro added inline comments.



Comment at: clang/test/Driver/solaris-ld-sanitizer.c:6
+/// Check sparc-sun-solaris2.11, 32bit
+// RUN: %clang %s -### 2>&1 \
+// RUN: --target=sparc-sun-solaris2.11 \

MaskRay wrote:
> The first line is now shorter. You can move --target= above.
> 
> For me, the number of lines of a test counts and sometimes compacter RUN 
> lines improve readability.
Depends: on long lines, crucial information can be harder to find.  I moved 
`--target=` up, but kept the distinguishing `-m64`/`-fsanitize=` in front.  
Unfortunately, that causes some of the lines to be longer than 80 characters.

However, the `--gcc-toolchain=` and `--sysroot=` lines could easily be joined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119829

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

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

First sanity check: applying this patch on my branch causes no test failures 
either in the `tools/clang/unittests/Tooling/ToolingTests` or `ninja 
check-clang-semacxx`. Hopefully I can think this through more deeply soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119095

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

[clang] a54b56e - Fix Wdocumentation unknown parameter warning

[Simon Pilgrim via cfe-commits]

Author: Simon Pilgrim
Date: 2022-02-19T13:06:09Z
New Revision: a54b56ecf2e7c35e0bb3e61585a2c27c252069c8

URL: 
https://github.com/llvm/llvm-project/commit/a54b56ecf2e7c35e0bb3e61585a2c27c252069c8
DIFF: 
https://github.com/llvm/llvm-project/commit/a54b56ecf2e7c35e0bb3e61585a2c27c252069c8.diff

LOG: Fix Wdocumentation unknown parameter warning

Added: 


Modified: 
clang/lib/Parse/ParseTemplate.cpp

Removed: 




diff  --git a/clang/lib/Parse/ParseTemplate.cpp 
b/clang/lib/Parse/ParseTemplate.cpp
index f875e3bf43e81..0d8ab6ad2fbcd 100644
--- a/clang/lib/Parse/ParseTemplate.cpp
+++ b/clang/lib/Parse/ParseTemplate.cpp
@@ -1233,8 +1233,6 @@ bool 
Parser::ParseGreaterThanInTemplateList(SourceLocation LAngleLoc,
 /// token that forms the template-id. Otherwise, we will leave the
 /// last token in the stream (e.g., so that it can be replaced with an
 /// annotation token).
-///
-/// \param NameHint is not required, and merely affects code completion.
 bool Parser::ParseTemplateIdAfterTemplateName(bool ConsumeLastToken,
   SourceLocation ,
   TemplateArgList ,



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

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I've reused the test cases from the old clang-tidy implementation of this, 
however this should eventually be moved into ASTMatchers tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120185

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

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

[Nathan James via Phabricator via cfe-commits]

njames93 abandoned this revision.
njames93 added a comment.

I have decided to move this behaviour into the ast matcher directly, I feel in 
the long term, that should be a better home for this functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118520

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

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: klimek, aaron.ballman, LegalizeAdulthood.
Herald added a subscriber: mgorny.
njames93 requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

Create a PrettyStackTraceEvent that will dump the current `MatchCallback` id as 
well as the `BoundNodes` if the 'run' method of a `MatchCallback` results in a 
crash.
The purpose of this is sometimes clang-tidy checks can crash in the `check` 
method. And in a large codebase with alot of checks enabled and in a release 
build, it can be near impossible to figure out which check as well as the 
source code that caused the crash. Without that information a reproducer is 
very hard to create.
This is a more generalised version of D118520 
 which has a nicer integration and should be 
useful to clients other than clang-tidy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/CMakeLists.txt
  clang-tools-extra/test/clang-tidy/CTCrashTestTrace.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/crash-trace.c
  clang-tools-extra/test/lit.cfg.py
  clang/lib/ASTMatchers/ASTMatchFinder.cpp

Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Timer.h"
 #include 
 #include 
@@ -416,6 +417,8 @@
 // matchers.
 class MatchASTVisitor : public RecursiveASTVisitor,
 public ASTMatchFinder {
+  friend class BoundNodeRunStackTrace;
+
 public:
   MatchASTVisitor(const MatchFinder::MatchersByType *Matchers,
   const MatchFinder::MatchFinderOptions )
@@ -765,6 +768,9 @@
   bool TraversingASTNodeNotAsIs = false;
   bool TraversingASTChildrenNotSpelledInSource = false;
 
+  const MatchCallback *CurMatched = nullptr;
+  const BoundNodes *CurBoundNodes = nullptr;
+
   struct ASTNodeNotSpelledInSourceScope {
 ASTNodeNotSpelledInSourceScope(MatchASTVisitor *V, bool B)
 : MV(V), MB(V->TraversingASTNodeNotSpelledInSource) {
@@ -831,7 +837,7 @@
 Timer.setBucket([MP.second->getID()]);
   BoundNodesTreeBuilder Builder;
   if (MP.first.matches(Node, this, )) {
-MatchVisitor Visitor(ActiveASTContext, MP.second);
+MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
 Builder.visitMatches();
   }
 }
@@ -863,7 +869,7 @@
   }
 
   if (MP.first.matches(DynNode, this, )) {
-MatchVisitor Visitor(ActiveASTContext, MP.second);
+MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
 Builder.visitMatches();
   }
 }
@@ -1049,18 +1055,34 @@
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
   // the aggregated bound nodes for each match.
   class MatchVisitor : public BoundNodesTreeBuilder::Visitor {
-  public:
-MatchVisitor(ASTContext* Context,
- MatchFinder::MatchCallback* Callback)
-  : Context(Context),
-Callback(Callback) {}
+struct CurBoundScope {
+  CurBoundScope(MatchASTVisitor , const BoundNodes ) : MV(MV) {
+assert(MV.CurMatched && !MV.CurBoundNodes);
+MV.CurBoundNodes = 
+  }
+
+  ~CurBoundScope() { MV.CurBoundNodes = nullptr; }
 
+private:
+  MatchASTVisitor 
+};
+
+  public:
+MatchVisitor(MatchASTVisitor , ASTContext *Context,
+ MatchFinder::MatchCallback *Callback)
+: MV(MV), Context(Context), Callback(Callback) {
+  assert(!MV.CurMatched && !MV.CurBoundNodes);
+  MV.CurMatched = Callback;
+}
+~MatchVisitor() { MV.CurMatched = nullptr; }
 void visitMatch(const BoundNodes& BoundNodesView) override {
   TraversalKindScope RAII(*Context, Callback->getCheckTraversalKind());
+  CurBoundScope RAII2(MV, BoundNodesView);
   Callback->run(MatchFinder::MatchResult(BoundNodesView, Context));
 }
 
   private:
+MatchASTVisitor 
 ASTContext* Context;
 MatchFinder::MatchCallback* Callback;
   };
@@ -1132,6 +1154,54 @@
   MemoizationMap ResultCache;
 };
 
+class BoundNodeRunStackTrace : llvm::PrettyStackTraceEntry {
+public:
+  BoundNodeRunStackTrace(const MatchASTVisitor ) : MV(MV) {}
+  void print(raw_ostream ) const override {
+if (!MV.CurMatched)
+  return;
+assert(MV.ActiveASTContext &&
+   "ActiveASTContext should be set if there is a matched callback");
+
+OS << "Processing '" << MV.CurMatched->getID() << "'\n";
+const BoundNodes::IDToNodeMap  = MV.CurBoundNodes->getMap();
+if (Map.empty()) {
+  OS << "No bound nodes\n";
+  return;
+}
+OS <<