[PATCH] D96281: [clang-tidy] Add options to flag individual core increments and to ignore macros to readability-function-cognitive-complexity check.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

It would be better to split this into two parts.




Comment at: 
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:540
 
+  if (!FlagBasicIncrements) {
+return;

Drop `{}`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:20
 
+.. option:: FlagBasicIncrements
+

`FlagBasicIncrements` is misleading, this should be `DescribeBasicIncrements`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96281

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

[PATCH] D76342: [OpenMP] Implement '#pragma omp tile'

[Michael Kruse via Phabricator via cfe-commits]

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

Who is going to commit?




Comment at: clang/include/clang/AST/StmtOpenMP.h:651-677
+  static bool
+  doForAllLoops(Stmt *CurStmt, bool TryImperfectlyNestedLoops,
+unsigned NumLoops,
+llvm::function_ref Callback);
+  static bool
+  doForAllLoops(const Stmt *CurStmt, bool TryImperfectlyNestedLoops,
+unsigned NumLoops,

ABataev wrote:
> Meinersbur wrote:
> > ABataev wrote:
> > > Meinersbur wrote:
> > > > Please add doxygen comments.
> > > > 
> > > > IMHO, using callbacks makes the callers significantly more complicated. 
> > > > Why not fill a SmallVectorImpl with the result?
> > > It just hides all internal representation in the class and the 
> > > user/caller should not worry about proper implementation of the loops 
> > > processing. Consider it as a way to encapsulate representation details.
> > That could also be done by returning and iterator/filling an array with the 
> > requested data. The latter is one of the primary uses of `SmallVectorImpl`.
> > 
> > coroutines should make returning an iterator much simpler, eventually when 
> > we are allowed to use C++20.
> I just prefer not to expose internals if they could be processed within the 
> class instance.
Using SmallVectorImpl for this is still the dominant style in clang. For 
instance:
 * Sema::getUndefinedButUsed
 * Sema::FindHiddenVirtualMethods
 * Sema::CollectMultipleMethodsInGlobalPool
 * Sema::SelectBestMethod
 * Sema::CollectIvarsToConstructOrDestruct
 * Sema::collectUnexpandedParameterPacks
 * Sema::FindProtocolDeclaration
 * Sema::GatherArgumentsForCall
 * Sema::GatherGlobalCodeCompletions
 * Sema::getUndefinedButUsed
 * ASTContext::getOverriddenMethods
 * ASTContext::DeepCollectObjCIvars
 * ASTContext::getInjectedTemplateArgs

 ...




Comment at: clang/include/clang/AST/StmtOpenMP.h:659
+llvm::function_ref Callback) {
+auto & = [Callback](unsigned Cnt, Stmt *CurStmt) {
+  return Callback(Cnt, CurStmt);

ABataev wrote:
> Meinersbur wrote:
> > ABataev wrote:
> > > Meinersbur wrote:
> > > > I do not see why making this a forwarding reference.
> > > There is a type mismatch in callback types, `Stmt *` and `const Stmt *`
> > I understand why the additional lambda is needed, but wondered about why 
> > the declaration is not
> > ```
> > auto NewCallback = [Callback](unsigned Cnt, Stmt *CurStmt) { 
> > ```
> > 
> I believe it just saves us from the extra constructor call.
It does not: https://godbolt.org/z/81ac7o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342

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

[PATCH] D96281: [clang-tidy] Add options to flag individual core increments and to ignore macros to readability-function-cognitive-complexity check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h:24-26
+///   * `FlagBasicIncrements`- if set to `true`, additional to flagging
+//  functions exceeding the threshold also every piece of code (loops, if
+//  statements, ...) which contributes to that complexity is flagged.

Thanks! Now the description contains enough information, but it was hard for me 
to understand it. I'd suggest to reword this a bit, for example: "if set to 
`true`, then for each function exceeding the complexity threshold the check 
will issue additional diagnostics on every piece of code (loop, `if` statement, 
etc.) which contributes to that complexity."

As mentioned earlier, an example may help understand the purpose of this option 
even better.



Comment at: 
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h:27
+//  statements, ...) which contributes to that complexity is flagged.
+//  Default is `true`
+///   * `IgnoreMacros` - if set to `true`, the check will ignore code inside

Trailing period is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96281

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

[PATCH] D95912: [OpenMP] Attribute target diagnostics properly

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

friendly ping, also D95928 .

These were supposed to be backported into 12 :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95912

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

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked 6 inline comments as done.
Meinersbur added a subscriber: rsmith.
Meinersbur added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:94
   // been "emitted" to the outside, thus, modifications are still sensible.
-  if (CGM.getLangOpts().OpenMPIRBuilder)
-CGM.getOpenMPRuntime().getOMPBuilder().finalize();
+  if (CGM.getLangOpts().OpenMPIRBuilder && CurFn)
+CGM.getOpenMPRuntime().getOMPBuilder().finalize(CurFn);

jdenny wrote:
> Without this patch, `finalize` is called even if `!CurFn`?
Without CurFn, there is no function to finalize.

The problem before this patch is that finalize would finalize ALL functions, 
even those that are still in construction. In particular, finalizing the 
Distance or LoopVar function would also finalize the parent function.

I added an assert for OpenMPIRBuilder's dtor that checks that everything has 
eventually been finalized.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:3771
+
+  void VisitOMPCanonicalLoop(OMPCanonicalLoop *S) { VisitStmt(S); }
+

jdenny wrote:
> Isn't `VisitStmt` called for any `Stmt` without this?
I think you are correct.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5139
+/// Used to capture variable references if already parsed 
statements/expressions
+/// into a CapturedStatement.
+class CaptureVars : public TreeTransform {

jdenny wrote:
> I think this means:
> 
> > If already parsed statements/expressions, used to capture variable 
> > references into a CapturedStatement.
> 
> But it reads like it means:
> 
> > If already parsed statements/expressions into a CapturedStatement, used to 
> > capture variable references.
if -> of

I reformulated the sentence which I hope is now more clear.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+  Expr *NegIncAmount =
+  AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+  Expr *BackwardDist = AssertSuccess(

jdenny wrote:
> It looks like the AST nodes `NewStart`, `NewStop`, and `NewStep` are shared 
> by multiple parent nodes here.  Does that ever happen anywhere else in a 
> Clang AST?
Yes. For instance, when instantiating a template, nodes form the TemplatedDecl 
are reused unless they need to change. Therefore multiple template 
instantiation can also share entire AST subtrees. Why rebuild them if they are 
identical?

However, I also found the following description of 
TreeTransform::AlwaysRebuild():
```
  /// We must always rebuild all AST nodes when performing variadic template
  /// pack expansion, in order to avoid violating the AST invariant that each
  /// statement node appears at most once in its containing declaration.
```
It was added by @rsmith in rG2aa81a718e64fb4ca651ea12ab7afaeffc11e463 to fix 
llvm.org/PR17800 . The assertion doesn't seem to exist in the current code base.

Does this apply here? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973

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

[PATCH] D96542: [clang-tidy] Fix `TransformerClangTidyCheck`'s handling of include insertions.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:108
+  Diag << Inserter.createIncludeInsertion(
+  Result.SourceManager->getFileID(T.Range.getBegin()), T.Replacement);
   break;

Can this be a macro file id? I'd suggest to add tests (probably for checks 
using this functionality) with a few nested includes and fixes in normal code, 
code in macros declared and expanded in different files, locations in macro 
bodies, macro arguments, and some tricky cases like fix pointing to a pasted 
token.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96542

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

[PATCH] D94779: [Clang] Ensure vector predication loop metadata is always emitted when pragma is specified.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I just pushed the patch with an slight addition to the summary motivitating why 
always emitting the predicate metadata: 
https://github.com/llvm/llvm-project/commit/74ddacd30de54e19b28218bc8563bd0f34f32cad


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94779

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

[PATCH] D94779: [Clang] Ensure vector predication loop metadata is always emitted when pragma is specified.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG74ddacd30de5: [Clang] Ensure vector predication loop 
metadata is always emitted when pragma… (authored by malharJ, committed by 
Meinersbur).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94779

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp


Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -58,6 +58,49 @@
 List[i] = i * 2;
 }
 
+// Check that vectorize_predicate is ignored when vectorization width is 1
+void test6(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test6{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP6:.*]]
+
+#pragma clang loop vectorize_predicate(disable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
+// Check that vectorize_width(!=1) does not affect vectorize_predicate.
+void test7(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test7{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP7:.*]]
+
+#pragma clang loop vectorize_predicate(disable) vectorize_width(4)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
+// Check that vectorize_predicate is ignored when vectorization width is 1
+void test8(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test8{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP8:.*]]
+
+#pragma clang loop vectorize_predicate(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
+// Check that vectorize_width(!=1) does not affect vectorize_predicate.
+void test9(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test9{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP9:.*]]
+
+#pragma clang loop vectorize_predicate(enable) vectorize_width(4)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], [[MP:![0-9]+]], 
[[GEN3:![0-9]+]]}
 // CHECK:  [[MP]] = !{!"llvm.loop.mustprogress"}
 // CHECK-NEXT: [[GEN3]] = !{!"llvm.loop.vectorize.enable", i1 true}
@@ -73,4 +116,14 @@
 // CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], [[MP]], [[GEN10:![0-9]+]]}
 // CHECK-NEXT: [[GEN10]] = !{!"llvm.loop.vectorize.width", i32 1}
 
-// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], [[MP]], [[GEN10]]}
+// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], [[MP]], [[GEN6]], [[GEN10]]}
+
+// CHECK-NEXT: ![[LOOP6]] = distinct !{![[LOOP6]], [[MP]], [[GEN8]], 
[[GEN10]], [[GEN11:![0-9]+]]}
+// CHECK-NEXT: [[GEN11]] = !{!"llvm.loop.vectorize.scalable.enable", i1 false}
+
+// CHECK-NEXT: ![[LOOP7]] = distinct !{![[LOOP7]], [[MP]], [[GEN8]], 
[[GEN12:![0-9]+]], [[GEN11]], [[GEN3]]}
+// CHECK-NEXT: [[GEN12]] = !{!"llvm.loop.vectorize.width", i32 4}
+
+// CHECK-NEXT: ![[LOOP8]] = distinct !{![[LOOP8]], [[MP]], [[GEN6]], 
[[GEN10]], [[GEN11]]}
+
+// CHECK-NEXT: ![[LOOP9]] = distinct !{![[LOOP9]], [[MP]], [[GEN6]], 
[[GEN12]], [[GEN11]], [[GEN3]]}
Index: clang/lib/CodeGen/CGLoopInfo.cpp
===
--- clang/lib/CodeGen/CGLoopInfo.cpp
+++ clang/lib/CodeGen/CGLoopInfo.cpp
@@ -250,12 +250,10 @@
   Args.push_back(nullptr);
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
-  // Setting vectorize.predicate
+  // Setting vectorize.predicate when it has been specified and vectorization
+  // has not been disabled.
   bool IsVectorPredicateEnabled = false;
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
-  Attrs.VectorizeEnable != LoopAttributes::Disable &&
-  Attrs.VectorizeWidth < 1) {
-
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
 IsVectorPredicateEnabled =
 (Attrs.VectorizePredicateEnable == LoopAttributes::Enable);
 
@@ -303,7 +301,8 @@
   //explicitly requested fixed-width vectorization, i.e.
   //vectorize.scalable.enable is false.
   if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
-  IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1 ||
+  (IsVectorPredicateEnabled && Attrs.VectorizeWidth != 1) ||
+  Attrs.VectorizeWidth > 1 ||
   Attrs.VectorizeScalable == LoopAttributes::Enable ||
   (Attrs.VectorizeScalable == LoopAttributes::Disable &&
Attrs.VectorizeWidth != 1)) {


Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -58,6 +58,49 @@
 List[i] = i * 2;
 }
 
+// Check that vectorize_predicate is ignored when vectorization width is 1
+void test6(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test6{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP6:.*]]
+

[clang] 74ddacd - [Clang] Ensure vector predication loop metadata is always emitted when pragma is specified.

[Michael Kruse via cfe-commits]

Author: Malhar
Date: 2021-02-13T17:35:54-06:00
New Revision: 74ddacd30de54e19b28218bc8563bd0f34f32cad

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

LOG: [Clang] Ensure vector predication loop metadata is always emitted when 
pragma is specified.

This patch ensures that vector predication and vectorization width
pragmas work together correctly/as expected. Specifically, this patch
fixes the issue that when vectorization_width > 1, the vector
predication behaviour (this would matter if it has NOT been disabled
explicitly by a pragma) was getting ignored, which was incorrect.

The fix here removes the dependence of vector predication on the
vectorization width. The loop metadata corresponding to clang loop
pragma vectorize_predicate is always emitted, if the pragma is
specified, even if vectorization is disabled by vectorize_width(1)
or vectorize(disable) since the option is also used for interleaving
by the LoopVectorize pass.

Reviewed By: dmgreen, Meinersbur

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

Added: 


Modified: 
clang/lib/CodeGen/CGLoopInfo.cpp
clang/test/CodeGenCXX/pragma-loop-predicate.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGLoopInfo.cpp 
b/clang/lib/CodeGen/CGLoopInfo.cpp
index 8ba40599cfaf..12a6cd8da603 100644
--- a/clang/lib/CodeGen/CGLoopInfo.cpp
+++ b/clang/lib/CodeGen/CGLoopInfo.cpp
@@ -250,12 +250,10 @@ LoopInfo::createLoopVectorizeMetadata(const 
LoopAttributes ,
   Args.push_back(nullptr);
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
-  // Setting vectorize.predicate
+  // Setting vectorize.predicate when it has been specified and vectorization
+  // has not been disabled.
   bool IsVectorPredicateEnabled = false;
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
-  Attrs.VectorizeEnable != LoopAttributes::Disable &&
-  Attrs.VectorizeWidth < 1) {
-
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
 IsVectorPredicateEnabled =
 (Attrs.VectorizePredicateEnable == LoopAttributes::Enable);
 
@@ -303,7 +301,8 @@ LoopInfo::createLoopVectorizeMetadata(const LoopAttributes 
,
   //explicitly requested fixed-width vectorization, i.e.
   //vectorize.scalable.enable is false.
   if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
-  IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1 ||
+  (IsVectorPredicateEnabled && Attrs.VectorizeWidth != 1) ||
+  Attrs.VectorizeWidth > 1 ||
   Attrs.VectorizeScalable == LoopAttributes::Enable ||
   (Attrs.VectorizeScalable == LoopAttributes::Disable &&
Attrs.VectorizeWidth != 1)) {

diff  --git a/clang/test/CodeGenCXX/pragma-loop-predicate.cpp 
b/clang/test/CodeGenCXX/pragma-loop-predicate.cpp
index ec8f82ef9b9e..8a25ed8de623 100644
--- a/clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ b/clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -58,6 +58,49 @@ void test5(int *List, int Length) {
 List[i] = i * 2;
 }
 
+// Check that vectorize_predicate is ignored when vectorization width is 1
+void test6(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test6{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP6:.*]]
+
+#pragma clang loop vectorize_predicate(disable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
+// Check that vectorize_width(!=1) does not affect vectorize_predicate.
+void test7(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test7{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP7:.*]]
+
+#pragma clang loop vectorize_predicate(disable) vectorize_width(4)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
+// Check that vectorize_predicate is ignored when vectorization width is 1
+void test8(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test8{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP8:.*]]
+
+#pragma clang loop vectorize_predicate(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
+// Check that vectorize_width(!=1) does not affect vectorize_predicate.
+void test9(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test9{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP9:.*]]
+
+#pragma clang loop vectorize_predicate(enable) vectorize_width(4)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], [[MP:![0-9]+]], 
[[GEN3:![0-9]+]]}
 // CHECK:  [[MP]] = !{!"llvm.loop.mustprogress"}
 // CHECK-NEXT: [[GEN3]] = !{!"llvm.loop.vectorize.enable", i1 true}
@@ -73,4 +116,14 @@ void test5(int *List, int Length) {
 // CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], [[MP]], [[GEN10:![0-9]+]]}
 // CHECK-NEXT: [[GEN10]] = !{!"llvm.loop.vectorize.width", i32 1}
 
-// CHECK-NEXT: ![[LOOP5]] = 

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 323568.
fhahn added a comment.

Rebased on top of 51bf4c0e6d4c 
, which 
added -ffinite-loops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96418

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/attr-mustprogress.c
  clang/test/CodeGenCXX/attr-mustprogress.cpp

Index: clang/test/CodeGenCXX/attr-mustprogress.cpp
===
--- clang/test/CodeGenCXX/attr-mustprogress.cpp
+++ clang/test/CodeGenCXX/attr-mustprogress.cpp
@@ -16,27 +16,31 @@
 // CHECK: datalayout
 
 // CXX98-NOT: mustprogress
-// CXX11-NOT: mustprogress
-// FINITE-NOT: mustprogress
+// CXX11: mustprogress
+// FINITE:mustprogress
 // CHECK-LABEL: @_Z2f0v(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:br label %for.cond
 // CHECK:   for.cond:
-// CHECK-NOT:br {{.*}} llvm.loop
+// CXX98-NOT:br {{.*}} llvm.loop
+// CXX11-NEXT:   br label %for.cond, !llvm.loop [[LOOP1:!.*]]
+// FINITE-NEXT:  br label %for.cond, !llvm.loop [[LOOP1:!.*]]
 void f0() {
   for (; ;) ;
 }
 
 // CXX98-NOT: mustprogress
-// CXX11-NOT: mustprogress
-// FINITE-NOT: mustprogress
+// CXX11: mustprogress
+// FINITE:mustprogress
 // CHECK-LABEL: @_Z2f1v(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:br label %for.cond
 // CHECK:   for.cond:
 // CHECK-NEXT:br i1 true, label %for.body, label %for.end
 // CHECK:   for.body:
-// CHECK-NOT:br {{.*}}, !llvm.loop
+// CXX98-NOT: br {{.*}}, !llvm.loop
+// CXX11-NEXT:br label %for.cond, !llvm.loop [[LOOP2:!.*]]
+// FINITE-NEXT:  br label %for.cond, !llvm.loop [[LOOP2:!.*]]
 // CHECK:   for.end:
 // CHECK-NEXT:ret void
 //
@@ -58,8 +62,8 @@
 // CHECK-NEXT:br i1 [[CMP]], label %for.body, label %for.end
 // CHECK:   for.body:
 // CXX98-NOT:br {{.*}}, !llvm.loop
-// CXX11:br label %for.cond, !llvm.loop [[LOOP1:!.*]]
-// FINITE-NEXT:   br label %for.cond, !llvm.loop [[LOOP1:!.*]]
+// CXX11:br label %for.cond, !llvm.loop [[LOOP3:!.*]]
+// FINITE-NEXT:  br label %for.cond, !llvm.loop [[LOOP3:!.*]]
 // CHECK:   for.end:
 // CHECK-NEXT:ret void
 //
@@ -69,15 +73,17 @@
 }
 
 // CXX98-NOT: mustprogress
-// CXX11-NOT: mustprogress
-// FINITE-NOT: mustprogress
+// CXX11: mustprogress
+// FINITE:mustprogress
 // CHECK-LABEL: @_Z1Fv(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:br label %for.cond
 // CHECK:   for.cond:
 // CHECK-NEXT:br i1 true, label %for.body, label %for.end
 // CHECK:   for.body:
-// CHECK-NOT: br {{.*}}, !llvm.loop
+// CXX98-NOT: br {{.*}}, !llvm.loop
+// CXX11-NEXT:br label %for.cond, !llvm.loop [[LOOP4:!.*]]
+// FINITE-NEXT:   br label %for.cond, !llvm.loop [[LOOP4:!.*]]
 // CHECK:   for.end:
 // CHECK-NEXT:br label %for.cond1
 // CHECK:   for.cond1:
@@ -87,8 +93,8 @@
 // CHECK-NEXT:br i1 [[CMP]], label %for.body2, label %for.end3
 // CHECK:   for.body2:
 // CXX98-NOT: br {{.*}}, !llvm.loop
-// CXX11-NEXT:br label %for.cond1, !llvm.loop [[LOOP2:!.*]]
-// FINITE-NEXT:   br label %for.cond1, !llvm.loop [[LOOP2:!.*]]
+// CXX11-NEXT:br label %for.cond1, !llvm.loop [[LOOP5:!.*]]
+// FINITE-NEXT:   br label %for.cond1, !llvm.loop [[LOOP5:!.*]]
 // CHECK:   for.end3:
 // CHECK-NEXT:ret void
 //
@@ -100,7 +106,7 @@
 }
 
 // CXX98-NOT: mustprogress
-// CXX11-NOT: mustprogress
+// CXX11: mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2F2v(
 // CHECK-NEXT:  entry:
@@ -112,14 +118,16 @@
 // CHECK-NEXT:br i1 [[CMP]], label %for.body, label %for.end
 // CHECK:   for.body:
 // CXX98_NOT: br {{.*}} !llvm.loop
-// CXX11-NEXT:br label %for.cond, !llvm.loop [[LOOP3:!.*]]
-// FINITE-NEXT:br label %for.cond, !llvm.loop [[LOOP3:!.*]]
+// CXX11-NEXT:br label %for.cond, !llvm.loop [[LOOP6:!.*]]
+// FINITE-NEXT:   br label %for.cond, !llvm.loop [[LOOP6:!.*]]
 // CHECK:   for.end:
 // CHECK-NEXT:br label %for.cond1
 // CHECK:   for.cond1:
 // CHECK-NEXT:br i1 true, label %for.body2, label %for.end3
 // CHECK:   for.body2:
-// CHECK-NOT: br {{.*}}, !llvm.loop
+// CXX98-NOT: br {{.*}}, !llvm.loop
+// CXX11-NEXT:br label %for.cond1, !llvm.loop [[LOOP7:!.*]]
+// FINITE-NEXT:   br label %for.cond1, !llvm.loop [[LOOP7:!.*]]
 // CHECK:   for.end3:
 // CHECK-NEXT:ret void
 //
@@ -131,13 +139,15 @@
 }
 
 // CXX98-NOT: mustprogress
-// CXX11-NOT: mustprogress
-// FINITE-NOT: mustprogress
+// CXX11: mustprogress
+// FINITE:mustprogress
 // CHECK-LABEL: @_Z2w1v(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:br label %while.body
 // CHECK:   while.body:
-// CHECK-NOT: br {{.*}}, !llvm.loop
+// CXX98-NOT: br {{.*}}, !llvm.loop
+// CXX11-NEXT:br label %while.body, 

[PATCH] D95754: [clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

Not sure who can review this, but looking through blame it seems like maybe 
@aaronpuchert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95754

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

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D96082#2550468 , @LukasHanel wrote:

> In D96082#2549943 , @steveire wrote:
>
>> In D96082#2545807 , @LukasHanel 
>> wrote:
>>
>>> Hi, thanks for discussing my proposal!
>>>
>>> - Usefulness of the fix-it's
>>
>> I agree with @njames93 that this check is dangerous. Even if you extended it 
>> to port callExprs, that would only work in translation units which can see 
>> the definition.
>
> Are you saying I should just remove the fix-its altogether?
> Or, put them under some option that is off by default?

I'm not sure this check meets the general applicability and usefulness 
threshold to be part of clang-tidy. The warning alone isn't actionable. Someone 
wanting to take action on the warning would have to write their own clang tidy 
check to make the change across their codebase. Add that to the false-positive 
issues I mentioned before.

Does anyone have any other thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96082

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

[PATCH] D95168: [clang-format] Add InsertBraces option

[MyDeveloperDay via Phabricator via cfe-commits]

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

LGTM, we need to run this on a large code base to ensure it doesn't make 
mistakes in real code, but I think this looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95168

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

[PATCH] D95737: [NFC][Docs] Fix RAVFrontendAction doc's CMakelists.txt for Shared build

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Thank @awarzynski for reviewing and explaining the details. I somehow missed 
it. Should be fixed by  
https://github.com/llvm/llvm-project/commit/d1ef9a63a68850bbe8cd8877f69c41833804c8dc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95737

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

[clang] d1ef9a6 - [NFC][Docs] Fix RAVFrontendAction doc's CMakeLists.txt for shared build

[via cfe-commits]

Author: Shivam Gupta
Date: 2021-02-13T19:48:49+05:30
New Revision: d1ef9a63a68850bbe8cd8877f69c41833804c8dc

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

LOG: [NFC][Docs] Fix RAVFrontendAction doc's CMakeLists.txt for shared build

It should fix following error:

  Undefined symbols for architecture x86_64:
"llvm::outs()", referenced from:
FindNamedClassVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl*) in 
FindClassDecls.cpp.o

Added: 


Modified: 
clang/docs/RAVFrontendAction.rst

Removed: 




diff  --git a/clang/docs/RAVFrontendAction.rst 
b/clang/docs/RAVFrontendAction.rst
index 71722a15936a..e38276b43295 100644
--- a/clang/docs/RAVFrontendAction.rst
+++ b/clang/docs/RAVFrontendAction.rst
@@ -205,6 +205,10 @@ following CMakeLists.txt to link it:
 
 ::
 
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
 add_clang_executable(find-class-decls FindClassDecls.cpp)
 
 target_link_libraries(find-class-decls 



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

[PATCH] D96508: [clangd] Retire clang-tidy-checks flag.

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG022920c25b8e: [clangd] Retire clang-tidy-checks flag. 
(authored by njames93).

Changed prior to commit:
  https://reviews.llvm.org/D96508?vs=323011=323553#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96508

Files:
  clang-tools-extra/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -172,14 +172,6 @@
 init(true),
 };
 
-opt ClangTidyChecks{
-"clang-tidy-checks",
-cat(Features),
-desc("List of clang-tidy checks to run (this will override "
- ".clang-tidy files). Only meaningful when -clang-tidy flag is on"),
-init(""),
-};
-
 opt CodeCompletionParse{
 "completion-parse",
 cat(Features),
@@ -287,6 +279,7 @@
 RetiredFlag AsyncPreamble("async-preamble");
 RetiredFlag CollectMainFileRefs("collect-main-file-refs");
 RetiredFlag CrossFileRename("cross-file-rename");
+RetiredFlag ClangTidyChecks("clang-tidy-checks");
 
 opt LimitResults{
 "limit-results",
@@ -826,10 +819,7 @@
 Providers.push_back(provideClangTidyFiles(TFS));
 if (EnableConfig)
   Providers.push_back(provideClangdConfig());
-if (!ClangTidyChecks.empty())
-  Providers.push_back(addTidyChecks(ClangTidyChecks));
-else
-  Providers.push_back(provideDefaultChecks());
+Providers.push_back(provideDefaultChecks());
 Providers.push_back(disableUnusableChecks());
 ClangTidyOptProvider = combine(std::move(Providers));
 Opts.ClangTidyProvider = ClangTidyOptProvider;


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -172,14 +172,6 @@
 init(true),
 };
 
-opt ClangTidyChecks{
-"clang-tidy-checks",
-cat(Features),
-desc("List of clang-tidy checks to run (this will override "
- ".clang-tidy files). Only meaningful when -clang-tidy flag is on"),
-init(""),
-};
-
 opt CodeCompletionParse{
 "completion-parse",
 cat(Features),
@@ -287,6 +279,7 @@
 RetiredFlag AsyncPreamble("async-preamble");
 RetiredFlag CollectMainFileRefs("collect-main-file-refs");
 RetiredFlag CrossFileRename("cross-file-rename");
+RetiredFlag ClangTidyChecks("clang-tidy-checks");
 
 opt LimitResults{
 "limit-results",
@@ -826,10 +819,7 @@
 Providers.push_back(provideClangTidyFiles(TFS));
 if (EnableConfig)
   Providers.push_back(provideClangdConfig());
-if (!ClangTidyChecks.empty())
-  Providers.push_back(addTidyChecks(ClangTidyChecks));
-else
-  Providers.push_back(provideDefaultChecks());
+Providers.push_back(provideDefaultChecks());
 Providers.push_back(disableUnusableChecks());
 ClangTidyOptProvider = combine(std::move(Providers));
 Opts.ClangTidyProvider = ClangTidyOptProvider;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 022920c - [clangd] Retire clang-tidy-checks flag.

[Nathan James via cfe-commits]

Author: Nathan James
Date: 2021-02-13T14:14:22Z
New Revision: 022920c25b8eb90b0bd0d209a9d0f836743a21bb

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

LOG: [clangd] Retire clang-tidy-checks flag.

In clangd-12 the ability to override what clang tidy checks should run was 
moved into config.
For the 13 release its a wise progression to remove the command line option for 
this.

Reviewed By: sammccall

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

Added: 


Modified: 
clang-tools-extra/clangd/tool/ClangdMain.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp 
b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 595cbcf80d51..51280b73afac 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -172,14 +172,6 @@ opt EnableClangTidy{
 init(true),
 };
 
-opt ClangTidyChecks{
-"clang-tidy-checks",
-cat(Features),
-desc("List of clang-tidy checks to run (this will override "
- ".clang-tidy files). Only meaningful when -clang-tidy flag is on"),
-init(""),
-};
-
 opt CodeCompletionParse{
 "completion-parse",
 cat(Features),
@@ -287,6 +279,7 @@ RetiredFlag RecoveryASTType("recovery-ast-type");
 RetiredFlag AsyncPreamble("async-preamble");
 RetiredFlag CollectMainFileRefs("collect-main-file-refs");
 RetiredFlag CrossFileRename("cross-file-rename");
+RetiredFlag ClangTidyChecks("clang-tidy-checks");
 
 opt LimitResults{
 "limit-results",
@@ -826,10 +819,7 @@ clangd accepts flags on the commandline, and in the 
CLANGD_FLAGS environment var
 Providers.push_back(provideClangTidyFiles(TFS));
 if (EnableConfig)
   Providers.push_back(provideClangdConfig());
-if (!ClangTidyChecks.empty())
-  Providers.push_back(addTidyChecks(ClangTidyChecks));
-else
-  Providers.push_back(provideDefaultChecks());
+Providers.push_back(provideDefaultChecks());
 Providers.push_back(disableUnusableChecks());
 ClangTidyOptProvider = combine(std::move(Providers));
 Opts.ClangTidyProvider = ClangTidyOptProvider;



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

[PATCH] D96138: [clang-tidy] Simplify delete null ptr check

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 323551.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96138

Files:
  clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.cpp
  clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.h
  clang-tools-extra/test/clang-tidy/checkers/readability-delete-null-pointer.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-delete-null-pointer.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-delete-null-pointer.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-delete-null-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-delete-null-pointer %t
+// RUN: %check_clang_tidy %s readability-delete-null-pointer %t -- -- -fno-delayed-template-parsing
 
 #define NULL 0
 
@@ -37,6 +37,34 @@
   ti3.foo();
 }
 
+template 
+struct NeverInstantiated {
+  void foo() {
+// t1
+if (mem) // t2
+  delete mem;
+// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 'if' statement is unnecessary;
+// CHECK-FIXES: // t1
+// CHECK-FIXES-NEXT: {{^}}// t2
+// CHECK-FIXES-NEXT: delete mem;
+  }
+  T mem;
+};
+
+template 
+struct NeverInstantiatedPtr {
+  void foo() {
+// t3
+if (mem) // t4
+  delete mem;
+// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 'if' statement is unnecessary;
+// CHECK-FIXES: // t3
+// CHECK-FIXES-NEXT: {{^}}// t4
+// CHECK-FIXES-NEXT: delete mem;
+  }
+  T *mem;
+};
+
 void f() {
   int *ps = 0;
   if (ps /**/) // #0
Index: clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.h
@@ -26,6 +26,9 @@
   : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -20,34 +20,30 @@
 
 void DeleteNullPointerCheck::registerMatchers(MatchFinder *Finder) {
   const auto DeleteExpr =
-  cxxDeleteExpr(has(castExpr(has(declRefExpr(
-to(decl(equalsBoundNode("deletedPointer"
+  cxxDeleteExpr(
+  has(declRefExpr(to(decl(equalsBoundNode("deletedPointer"))
   .bind("deleteExpr");
 
   const auto DeleteMemberExpr =
-  cxxDeleteExpr(has(castExpr(has(memberExpr(hasDeclaration(
-fieldDecl(equalsBoundNode("deletedMemberPointer"
+  cxxDeleteExpr(has(memberExpr(hasDeclaration(
+fieldDecl(equalsBoundNode("deletedMemberPointer"))
   .bind("deleteMemberExpr");
 
-  const auto PointerExpr = ignoringImpCasts(anyOf(
+  const auto PointerExpr = anyOf(
   declRefExpr(to(decl().bind("deletedPointer"))),
-  memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer");
+  memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer";
 
-  const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean),
- hasSourceExpression(PointerExpr));
-  const auto BinaryPointerCheckCondition = binaryOperator(
-  hasOperands(castExpr(hasCastKind(CK_NullToPointer)), PointerExpr));
+  const auto BinaryPointerCheckCondition = binaryOperator(hasOperands(
+  anyOf(cxxNullPtrLiteralExpr(), integerLiteral(equals(0))), PointerExpr));
 
   Finder->addMatcher(
-  traverse(TK_AsIs,
-   ifStmt(hasCondition(
-  anyOf(PointerCondition, BinaryPointerCheckCondition)),
-  hasThen(anyOf(DeleteExpr, DeleteMemberExpr,
-compoundStmt(anyOf(has(DeleteExpr),
-   has(DeleteMemberExpr)),
- statementCountIs(1))
-.bind("compound"
-   .bind("ifWithDelete")),
+  ifStmt(hasCondition(anyOf(PointerExpr, BinaryPointerCheckCondition)),
+ hasThen(anyOf(
+ DeleteExpr, DeleteMemberExpr,
+ compoundStmt(anyOf(has(DeleteExpr), has(DeleteMemberExpr)),
+  statementCountIs(1))
+ .bind("compound"
+  .bind("ifWithDelete"),
   

[PATCH] D96139: [clang-tidy] Simplify inaccurate erase check

[Stephen Kelly 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 rGf2f920b987f3: [clang-tidy] Simplify inaccurate erase check 
(authored by stephenkelly).

Changed prior to commit:
  https://reviews.llvm.org/D96139?vs=321747=323550#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96139

Files:
  clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-inaccurate-erase %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s bugprone-inaccurate-erase %t
 
 namespace std {
 template  struct vec_iterator {
Index: clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
===
--- clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
@@ -31,6 +31,9 @@
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 };
 
 } // namespace bugprone
Index: clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
@@ -22,25 +22,18 @@
   callExpr(
   callee(functionDecl(hasAnyName("remove", "remove_if", "unique"))),
   hasArgument(
-  1,
-  anyOf(cxxConstructExpr(has(ignoringImplicit(
-
cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"
-.bind("end",
-anything(
+  1, 
optionally(cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"
+   .bind("end"
   .bind("alg");
 
   const auto DeclInStd = type(hasUnqualifiedDesugaredType(
   tagType(hasDeclaration(decl(isInStdNamespace());
   Finder->addMatcher(
-  traverse(
-  TK_AsIs,
-  cxxMemberCallExpr(
-  on(anyOf(hasType(DeclInStd), hasType(pointsTo(DeclInStd,
-  callee(cxxMethodDecl(hasName("erase"))), argumentCountIs(1),
-  hasArgument(0, has(ignoringImplicit(anyOf(
- EndCall, has(ignoringImplicit(EndCall)),
-  unless(isInTemplateInstantiation()))
-  .bind("erase")),
+  cxxMemberCallExpr(
+  on(anyOf(hasType(DeclInStd), hasType(pointsTo(DeclInStd,
+  callee(cxxMethodDecl(hasName("erase"))), argumentCountIs(1),
+  hasArgument(0, EndCall))
+  .bind("erase"),
   this);
 }
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-inaccurate-erase %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s bugprone-inaccurate-erase %t
 
 namespace std {
 template  struct vec_iterator {
Index: clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
===
--- clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
@@ -31,6 +31,9 @@
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 };
 
 } // namespace bugprone
Index: clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
@@ -22,25 +22,18 @@
   callExpr(
   callee(functionDecl(hasAnyName("remove", "remove_if", "unique"))),
   hasArgument(
-  1,
-

[clang-tools-extra] f2f920b - [clang-tidy] Simplify inaccurate erase check

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2021-02-13T13:51:27Z
New Revision: f2f920b987f3810863b9d0160cbb52df3fa2ab6f

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

LOG: [clang-tidy] Simplify inaccurate erase check

The normalization of matchers means that this now works in all language
modes.

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

Added: 


Modified: 
clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp 
b/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
index 8e5a53280183..96fa7ef48348 100644
--- a/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.cpp
@@ -22,25 +22,18 @@ void InaccurateEraseCheck::registerMatchers(MatchFinder 
*Finder) {
   callExpr(
   callee(functionDecl(hasAnyName("remove", "remove_if", "unique"))),
   hasArgument(
-  1,
-  anyOf(cxxConstructExpr(has(ignoringImplicit(
-
cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"
-.bind("end",
-anything(
+  1, 
optionally(cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"
+   .bind("end"
   .bind("alg");
 
   const auto DeclInStd = type(hasUnqualifiedDesugaredType(
   tagType(hasDeclaration(decl(isInStdNamespace());
   Finder->addMatcher(
-  traverse(
-  TK_AsIs,
-  cxxMemberCallExpr(
-  on(anyOf(hasType(DeclInStd), hasType(pointsTo(DeclInStd,
-  callee(cxxMethodDecl(hasName("erase"))), argumentCountIs(1),
-  hasArgument(0, has(ignoringImplicit(anyOf(
- EndCall, has(ignoringImplicit(EndCall)),
-  unless(isInTemplateInstantiation()))
-  .bind("erase")),
+  cxxMemberCallExpr(
+  on(anyOf(hasType(DeclInStd), hasType(pointsTo(DeclInStd,
+  callee(cxxMethodDecl(hasName("erase"))), argumentCountIs(1),
+  hasArgument(0, EndCall))
+  .bind("erase"),
   this);
 }
 

diff  --git a/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h 
b/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
index ef6006dca888..dd6eb80a41d1 100644
--- a/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/InaccurateEraseCheck.h
@@ -31,6 +31,9 @@ class InaccurateEraseCheck : public ClangTidyCheck {
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 };
 
 } // namespace bugprone

diff  --git 
a/clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
index eff57eaabf7a..d29fa9cdd4e9 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-inaccurate-erase.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-inaccurate-erase %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s bugprone-inaccurate-erase %t
 
 namespace std {
 template  struct vec_iterator {



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

[PATCH] D96223: [clang-tidy] Simplify static assert check

[Stephen Kelly via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1709bb8c7395: [clang-tidy] Simplify static assert check 
(authored by stephenkelly).

Changed prior to commit:
  https://reviews.llvm.org/D96223?vs=322000=323549#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96223

Files:
  clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
  clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h


Index: clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
===
--- clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
+++ clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
@@ -30,6 +30,9 @@
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   SourceLocation getLastParenLoc(const ASTContext *ASTCtx,
Index: clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
@@ -27,48 +27,37 @@
 : ClangTidyCheck(Name, Context) {}
 
 void StaticAssertCheck::registerMatchers(MatchFinder *Finder) {
-  auto NegatedString = unaryOperator(
-  hasOperatorName("!"), 
hasUnaryOperand(ignoringImpCasts(stringLiteral(;
+  auto NegatedString =
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(stringLiteral()));
   auto IsAlwaysFalse =
   expr(anyOf(cxxBoolLiteral(equals(false)), integerLiteral(equals(0)),
  cxxNullPtrLiteralExpr(), gnuNullExpr(), NegatedString))
   .bind("isAlwaysFalse");
-  auto IsAlwaysFalseWithCast = ignoringParenImpCasts(anyOf(
-  IsAlwaysFalse, cStyleCastExpr(has(ignoringParenImpCasts(IsAlwaysFalse)))
- .bind("castExpr")));
-  auto AssertExprRoot = anyOf(
-  binaryOperator(
-  hasAnyOperatorName("&&", "=="),
-  
hasEitherOperand(ignoringImpCasts(stringLiteral().bind("assertMSG"))),
-  anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalseWithCast)),
-anything()))
-  .bind("assertExprRoot"),
-  IsAlwaysFalse);
+  auto IsAlwaysFalseWithCast =
+  anyOf(IsAlwaysFalse, 
cStyleCastExpr(has(IsAlwaysFalse)).bind("castExpr"));
+  auto AssertExprRoot =
+  anyOf(binaryOperator(
+hasAnyOperatorName("&&", "=="),
+hasEitherOperand(stringLiteral().bind("assertMSG")),
+anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalseWithCast)),
+  anything()))
+.bind("assertExprRoot"),
+IsAlwaysFalse);
   auto NonConstexprFunctionCall =
   callExpr(hasDeclaration(functionDecl(unless(isConstexpr();
   auto AssertCondition =
-  expr(
-  anyOf(expr(ignoringParenCasts(anyOf(
-AssertExprRoot, unaryOperator(hasUnaryOperand(
-
ignoringParenCasts(AssertExprRoot)),
-anything()),
-  unless(findAll(NonConstexprFunctionCall)))
+  expr(optionally(expr(anyOf(AssertExprRoot,
+unaryOperator(hasUnaryOperand(AssertExprRoot),
+   unless(findAll(NonConstexprFunctionCall)))
   .bind("condition");
   auto Condition =
-  anyOf(ignoringParenImpCasts(callExpr(
-hasDeclaration(functionDecl(hasName("__builtin_expect"))),
-hasArgument(0, AssertCondition))),
+  anyOf(callExpr(traverse(TK_AsIs, callExpr(hasDeclaration(functionDecl(
+   hasName("__builtin_expect"),
+ hasArgument(0, AssertCondition)),
 AssertCondition);
 
-  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
- unless(isInTemplateInstantiation()))
- .bind("condStmt"),
- this);
-
   Finder->addMatcher(
-  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
-  .bind("condStmt"),
-  this);
+  mapAnyOf(ifStmt, 
conditionalOperator).with(hasCondition(Condition)).bind("condStmt"), this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {


Index: clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
===
--- clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
+++ clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
@@ -30,6 +30,9 @@
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() 

[clang-tools-extra] 1709bb8 - [clang-tidy] Simplify static assert check

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2021-02-13T13:49:01Z
New Revision: 1709bb8c7395418236ec94fe3b9d91fed746452b

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

LOG: [clang-tidy] Simplify static assert check

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

Added: 


Modified: 
clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp 
b/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
index 3e3f8dbf02ff5..2d6504cae689a 100644
--- a/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
@@ -27,48 +27,37 @@ StaticAssertCheck::StaticAssertCheck(StringRef Name, 
ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context) {}
 
 void StaticAssertCheck::registerMatchers(MatchFinder *Finder) {
-  auto NegatedString = unaryOperator(
-  hasOperatorName("!"), 
hasUnaryOperand(ignoringImpCasts(stringLiteral(;
+  auto NegatedString =
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(stringLiteral()));
   auto IsAlwaysFalse =
   expr(anyOf(cxxBoolLiteral(equals(false)), integerLiteral(equals(0)),
  cxxNullPtrLiteralExpr(), gnuNullExpr(), NegatedString))
   .bind("isAlwaysFalse");
-  auto IsAlwaysFalseWithCast = ignoringParenImpCasts(anyOf(
-  IsAlwaysFalse, cStyleCastExpr(has(ignoringParenImpCasts(IsAlwaysFalse)))
- .bind("castExpr")));
-  auto AssertExprRoot = anyOf(
-  binaryOperator(
-  hasAnyOperatorName("&&", "=="),
-  
hasEitherOperand(ignoringImpCasts(stringLiteral().bind("assertMSG"))),
-  anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalseWithCast)),
-anything()))
-  .bind("assertExprRoot"),
-  IsAlwaysFalse);
+  auto IsAlwaysFalseWithCast =
+  anyOf(IsAlwaysFalse, 
cStyleCastExpr(has(IsAlwaysFalse)).bind("castExpr"));
+  auto AssertExprRoot =
+  anyOf(binaryOperator(
+hasAnyOperatorName("&&", "=="),
+hasEitherOperand(stringLiteral().bind("assertMSG")),
+anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalseWithCast)),
+  anything()))
+.bind("assertExprRoot"),
+IsAlwaysFalse);
   auto NonConstexprFunctionCall =
   callExpr(hasDeclaration(functionDecl(unless(isConstexpr();
   auto AssertCondition =
-  expr(
-  anyOf(expr(ignoringParenCasts(anyOf(
-AssertExprRoot, unaryOperator(hasUnaryOperand(
-
ignoringParenCasts(AssertExprRoot)),
-anything()),
-  unless(findAll(NonConstexprFunctionCall)))
+  expr(optionally(expr(anyOf(AssertExprRoot,
+unaryOperator(hasUnaryOperand(AssertExprRoot),
+   unless(findAll(NonConstexprFunctionCall)))
   .bind("condition");
   auto Condition =
-  anyOf(ignoringParenImpCasts(callExpr(
-hasDeclaration(functionDecl(hasName("__builtin_expect"))),
-hasArgument(0, AssertCondition))),
+  anyOf(callExpr(traverse(TK_AsIs, callExpr(hasDeclaration(functionDecl(
+   hasName("__builtin_expect"),
+ hasArgument(0, AssertCondition)),
 AssertCondition);
 
-  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
- unless(isInTemplateInstantiation()))
- .bind("condStmt"),
- this);
-
   Finder->addMatcher(
-  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
-  .bind("condStmt"),
-  this);
+  mapAnyOf(ifStmt, 
conditionalOperator).with(hasCondition(Condition)).bind("condStmt"), this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {

diff  --git a/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h 
b/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
index 0168d1fcd107f..796fc4827db42 100644
--- a/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h
@@ -30,6 +30,9 @@ class StaticAssertCheck : public ClangTidyCheck {
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  llvm::Optional getCheckTraversalKind() const override {
+return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   SourceLocation getLastParenLoc(const ASTContext *ASTCtx,



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D95737: [NFC][Docs] Fix RAVFrontendAction doc's CMakelists.txt for Shared build

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hi @xgupta , thank you for working on this!

It's quite easy to miss dependencies when building with `BUILD_SHARED_LIBS=Off` 
(i.e. with the default setting - static libs). IIUC, that's what happened here. 
This example clearly requires the following:

- `ASTConsumer` implemented in `clangAST`
- `FullSourceLoc` implemented in `clangBasic`
- `ASTFrontendAction` implemented in `clangFrontend`
- `PCHContainerOperations` (required for the invocation of `runToolOnCode`)  
implemented in `clangSerialization`

All the above were missing and your patch fixes that, thank you a ton! I think 
that this is also missing (implements `llvm::outs()`):

  set(LLVM_LINK_COMPONENTS
Support
)

I tested on Darwin with `BUILD_SHARED_LIBS=On` and I'm getting build errors 
without it:

  Undefined symbols for architecture x86_64:
"llvm::outs()", referenced from:
FindNamedClassVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl*) in 
FindClassDecls.cpp.o

PS Apologies for taking so much time to review this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95737

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

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

the LGTM, please mark your comments as done when done.


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

https://reviews.llvm.org/D94661

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

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

I added the changes to the langref in https://reviews.llvm.org/D96646


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645

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

[PATCH] D96607: [clang-tidy] Add check 'readability-pointer-type-star-placement'.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D96607#2561287 , @njames93 wrote:

> I don't see the use case you suggest as a strong enough motivation for this, 
> clang-format does this job very well and we shouldn't be reinventing the 
> wheel here.

Can you turn all mechanisms of clang-format off, except the pointer placement 
rule?



Several coding guidelines and projects put the pointer to the left, not to the 
right. So even if this check goes in, there should definitely be an option for 
which side you want to enforce and fix for...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96607

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

Re: [PATCH] D96244: [clangd] Introduce Modules

[Kadir Çetinkaya via cfe-commits]

oops, d25fbaa4a4a1284e998f545d45280e976c29cc85 should fix the issue.

On Sat, Feb 13, 2021 at 2:54 PM Simon Pilgrim via Phabricator <
revi...@reviews.llvm.org> wrote:

> RKSimon added a comment.
>
> In D96244#2558345 , @sammccall
> wrote:
>
> > I'm a little bit nervous about adding this with *no* usage, but it keeps
> the patch size down :-)
>
> Indeed - this has broken at least one buildbot (
> http://lab.llvm.org:8011/#/builders/57) due to
> -Werror,-Wunused-private-field
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D96244/new/
>
> https://reviews.llvm.org/D96244
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] d25fbaa - [clangd] Fix unsued private field warning

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2021-02-13T13:20:15+01:00
New Revision: d25fbaa4a4a1284e998f545d45280e976c29cc85

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

LOG: [clangd] Fix unsued private field warning

Added: 


Modified: 
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp 
b/clang-tools-extra/clangd/ClangdServer.cpp
index bf834eccd9f5..b39e582d84a1 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -127,7 +127,7 @@ ClangdServer::Options::operator TUScheduler::Options() 
const {
 ClangdServer::ClangdServer(const GlobalCompilationDatabase ,
const ThreadsafeFS , const Options ,
Callbacks *Callbacks)
-: CDB(CDB), TFS(TFS), Modules(Opts.Modules),
+: CDB(CDB), TFS(TFS),
   DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
   ClangTidyProvider(Opts.ClangTidyProvider),
   WorkspaceRoot(Opts.WorkspaceRoot),

diff  --git a/clang-tools-extra/clangd/ClangdServer.h 
b/clang-tools-extra/clangd/ClangdServer.h
index 5e51616a4b2d..83c8567461ec 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -339,7 +339,6 @@ class ClangdServer {
 
   const GlobalCompilationDatabase 
   const ThreadsafeFS 
-  ModuleSet *Modules = nullptr;
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:



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

[PATCH] D96244: [clangd] Introduce Modules

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

In D96244#2558345 , @sammccall wrote:

> I'm a little bit nervous about adding this with *no* usage, but it keeps the 
> patch size down :-)

Indeed - this has broken at least one buildbot 
(http://lab.llvm.org:8011/#/builders/57) due to -Werror,-Wunused-private-field


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96244

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

[PATCH] D96611: [analyzer][tests] Fix issue comparison script

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG94a1a5d25f55: [analyzer][tests] Fix issue comparison script 
(authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96611

Files:
  clang/utils/analyzer/CmpRuns.py


Index: clang/utils/analyzer/CmpRuns.py
===
--- clang/utils/analyzer/CmpRuns.py
+++ clang/utils/analyzer/CmpRuns.py
@@ -36,7 +36,7 @@
 from copy import copy
 from enum import Enum
 from typing import (Any, DefaultDict, Dict, List, NamedTuple, Optional,
-Sequence, TextIO, TypeVar, Tuple, Union)
+Sequence, Set, TextIO, TypeVar, Tuple, Union)
 
 
 Number = Union[int, float]
@@ -374,8 +374,9 @@
 
 # Quadratic algorithms in this part are fine because 'old' and 'new'
 # are most commonly of size 1.
-for a in copy(old):
-for b in copy(new):
+common: Set[AnalysisDiagnostic] = set()
+for a in old:
+for b in new:
 if a.get_issue_identifier() == b.get_issue_identifier():
 a_path_len = a.get_path_length()
 b_path_len = b.get_path_length()
@@ -394,16 +395,22 @@
 path_difference_data.append(
 a_path_len - b_path_len)
 
-res.add_common(a)
-old.remove(a)
-new.remove(b)
+res.add_common(b)
+common.add(a)
+
+old = filter_issues(old, common)
+new = filter_issues(new, common)
+common = set()
 
-for a in copy(old):
-for b in copy(new):
+for a in old:
+for b in new:
 if a.is_similar_to(b):
 res.add_changed(a, b)
-old.remove(a)
-new.remove(b)
+common.add(a)
+common.add(b)
+
+old = filter_issues(old, common)
+new = filter_issues(new, common)
 
 # Whatever is left in 'old' doesn't have a corresponding diagnostic
 # in 'new', so we need to mark it as 'removed'.
@@ -443,6 +450,12 @@
 return res
 
 
+def filter_issues(origin: List[AnalysisDiagnostic],
+  to_remove: Set[AnalysisDiagnostic]) \
+  -> List[AnalysisDiagnostic]:
+return [diag for diag in origin if diag not in to_remove]
+
+
 def compute_percentile(values: Sequence[T], percentile: float) -> T:
 """
 Return computed percentile.


Index: clang/utils/analyzer/CmpRuns.py
===
--- clang/utils/analyzer/CmpRuns.py
+++ clang/utils/analyzer/CmpRuns.py
@@ -36,7 +36,7 @@
 from copy import copy
 from enum import Enum
 from typing import (Any, DefaultDict, Dict, List, NamedTuple, Optional,
-Sequence, TextIO, TypeVar, Tuple, Union)
+Sequence, Set, TextIO, TypeVar, Tuple, Union)
 
 
 Number = Union[int, float]
@@ -374,8 +374,9 @@
 
 # Quadratic algorithms in this part are fine because 'old' and 'new'
 # are most commonly of size 1.
-for a in copy(old):
-for b in copy(new):
+common: Set[AnalysisDiagnostic] = set()
+for a in old:
+for b in new:
 if a.get_issue_identifier() == b.get_issue_identifier():
 a_path_len = a.get_path_length()
 b_path_len = b.get_path_length()
@@ -394,16 +395,22 @@
 path_difference_data.append(
 a_path_len - b_path_len)
 
-res.add_common(a)
-old.remove(a)
-new.remove(b)
+res.add_common(b)
+common.add(a)
+
+old = filter_issues(old, common)
+new = filter_issues(new, common)
+common = set()
 
-for a in copy(old):
-for b in copy(new):
+for a in old:
+for b in new:
 if a.is_similar_to(b):
 res.add_changed(a, b)
-old.remove(a)
-new.remove(b)
+common.add(a)
+common.add(b)
+
+old = filter_issues(old, common)
+new = filter_issues(new, common)
 
 # Whatever is left in 'old' doesn't have a corresponding diagnostic
 # in 'new', so we need to mark it as 'removed'.
@@ -443,6 +450,12 @@
 return res
 
 
+def filter_issues(origin: List[AnalysisDiagnostic],
+  to_remove: Set[AnalysisDiagnostic]) \
+  -> List[AnalysisDiagnostic]:
+return [diag for diag in origin if diag not in to_remove]
+
+
 def compute_percentile(values: Sequence[T], percentile: float) -> T:
 """
 

[clang] 94a1a5d - [analyzer][tests] Fix issue comparison script

[Valeriy Savchenko via cfe-commits]

Author: Valeriy Savchenko
Date: 2021-02-13T13:58:47+03:00
New Revision: 94a1a5d25f5546019fc5feeb24d924b09b9d27e4

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

LOG: [analyzer][tests] Fix issue comparison script

When newer build has duplicate issues the script tried to
remove it from the list more than once.  The new approach
changes the way we filter out matching issues.

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

Added: 


Modified: 
clang/utils/analyzer/CmpRuns.py

Removed: 




diff  --git a/clang/utils/analyzer/CmpRuns.py b/clang/utils/analyzer/CmpRuns.py
index 9d5e00767067..7afe865d77f2 100644
--- a/clang/utils/analyzer/CmpRuns.py
+++ b/clang/utils/analyzer/CmpRuns.py
@@ -36,7 +36,7 @@
 from copy import copy
 from enum import Enum
 from typing import (Any, DefaultDict, Dict, List, NamedTuple, Optional,
-Sequence, TextIO, TypeVar, Tuple, Union)
+Sequence, Set, TextIO, TypeVar, Tuple, Union)
 
 
 Number = Union[int, float]
@@ -374,8 +374,9 @@ def compare_results(results_old: AnalysisRun, results_new: 
AnalysisRun,
 
 # Quadratic algorithms in this part are fine because 'old' and 'new'
 # are most commonly of size 1.
-for a in copy(old):
-for b in copy(new):
+common: Set[AnalysisDiagnostic] = set()
+for a in old:
+for b in new:
 if a.get_issue_identifier() == b.get_issue_identifier():
 a_path_len = a.get_path_length()
 b_path_len = b.get_path_length()
@@ -394,16 +395,22 @@ def compare_results(results_old: AnalysisRun, 
results_new: AnalysisRun,
 path_
diff erence_data.append(
 a_path_len - b_path_len)
 
-res.add_common(a)
-old.remove(a)
-new.remove(b)
+res.add_common(b)
+common.add(a)
+
+old = filter_issues(old, common)
+new = filter_issues(new, common)
+common = set()
 
-for a in copy(old):
-for b in copy(new):
+for a in old:
+for b in new:
 if a.is_similar_to(b):
 res.add_changed(a, b)
-old.remove(a)
-new.remove(b)
+common.add(a)
+common.add(b)
+
+old = filter_issues(old, common)
+new = filter_issues(new, common)
 
 # Whatever is left in 'old' doesn't have a corresponding diagnostic
 # in 'new', so we need to mark it as 'removed'.
@@ -443,6 +450,12 @@ def compare_results(results_old: AnalysisRun, results_new: 
AnalysisRun,
 return res
 
 
+def filter_issues(origin: List[AnalysisDiagnostic],
+  to_remove: Set[AnalysisDiagnostic]) \
+  -> List[AnalysisDiagnostic]:
+return [diag for diag in origin if diag not in to_remove]
+
+
 def compute_percentile(values: Sequence[T], percentile: float) -> T:
 """
 Return computed percentile.



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

[libunwind] f042fd4 - [libunwind][cmake] Add an option to enable/disable tests

[Kristina Bessonova via cfe-commits]

Author: Kristina Bessonova
Date: 2021-02-13T12:49:48+02:00
New Revision: f042fd46b527bea97dfa1e09558700173b70405e

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

LOG: [libunwind][cmake] Add an option to enable/disable tests

Reviewed By: ldionne, compnerd

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

Added: 


Modified: 
libunwind/CMakeLists.txt

Removed: 




diff  --git a/libunwind/CMakeLists.txt b/libunwind/CMakeLists.txt
index 05131bd77b39..140700030aff 100644
--- a/libunwind/CMakeLists.txt
+++ b/libunwind/CMakeLists.txt
@@ -63,6 +63,7 @@ option(LIBUNWIND_ENABLE_THREADS "Build libunwind with 
threading support." ON)
 option(LIBUNWIND_WEAK_PTHREAD_LIB "Use weak references to refer to pthread 
functions." OFF)
 option(LIBUNWIND_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 option(LIBUNWIND_INCLUDE_DOCS "Build the libunwind documentation." 
${LLVM_INCLUDE_DOCS})
+option(LIBUNWIND_INCLUDE_TESTS "Build the libunwind tests." 
${LLVM_INCLUDE_TESTS})
 option(LIBUNWIND_IS_BAREMETAL "Build libunwind for baremetal targets." OFF)
 option(LIBUNWIND_USE_FRAME_HEADER_CACHE "Cache frame headers for unwinding. 
Requires locking dl_iterate_phdr." OFF)
 option(LIBUNWIND_REMEMBER_HEAP_ALLOC "Use heap instead of the stack for 
.cfi_remember_state." OFF)
@@ -344,6 +345,6 @@ if (LIBUNWIND_INCLUDE_DOCS)
   add_subdirectory(docs)
 endif()
 
-if (EXISTS ${LLVM_CMAKE_PATH})
+if (LIBUNWIND_INCLUDE_TESTS AND EXISTS ${LLVM_CMAKE_PATH})
   add_subdirectory(test)
 endif()



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

[clang] 39db16e - [test] Make ELF tests less reliant on the lexicographical order of non-local symbols

[Fangrui Song via cfe-commits]

Author: Fangrui Song
Date: 2021-02-13T01:01:06-08:00
New Revision: 39db16e75bd85f9c85a9df1c92a92920006b31b3

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

LOG: [test] Make ELF tests less reliant on the lexicographical order of 
non-local symbols

Added: 


Modified: 
clang/test/InterfaceStubs/hidden-class-inheritance.cpp
llvm/test/CodeGen/AMDGPU/code-object-v3.ll
llvm/test/CodeGen/PowerPC/pcrel-tls-general-dynamic.ll
llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll
llvm/test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll

Removed: 




diff  --git a/clang/test/InterfaceStubs/hidden-class-inheritance.cpp 
b/clang/test/InterfaceStubs/hidden-class-inheritance.cpp
index 2219fd5b2e8a..b0a039c46f3e 100644
--- a/clang/test/InterfaceStubs/hidden-class-inheritance.cpp
+++ b/clang/test/InterfaceStubs/hidden-class-inheritance.cpp
@@ -57,14 +57,14 @@
 // CHECK-X-DAG: _ZN1C1mEv
 // CHECK-X-DAG: _ZN1S1nEv
 
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1C1mEv
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1CC2Ev
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1CD0Ev
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1CD2Ev
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1S1nEv
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1SC2Ev
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1SD0Ev
-// CHECK-X-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1SD2Ev
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1C1mEv
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1CC2Ev
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1CD0Ev
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1CD2Ev
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1S1nEv
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1SC2Ev
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1SD0Ev
+// CHECK-X-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1SD2Ev
 
 // CHECK-HP2-DAG: _ZN1CC2Ev
 // CHECK-HP2-DAG: _ZN1CD0Ev
@@ -76,14 +76,14 @@
 // CHECK-HP-NOT: _ZN1SD0Ev
 // CHECK-HP-NOT: _ZN1SD2Ev
 
-// CHECK-HP-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1C1mEv
-// CHECK-HP-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1CC2Ev
-// CHECK-HP-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1CD0Ev
-// CHECK-HP-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1CD2Ev
-// CHECK-HP-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1S1nEv
-// CHECK-HP-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1SC2Ev
-// CHECK-HP-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1SD0Ev
-// CHECK-HP-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1SD2Ev
+// CHECK-HP-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1C1mEv
+// CHECK-HP-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1CC2Ev
+// CHECK-HP-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1CD0Ev
+// CHECK-HP-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1CD2Ev
+// CHECK-HP-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1S1nEv
+// CHECK-HP-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1SC2Ev
+// CHECK-HP-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1SD0Ev
+// CHECK-HP-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1SD2Ev
 
 // CHECK-HC2-DAG: _ZN1SC2Ev
 // CHECK-HC2-DAG: _ZN1SD0Ev
@@ -95,14 +95,14 @@
 // CHECK-HC-NOT: _ZN1CD0Ev
 // CHECK-HC-NOT: _ZN1CD2Ev
 
-// CHECK-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1C1mEv
-// CHECK-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1CC2Ev
-// CHECK-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1CD0Ev
-// CHECK-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1CD2Ev
-// CHECK-HC-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1S1nEv
-// CHECK-HC-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1SC2Ev
-// CHECK-HC-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1SD0Ev
-// CHECK-HC-RE: FUNCWEAK   DEFAULT   {{[0-9]+}} _ZN1SD2Ev
+// CHECK-HC-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1C1mEv
+// CHECK-HC-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1CC2Ev
+// CHECK-HC-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1CD0Ev
+// CHECK-HC-RE-DAG: FUNCWEAK   HIDDEN[[#]] _ZN1CD2Ev
+// CHECK-HC-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1S1nEv
+// CHECK-HC-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1SC2Ev
+// CHECK-HC-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1SD0Ev
+// CHECK-HC-RE-DAG: FUNCWEAK   DEFAULT   [[#]] _ZN1SD2Ev
 
 // CHECK-HP-HC-NOT: _ZN1CC2Ev
 // CHECK-HP-HC-NOT: _ZN1CD0Ev
@@ -113,14 +113,14 @@
 // CHECK-HP-HC-NOT: _ZN1C1mEv
 // CHECK-HP-HC-NOT: _ZN1S1nEv
 
-// CHECK-HP-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1C1mEv
-// CHECK-HP-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1CC2Ev
-// CHECK-HP-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1CD0Ev
-// CHECK-HP-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1CD2Ev
-// CHECK-HP-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1S1nEv
-// CHECK-HP-HC-RE: FUNCWEAK   HIDDEN{{[0-9]+}} _ZN1SC2Ev
-// CHECK-HP-HC-RE: FUNC  

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908
 
+CodeGenerator *CodeGenAction::getCodeGenerator() const {
+  return BEConsumer->getCodeGenerator();

@rjmccall, we were wondering if there is a better way to ask CodeGen to start a 
new module. The current approach seems to be drilling hole in a number of 
abstraction layers.

In the past we have touched that area a little in 
https://reviews.llvm.org/D3 and the answer may be already there but I fail 
to connect the dots.

Recently, we thought about having a new FrontendAction callback for beginning a 
new phase when compiling incremental input. We need to keep track of the 
created objects (needed for error recovery) in our Transaction. We can have a 
map of `Transaction*` to `llvm::Module*` in CodeGen. The issue is that new JITs 
take ownership of the `llvm::Module*` which seems to make it impossible to 
support jitted code removal with that model (cc: @lhames, @rsmith).


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

https://reviews.llvm.org/D96033

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.



Comment at: clang/include/clang/Frontend/FrontendAction.h:238
   /// objects, and run statistics and output file cleanup code.
-  void EndSourceFile();
+  virtual void EndSourceFile();
 

teemperor wrote:
> I think this and the change above requires updating `WrapperFrontendAction` 
> to also forward these functions to the wrapped action. Otherwise wrapping 
> actions would change their behaviour. See the other virtual functions there.
Done. Ideally I would like to avoid making these routines virtual. However, 
here we call `EndSourceFile` 
https://github.com/llvm/llvm-project/blob/1f6ec3d08f75dba6c93c291bd92552b807736eb3/clang/lib/Frontend/CompilerInstance.cpp#L952
 and this shuts down some of the objects required by the IncrementalAction.

In fact, after some checking, it does not make sense to allow overriding 
`BeginSourceFile` -- it initializes state of the `Action` class and does not 
make sense to forward it for instance in the `WrapperFrontendAction`. In 
principle, clients may be interested in initialize differently but for 
clang-repl we only need the `EndSourceFile` to be virtual.

Alternatively, we could somehow make the Inputs to take an "external" iterator 
(https://github.com/llvm/llvm-project/blob/1f6ec3d08f75dba6c93c291bd92552b807736eb3/clang/lib/Frontend/CompilerInstance.cpp#L942)
 which will provide us with a hook to treat each input line as a separate Input 
file. AFAICT, that has two disadvantages: 1) we will initialize/finalize more 
state of various objects which can hinder performance; 2) each input file is 
considered as a separate translation unit (TU) which clashes with the concept 
we introduce of an ever growing single TU.



Comment at: clang/include/clang/Interpreter/Transaction.h:1
+//===--- Transaction.h - Incremental Compilation and Execution---*- C++ 
-*-===//
+//

teemperor wrote:
> v.g.vassilev wrote:
> > teemperor wrote:
> > > Could this whole file just be part of `IncrementalParser.h` which is the 
> > > only user ? `clang::Transaction` seems anyway a bit of a generic name, so 
> > > maybe this could become `clang::IncrementalParser::Transaction` then.
> > The intent is to expose the Transaction class and if I move it in the 
> > `IncrementalParser` I will have to expose the `IncrementalParser.h`. Would 
> > it make sense to move it as part of the Interpreter object? Eg. 
> > `clang::Interpreter::Transaction`?
> Makes sense. I guess moving this into the Interpreter would create some 
> strange dependencies, so I would say we keep it in its own file. Maybe we 
> could put it into a `interpreter` namespace or give it a more unique name 
> `InterpreterTransaction`?
> 
> I also think this should have some documentation.
I added some documentation.

Having a long name for the Transaction class will make the future code clunky. 
This class will be extensively used throughout the interpreter codebase and 
api. I'd be in favor of going for a namespace but then I think we will have 
confusing naming for say `clang::interpreter::Interpreter`...



Comment at: clang/lib/Interpreter/IncrementalExecutor.h:36
+  llvm::Error addModule(std::unique_ptr M);
+  llvm::Error runCtors() const;
+};

teemperor wrote:
> Should we maybe merge `runCtors` and `addModule`? Not sure if there is a use 
> case for adding a Module but not running Ctors. Also documentation.
The case we have is when there is no JIT -- currently we have such a case in 
IncrementalProcessingTest I think. Another example, which will show up in 
future patches, is the registration of atexit handlers. That is, before we 
`runCtors` we make a pass over the LLVM IR and collect some specific details 
and (possibly change the IR and then run).

I'd rather keep it separate for now if that's okay.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:63
+if (CI.hasCodeCompletionConsumer())
+  CompletionConsumer = ();
+

teemperor wrote:
> Can this completion code even be used? It doesn't look like it can (and I'm 
> not sure if using the `CodeCompletionAt` flag is really useful in a REPL as 
> you can only specify it once during startup). IMHO this can be left out until 
> we actually can hook up this into the LineEditor (and we have a way to test 
> this).
Cling, on which this patch is based on, has a CodeCompleteConsumer and it seems 
to work.

I can leave it out but then we will have to remember where to put it. I have a 
slight preference to leave it as it is.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:69
+Preprocessor  = CI.getPreprocessor();
+PP.enableIncrementalProcessing();
+PP.EnterMainSourceFile();

teemperor wrote:
> Could we do that before creating the Sema? Sema's constructor is doing quite 
> a few things and some of them might depend on this setting in the future.
Indeed, should 

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev updated this revision to Diff 323531.
v.g.vassilev marked 21 inline comments as done.
v.g.vassilev added a comment.

Address more review comments.


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

https://reviews.llvm.org/D96033

Files:
  clang/include/clang/CodeGen/CodeGenAction.h
  clang/include/clang/Frontend/FrontendAction.h
  clang/include/clang/Interpreter/Interpreter.h
  clang/include/clang/Interpreter/Transaction.h
  clang/lib/CMakeLists.txt
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Interpreter/CMakeLists.txt
  clang/lib/Interpreter/IncrementalExecutor.cpp
  clang/lib/Interpreter/IncrementalExecutor.h
  clang/lib/Interpreter/IncrementalParser.cpp
  clang/lib/Interpreter/IncrementalParser.h
  clang/lib/Interpreter/Interpreter.cpp
  clang/test/CMakeLists.txt
  clang/test/Interpreter/execute.c
  clang/test/Interpreter/sanity.c
  clang/tools/CMakeLists.txt
  clang/tools/clang-repl/CMakeLists.txt
  clang/tools/clang-repl/ClangRepl.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/CodeGen/CMakeLists.txt
  clang/unittests/CodeGen/IncrementalProcessingTest.cpp
  clang/unittests/Interpreter/CMakeLists.txt
  clang/unittests/Interpreter/InterpreterTest.cpp

Index: clang/unittests/Interpreter/InterpreterTest.cpp
===
--- /dev/null
+++ clang/unittests/Interpreter/InterpreterTest.cpp
@@ -0,0 +1,92 @@
+//===- unittests/Interpreter/InterpreterTest.cpp --- Interpreter tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Unit tests for Clang's Interpreter library.
+//
+//===--===//
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Interpreter/Interpreter.h"
+
+#include "llvm/ADT/ArrayRef.h"
+
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+static std::unique_ptr createInterpreter() {
+  std::vector ClangArgs = {"-Xclang", "-emit-llvm-only"};
+  auto CI = cantFail(clang::IncrementalCompilerBuilder::create(ClangArgs));
+  return std::move(cantFail(clang::Interpreter::create(std::move(CI;
+}
+
+TEST(InterpreterTest, Sanity) {
+  std::unique_ptr Interp = createInterpreter();
+  Transaction (cantFail(Interp->Parse("void g(); void g() {}")));
+  EXPECT_EQ(2U, R1.Decls.size());
+
+  Transaction (cantFail(Interp->Parse("int i;")));
+  EXPECT_EQ(1U, R2.Decls.size());
+}
+
+static std::string DeclToString(DeclGroupRef DGR) {
+  return llvm::cast(DGR.getSingleDecl())->getQualifiedNameAsString();
+}
+
+TEST(InterpreterTest, IncrementalInputTopLevelDecls) {
+  std::unique_ptr Interp = createInterpreter();
+  auto R1OrErr = Interp->Parse("int var1 = 42; int f() { return var1; }");
+  // gtest doesn't expand into explicit bool conversions.
+  EXPECT_TRUE(!!R1OrErr);
+  auto R1 = R1OrErr->Decls;
+  EXPECT_EQ(2U, R1.size());
+  EXPECT_EQ("var1", DeclToString(R1[0]));
+  EXPECT_EQ("f", DeclToString(R1[1]));
+
+  auto R2OrErr = Interp->Parse("int var2 = f();");
+  EXPECT_TRUE(!!R2OrErr);
+  auto R2 = R2OrErr->Decls;
+  EXPECT_EQ(1U, R2.size());
+  EXPECT_EQ("var2", DeclToString(R2[0]));
+}
+
+
+TEST(InterpreterTest, Errors) {
+  std::unique_ptr Interp = createInterpreter();
+  auto Err = Interp->Parse("intentional_error v1 = 42; ").takeError();
+  EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err)));
+
+  EXPECT_DEATH((void)Interp->Parse("int var1 = 42;"), "");
+}
+
+// Here we test whether the user can mix declarations and statements. The
+// interpreter should be smart enough to recognize the declarations from the
+// statements and wrap the latter into a declaration, producing valid code.
+TEST(InterpreterTest, DeclsAndStatements) {
+  std::unique_ptr Interp = createInterpreter();
+  auto R1OrErr = Interp->Parse(
+  "int var1 = 42; extern \"C\" int printf(const char*, ...);");
+  // gtest doesn't expand into explicit bool conversions.
+  EXPECT_TRUE(!!R1OrErr);
+
+  auto R1 = R1OrErr->Decls;
+  EXPECT_EQ(2U, R1.size());
+
+  // FIXME: Add support for wrapping and running statements.
+  auto R2OrErr =
+Interp->Parse("var1++; printf(\"var1 value is %d\\n\", var1);");
+  EXPECT_FALSE(!!R2OrErr);
+  auto Err = R2OrErr.takeError();
+  EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err)));
+}
+
+} // end anonymous namespace
Index: clang/unittests/Interpreter/CMakeLists.txt
===
--- /dev/null
+++ clang/unittests/Interpreter/CMakeLists.txt
@@ -0,0 +1,10 @@
+set(LLVM_LINK_COMPONENTS
+  )
+
+add_clang_unittest(ClangReplInterpreterTests
+  InterpreterTest.cpp
+  )