date:20190825

[PATCH] D64671: [clang-tidy] New check: misc-init-local-variables

2019-08-25 Thread Jussi Pakkanen via Phabricator via cfe-commits

jpakkane added a comment.

I used stdint to replicate a real world use case as I'd imagine those types 
would match this search quite heavily.

The tests already have one test for a typedeffed integer and one that is 
defined with a macro. If those are deemed sufficient, the stdint type can be 
removed altogether. If the stdint types are defined via some other mechanism 
such as compiler intrinsics, there would be no test for that. I do not know if 
that is the case with current libcs.

I can update the MR as requested once someone with enough stripes makes the 
policy decision.


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

https://reviews.llvm.org/D64671



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

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

2019-08-25 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

Another bug found by -Wxor-used-as-pow :)
https://github.com/christinaa/PcapPlusPlus/commit/4646a004f5168bcb78fe2dce78afa08d794c1450


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

https://reviews.llvm.org/D66397



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

[PATCH] D66646: Ensure that statements, expressions and types are trivially destructible with a static_assert

2019-08-25 Thread Bruno Ricci via Phabricator via cfe-commits

riccibruno added a comment.

> I was looking at SpecificBumpPtrAllocator, which knows it's type.
>  But if i look down the indirection, i think 
> AllocatorBase::Allocate()/AllocatorBase::Deallocate() is the place.

I did look at that, but I think it won't work for two reasons:

1. AST nodes are very often allocated with some trailing objects and so the 
typed version of `Allocate` is not used.
2. It would be difficult to add exceptions as done in this patch. The two 
exceptions I added are (I think!) latent bugs, but they seem to be relatively 
harmless (for `ConstantArrayType` you would need to have an array with a size 
which do not fit inline in the `APInt`, and then it would only cause a leak).

In time it would be nice to just remove the two exceptions, but then point 1 
still apply.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66646



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

[PATCH] D66646: Ensure that statements, expressions and types are trivially destructible with a static_assert

2019-08-25 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri accepted this revision.
lebedev.ri added a subscriber: rsmith.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

SGTM but please wait a bit in case @aaron.ballman / @rsmith wants to comment.
Thanks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66646



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

[PATCH] D66714: [analyzer] Don't run the analyzer for -analyzer-list-enabled-checkers

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus created this revision.
Szelethus added reviewers: NoQ, dcoughlin, baloghadamsoftware, Charusso, 
rnkovacs, xazax.hun.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

Short and sweet. Whenever I use `-analyzer-list-enabled-checkers`, I'm only 
interested about the configuration, not about the analysis.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66714

Files:
  clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp


Index: clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
===
--- clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
+++ clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
@@ -272,6 +272,7 @@
   AnOpts,
   Clang->getDiagnostics(),
   Clang->getLangOpts());
+return true;
   }
 
   // Honor -analyzer-config-help.


Index: clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
===
--- clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
+++ clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
@@ -272,6 +272,7 @@
   AnOpts,
   Clang->getDiagnostics(),
   Clang->getLangOpts());
+return true;
   }
 
   // Honor -analyzer-config-help.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

2019-08-25 Thread Chris Hamilton via Phabricator via cfe-commits

chrish_ericsson_atx added a comment.

@Szelethus, firstly, thank you for landing this change. I really appreciate it! 
  Secondly, apologies for misspelling your name earlier.  And lastly, thank you 
for your patient explanation of how to get the diffs updated correctly in a 
Phabricator review.  I think my mistake was that I made the change and the 
updates updates in a private branch, and not directly off master, and then 
duplicated the changes on master.  For one of those sets of changes, I amended 
the first commit with the second round of changes, and I think I confused 
myself by doing that.  In any case, lesson learned, and thank you again for 
your coaching!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014



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

[PATCH] D66715: [CFG] Add dumps for CFGElement and CFGElementRef

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, dcoughlin, rnkovacs, Charusso, 
baloghadamsoftware.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp.

Seems like we never had these, so here we go! I also did some refactoring as I 
was chasing a bug unrelated to this revision.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66715

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/unittests/Analysis/CFGTest.cpp

Index: clang/unittests/Analysis/CFGTest.cpp
===
--- clang/unittests/Analysis/CFGTest.cpp
+++ clang/unittests/Analysis/CFGTest.cpp
@@ -67,6 +67,46 @@
   expectLinear(true,  "void foo() { foo(); }"); // Recursion is not our problem.
 }
 
+TEST(CFG, ElementDump) {
+  const char *Code = R"(void f() {
+  int i;
+  int j;
+  i = 5;
+  i = 6;
+  j = 7;
+})";
+
+  BuildResult B = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus());
+  CFG *Cfg = B.getCFG();
+
+  // [B2 (ENTRY)]
+  //   Succs (1): B1
+
+  // [B1]
+  //   1: int i;
+  //   2: int j;
+  //   3: i = 5
+  //   4: i = 6
+  //   5: j = 7
+  //   Preds (1): B2
+  //   Succs (1): B0
+
+  // [B0 (EXIT)]
+  //   Preds (1): B1
+  CFGBlock *MainBlock = *(Cfg->begin() + 1);
+
+  llvm::SmallString<48> DumpBuffer;
+  llvm::raw_svector_ostream OS(DumpBuffer);
+  (*MainBlock)[0].dumpToStream(OS);
+  EXPECT_EQ("int i;\n", DumpBuffer);
+
+  DumpBuffer.clear();
+
+  (*MainBlock->ref_begin()).dumpToStream(OS);
+  EXPECT_EQ("1: int i;\n", DumpBuffer);
+}
+
 TEST(CFG, ElementRefIterator) {
   const char *Code = R"(void f() {
   int i;
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -4965,6 +4965,8 @@
 public:
   StmtPrinterHelper(const CFG* cfg, const LangOptions &LO)
   : LangOpts(LO) {
+if (!cfg)
+  return;
 for (CFG::const_iterator I = cfg->begin(), E = cfg->end(); I != E; ++I ) {
   unsigned j = 1;
   for (CFGBlock::const_iterator BI = (*I)->begin(), BEnd = (*I)->end() ;
@@ -5286,10 +5288,22 @@
 }
 }
 
+static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
+   const CFGElement &E);
+
+void CFGElement::dumpToStream(llvm::raw_ostream &OS) const {
+  StmtPrinterHelper Helper(nullptr, {});
+  print_elem(OS, Helper, *this);
+}
+
 static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper,
const CFGElement &E) {
-  if (Optional CS = E.getAs()) {
-const Stmt *S = CS->getStmt();
+  switch (E.getKind()) {
+  case CFGElement::Kind::Statement:
+  case CFGElement::Kind::CXXRecordTypedCall:
+  case CFGElement::Kind::Constructor: {
+CFGStmt CS = E.castAs();
+const Stmt *S = CS.getStmt();
 assert(S != nullptr && "Expecting non-null Stmt");
 
 // special printing for statement-expressions.
@@ -5341,12 +5355,18 @@
 // Expressions need a newline.
 if (isa(S))
   OS << '\n';
-  } else if (Optional IE = E.getAs()) {
-print_initializer(OS, Helper, IE->getInitializer());
+
+break;
+  }
+
+  case CFGElement::Kind::Initializer:
+print_initializer(OS, Helper, E.castAs().getInitializer());
 OS << '\n';
-  } else if (Optional DE =
- E.getAs()) {
-const VarDecl *VD = DE->getVarDecl();
+break;
+
+  case CFGElement::Kind::AutomaticObjectDtor: {
+CFGAutomaticObjDtor DE = E.castAs();
+const VarDecl *VD = DE.getVarDecl();
 Helper.handleDecl(VD, OS);
 
 ASTContext &ACtx = VD->getASTContext();
@@ -5358,53 +5378,76 @@
 
 OS << ".~" << T->getAsCXXRecordDecl()->getName().str() << "()";
 OS << " (Implicit destructor)\n";
-  } else if (Optional DE = E.getAs()) {
-const VarDecl *VD = DE->getVarDecl();
-Helper.handleDecl(VD, OS);
+break;
+  }
+
+  case CFGElement::Kind::LifetimeEnds:
+Helper.handleDecl(E.castAs().getVarDecl(), OS);
 
 OS << " (Lifetime ends)\n";
-  } else if (Optional LE = E.getAs()) {
-const Stmt *LoopStmt = LE->getLoopStmt();
-OS << LoopStmt->getStmtClassName() << " (LoopExit)\n";
-  } else if (Optional SB = E.getAs()) {
+break;
+
+  case CFGElement::Kind::LoopExit:
+OS << E.castAs().getLoopStmt()->getStmtClassName() << " (LoopExit)\n";
+break;
+
+  case CFGElement::Kind::ScopeBegin:
 OS << "CFGScopeBegin(";
-if (const VarDecl *VD = SB->getVarDecl())
+if (const VarDecl *VD = E.castAs().getVarDecl())
   OS << VD->getQualifiedNameAsString();
 OS << ")\n";
-  } else if (Optional SE = E.getAs()) {
+break;
+
+  case CFGElement::Kind::ScopeEnd:
 OS << "CFGScopeEnd(";
-if (const VarDecl *VD = SE->getVarDecl())
+if (const VarDecl *VD = E.castAs().getVarDec

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, Charusso, 
dcoughlin, rnkovacs, TWeaver.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

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

In today's edition of "Is this any better now that it isn't crashing?", I'd 
like to show you a very interesting test case with loop widening.

Looking at the included test case, it's immediately obvious that this is not 
only a false positive, but also a very bad bug report in general. We can see 
how the analyzer mistakenly invalidated `b`, instead of its pointee, resulting 
in it reporting a null pointer dereference error. Not only that, the point at 
which this change of value is noted at is at the loop, rather then at the 
method call.

It turns out that `FindLastStoreVisitor` works correctly, rather the supplied 
explodedgraph is faulty, because `BlockEdge` really is the `ProgramPoint` where 
this happens.
F9855739: image.png 
So it's fair to say that this needs improving on multiple fronts. In any case, 
at least the crash is gone.

Full ExplodedGraph: F9855743: loop.dot 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66716

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/loop-widening.cpp


Index: clang/test/Analysis/loop-widening.cpp
===
--- /dev/null
+++ clang/test/Analysis/loop-widening.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config widen-loops=true \
+// RUN:   -analyzer-config track-conditions=false \
+// RUN:   -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+  void m_fn1();
+};
+bool g;
+void fn1() {
+  A a;
+  A *b = &a;
+
+  for (;;) { // expected-note{{Loop condition is true.  Entering loop body}}
+ // expected-note@-1{{Loop condition is true.  Entering loop body}}
+ // expected-note@-2{{Value assigned to 'b'}}
+ // no crash during bug report construction
+
+g = !b; // expected-note{{Assuming 'b' is null}}
+b->m_fn1(); // expected-warning{{Called C++ object pointer is null}}
+// expected-note@-1{{Called C++ object pointer is null}}
+  }
+}
+} // end of namespace pr43102
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -761,14 +761,18 @@
 return PathDiagnosticLocation(
 CEB->getLocationContext()->getDecl()->getSourceRange().getEnd(), SMng);
   } else if (Optional BE = P.getAs()) {
-CFGElement BlockFront = BE->getBlock()->front();
-if (auto StmtElt = BlockFront.getAs()) {
-  return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
-} else if (auto NewAllocElt = BlockFront.getAs()) {
-  return PathDiagnosticLocation(
-  NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+if (Optional BlockFront = BE->getFirstElement()) {
+  if (auto StmtElt = BlockFront->getAs()) {
+return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
+  } else if (auto NewAllocElt = BlockFront->getAs()) {
+return PathDiagnosticLocation(
+NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+  }
+  llvm_unreachable("Unexpected CFG element at front of block");
 }
-llvm_unreachable("Unexpected CFG element at front of block");
+
+return PathDiagnosticLocation(
+BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
   } else if (Optional FE = P.getAs()) {
 return PathDiagnosticLocation(FE->getStmt(), SMng,
   FE->getLocationContext());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1441,6 +1441,7 @@
 
   if (!StoreSite)
 return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1802,7 +1803,7 @@
   if (ControlDeps.isControlDependent(OriginB, NB)) {
 // We don't really want to explain for range loops. Evidence suggests that
 // the only thing that leads to is the addition of calls to operator!=.
-if (isa(NB->getTerminator()))
+if (llvm::isa_and_nonnull(NB->getTerminatorStmt()))
   return nullptr;
 
 if (const Expr *Condition = NB->getLastCondition()) {


Index: clang/test/Analysis/loop-widening.cpp

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:75
 /// individual bug reports.
 class BugReport : public llvm::ilist_node {
 public:

Shouldn't we make this an abstract class?


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

https://reviews.llvm.org/D66572



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

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

2019-08-25 Thread Dávid Bolvanský via Phabricator via cfe-commits

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

Looks fine


Repository:
  rC Clang

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

https://reviews.llvm.org/D66711



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

[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

2019-08-25 Thread Aleksei Sidorin via Phabricator via cfe-commits

a_sidorin added a comment.

Hello Balasz,
I have some comments inline.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1579
+  D2 = D2->getCanonicalDecl();
+  std::pair P = std::make_pair(D1, D2);
+

`std::pair P{D1, D2}`?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1888
 
-Decl *D2 = TentativeEquivalences[D1];
-assert(D2 && "Unrecorded tentative equivalence?");
+Decl *D1 = P.first;
+Decl *D2 = P.second;

I would prefer std::tie instead: `std::tie(D1, D2) = P;`. But it is a matter of 
taste.



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1290
+  bool isInNonEqCache(std::tuple D) {
+return NonEquivalentDecls.find(std::make_pair(get<0>(D), get<1>(D))) !=
+   NonEquivalentDecls.end();

Can we use std::pair instead of std::tuple in this class? The size of tuple is 
known to be 2 and it will help to avoid such conversions.



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1291
+return NonEquivalentDecls.find(std::make_pair(get<0>(D), get<1>(D))) !=
+   NonEquivalentDecls.end();
+  }

`return NonEquivalentDecls.count({get<0>(D), get<1>(D)});`?



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1314
+  bool Eq = Ctx.IsEquivalent(get<0>(X), get<1>(X));
+  EXPECT_FALSE(Eq);
+

It seems to me that the assertion will become a bit easier to read without an 
intermediate variable. What do you think?



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1357
+
+  EXPECT_FALSE(isInNonEqCache(C));
+  EXPECT_FALSE(isInNonEqCache(findDeclPair(

The declarations of C are not equivalent, but they are not placed into the 
non-equivalence cache. This looks strange to me, could you please explain this 
behavior?



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1399
+  TU, cxxRecordDecl(hasName("A"), unless(isImplicit());
+  EXPECT_FALSE(isInNonEqCache(
+  findDeclPair(TU, functionDecl(hasName("x");

We don't have any tests where isInNonEqCache() can return true. Is it expected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66538



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

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

2019-08-25 Thread Arthur O'Dwyer via Phabricator via cfe-commits

Quuxplusone added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2204
   "abstract class is marked '%select{final|sealed}0'">, 
InGroup;
+def warn_final_destructor_nonfinal_class : Warning<
+  "class with destructor marked '%select{final|sealed}0' cannot be inherited 
from">,

Is there any good reason to spell it `dtor` in the preceding file but 
`destructor` in this one? I think the spelling should be consistent one way or 
the other. Helps with greppability/maintainability.



Comment at: clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp:12
+class C final {
+virtual ~C() final;
+};

Should there be a test for the `sealed` spelling?


Repository:
  rC Clang

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

https://reviews.llvm.org/D66711



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

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

2019-08-25 Thread Logan Smith via Phabricator via cfe-commits

logan-5 updated this revision to Diff 217049.
logan-5 added a comment.

Addressed some feedback.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66711

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/MicrosoftExtensions.cpp
  clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp


Index: clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s 
-Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 
'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};
Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -475,6 +475,11 @@
 // expected-error@+1 {{base 'SealedType' is marked 'sealed'}}
 struct InheritFromSealed : SealedType {};
 
+class SealedDestructor { // expected-note {{mark 'SealedDestructor' as 
'sealed' to silence this warning}}
+// expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}
+virtual ~SealedDestructor() sealed; // expected-warning {{class with 
destructor marked 'sealed' cannot be inherited from}}
+};
+
 void AfterClassBody() {
   // expected-warning@+1 {{attribute 'deprecated' is ignored, place it after 
"struct" to apply attribute to type declaration}}
   struct D {} __declspec(deprecated);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6236,6 +6236,19 @@
 }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr()) {
+if (const CXXDestructorDecl *dtor = Record->getDestructor()) {
+  if (const FinalAttr *FA = dtor->getAttr()) {
+Diag(FA->getLocation(), diag::warn_final_dtor_non_final_class)
+  << FA->isSpelledAsSealed();
+Diag(Record->getLocation(), 
diag::note_final_dtor_non_final_class_silence)
+  << Context.getRecordType(Record)
+  << FA->isSpelledAsSealed();
+  }
+}
+  }
+
   // See if trivial_abi has to be dropped.
   if (Record->hasAttr())
 checkIllFormedTrivialABIStruct(*Record);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2201,6 +2201,11 @@
   "base %0 is marked '%select{final|sealed}1'">;
 def warn_abstract_final_class : Warning<
   "abstract class is marked '%select{final|sealed}0'">, 
InGroup;
+def warn_final_dtor_non_final_class : Warning<
+  "class with destructor marked '%select{final|sealed}0' cannot be inherited 
from">,
+  InGroup;
+def note_final_dtor_non_final_class_silence : Note<
+  "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -114,6 +114,7 @@
  [DeleteNonAbstractNonVirtualDtor,
   DeleteAbstractNonVirtualDtor]>;
 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
+def FinalDtorNonFinalClass : DiagGroup<"final-dtor-non-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;


Index: clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};
Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -475,6 +475,11 @@
 // expected-error@+1 {{base 'SealedType' is marked

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

No worries, always happy to help! :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014



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

[PATCH] D66715: [CFG] Add dumps for CFGElement and CFGElementRef

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

Mind that I'll probably commit without the unit test, last time I learned the 
hard way that the AST's lifetime ends by the time we retrieve the CFG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66715



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

[PATCH] D66565: [analyzer] pr43036: Fix support for operator `sizeof...'.

2019-08-25 Thread Aleksei Sidorin via Phabricator via cfe-commits

a_sidorin accepted this revision.
a_sidorin added a comment.

LGTM++


Repository:
  rC Clang

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

https://reviews.llvm.org/D66565



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

[PATCH] D66706: [Wdocumentation] fixes an assertion failure with typedefed function and block pointer

2019-08-25 Thread Dmitri Gribenko via Phabricator via cfe-commits

gribozavr added inline comments.



Comment at: clang/lib/AST/Comment.cpp:151
+static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL,
+   bool testTypedefTypeLoc = false) {
   TypeLoc PrevTL;

Why is the new functionality turned off sometimes? It seems to me that we 
always want to look through typedefs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66706



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

[PATCH] D66706: [Wdocumentation] fixes an assertion failure with typedefed function and block pointer

2019-08-25 Thread Mark de Wever via Phabricator via cfe-commits

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

Comment at: clang/lib/AST/Comment.cpp:151
+static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL,
+   bool testTypedefTypeLoc = false) {
   TypeLoc PrevTL;

gribozavr wrote:
> Why is the new functionality turned off sometimes? It seems to me that we 
> always want to look through typedefs.
Setting `testTypedefTypeLoc` to `true` breaks a unit test in 
`test/Sema/warn-documentation.cpp:358`

```
typedef int (*test_not_function_like_typedef1)(int aaa);

// expected-warning@+1 {{'\param' command used in a comment that is not 
attached to a function declaration}}
/// \param aaa Meow.
typedef test_not_function_like_typedef1 test_not_function_like_typedef2;
```
and its corresponding test using a `using` instead of `typedef`. This has been 
introduced in:
```
commit 49fdf8d3f5e114e6f8b49fde29a30b0eaaa3c5dd
Author: Dmitri Gribenko 
Date:   Sat Sep 15 21:13:36 2012 +

Comment parsing: don't treat typedef to a typedef to a function as a
'function-like' type that can be annotated with \param.

Thanks to Eli Friedman for noticing!

llvm-svn: 163985

```
I'm not sure whether or not we should allow this typedef documentation. I just 
tested with Doxygen. It doesn't complain and shows the parameter documentation 
for `test_not_function_like_typedef2`. So on that basis we could remove this 
`expected-warning` and `testTypedefTypeLoc`.

What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66706

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

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

2019-08-25 Thread Alexandre Ganea via Phabricator via cfe-commits

aganea updated this revision to Diff 217055.
aganea marked 2 inline comments as done.
aganea added a comment.

As requested.


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

https://reviews.llvm.org/D66556

Files:
  lib/Lex/DependencyDirectivesSourceMinimizer.cpp
  test/Lexer/minimize_source_to_dependency_directives_invalid_error.c

Index: test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
===
--- test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
+++ test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
@@ -0,0 +1,16 @@
+// Test CF+LF are properly handled along with quoted, multi-line #error
+// RUN: cat %s | unix2dos | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s
+
+#ifndef TEST
+#error "message \
+   more message \
+   even more"
+#endif
+
+#ifdef OTHER
+#include 
+#endif
+
+// CHECK:  #ifdef OTHER
+// CHECK-NEXT: #include 
+// CHECK-NEXT: #endif
Index: lib/Lex/DependencyDirectivesSourceMinimizer.cpp
===
--- lib/Lex/DependencyDirectivesSourceMinimizer.cpp
+++ lib/Lex/DependencyDirectivesSourceMinimizer.cpp
@@ -196,15 +196,27 @@
 ++First; // Finish off the string.
 }
 
-static void skipNewline(const char *&First, const char *End) {
-  assert(isVerticalWhitespace(*First));
-  ++First;
+// Returns the length of EOL, either 0 (no end-of-line), 1 (\n) or 2 (\r\n)
+static unsigned isEOL(const char *First, const char *const End) {
   if (First == End)
-return;
+return 0;
+  if (End - First > 1 && isVerticalWhitespace(First[0]) &&
+  isVerticalWhitespace(First[1]) && First[0] != First[1])
+return 2;
+  return !!isVerticalWhitespace(First[0]);
+}
 
-  // Check for "\n\r" and "\r\n".
-  if (LLVM_UNLIKELY(isVerticalWhitespace(*First) && First[-1] != First[0]))
-++First;
+// Returns the length of the skipped newline
+static unsigned skipNewline(const char *&First, const char *End) {
+  assert(isVerticalWhitespace(*First));
+  unsigned Len = isEOL(First, End);
+  assert(Len && "expected newline");
+  First += Len;
+  return Len;
+}
+
+static bool wasLineContinuation(const char *First, unsigned EOLLen) {
+  return *(First - (int)EOLLen - 1) == '\\';
 }
 
 static void skipToNewlineRaw(const char *&First, const char *const End) {
@@ -212,17 +224,21 @@
 if (First == End)
   return;
 
-if (isVerticalWhitespace(*First))
+unsigned Len = isEOL(First, End);
+if (Len)
   return;
 
-while (!isVerticalWhitespace(*First))
+do {
   if (++First == End)
 return;
+  Len = isEOL(First, End);
+} while (!Len);
 
 if (First[-1] != '\\')
   return;
 
-++First; // Keep going...
+First += Len;
+// Keep skipping lines...
   }
 }
 
@@ -277,7 +293,7 @@
 }
 
 static void skipLine(const char *&First, const char *const End) {
-  do {
+  for (;;) {
 assert(First <= End);
 if (First == End)
   return;
@@ -322,9 +338,10 @@
   return;
 
 // Skip over the newline.
-assert(isVerticalWhitespace(*First));
-skipNewline(First, End);
-  } while (First[-2] == '\\'); // Continue past line-continuations.
+unsigned Len = skipNewline(First, End);
+if (!wasLineContinuation(First, Len)) // Continue past line-continuations.
+  break;
+  }
 }
 
 static void skipDirective(StringRef Name, const char *&First,
@@ -379,6 +396,8 @@
 // Print out the string.
 if (Last == End || Last == First || Last[-1] != '\\') {
   append(First, reverseOverSpaces(First, Last));
+  First = Last;
+  skipNewline(First, End);
   return;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66669: [X86] Remove what little support we had for MPX

2019-08-25 Thread Craig Topper via Phabricator via cfe-commits

craig.topper updated this revision to Diff 217056.
craig.topper added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add to clang and llvm release notes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D9

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h
  clang/test/Driver/x86-target-features.c
  clang/test/Preprocessor/predefined-arch-macros.c
  llvm/docs/ReleaseNotes.rst
  llvm/lib/Support/Host.cpp
  llvm/lib/Target/X86/X86.td
  llvm/lib/Target/X86/X86InstrInfo.td
  llvm/lib/Target/X86/X86InstrMPX.td
  llvm/lib/Target/X86/X86Subtarget.h
  llvm/test/CodeGen/X86/ms-inline-asm-avx512.ll
  llvm/test/CodeGen/X86/vector-width-store-merge.ll

Index: llvm/test/CodeGen/X86/vector-width-store-merge.ll
===
--- llvm/test/CodeGen/X86/vector-width-store-merge.ll
+++ llvm/test/CodeGen/X86/vector-width-store-merge.ll
@@ -46,8 +46,8 @@
 ; Function Attrs: argmemonly nounwind
 declare void @llvm.memmove.p0i8.p0i8.i64(i8* nocapture, i8* nocapture readonly, i64, i1 immarg) #1
 
-attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "prefer-vector-width"="128" "stack-protector-buffer-size"="8" "target-cpu"="skylake-avx512" "target-features"="+adx,+aes,+avx,+avx2,+avx512bw,+avx512cd,+avx512dq,+avx512f,+avx512vl,+bmi,+bmi2,+clflushopt,+clwb,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+mpx,+pclmul,+pku,+popcnt,+prfchw,+rdrnd,+rdseed,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "prefer-vector-width"="128" "stack-protector-buffer-size"="8" "target-cpu"="skylake-avx512" "target-features"="+adx,+aes,+avx,+avx2,+avx512bw,+avx512cd,+avx512dq,+avx512f,+avx512vl,+bmi,+bmi2,+clflushopt,+clwb,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+pclmul,+pku,+popcnt,+prfchw,+rdrnd,+rdseed,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" "unsafe-fp-math"="false" "use-soft-float"="false" }
 attributes #1 = { argmemonly nounwind }
-attributes #2 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "prefer-vector-width"="256" "stack-protector-buffer-size"="8" "target-cpu"="skylake-avx512" "target-features"="+adx,+aes,+avx,+avx2,+avx512bw,+avx512cd,+avx512dq,+avx512f,+avx512vl,+bmi,+bmi2,+clflushopt,+clwb,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+mpx,+pclmul,+pku,+popcnt,+prfchw,+rdrnd,+rdseed,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "prefer-vector-width"="256" "stack-protector-buffer-size"="8" "target-cpu"="skylake-avx512" "target-features"="+adx,+aes,+avx,+avx2,+avx512bw,+avx512cd,+avx512dq,+avx512f,+avx512vl,+bmi,+bmi2,+clflushopt,+clwb,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+pclmul,+pku,+popcnt,+prfchw,+rdrnd,+rdseed,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" "unsafe-fp-math"="false" "use-soft-float"="false" }
 
 !0 = !{i32 1, !"wchar_size", i32 4}
Index: llvm/test/CodeGen/X86/ms-inline-asm-avx512.ll
===
--- llvm/test/CodeGen/X86/ms-inline-asm-avx512.ll
+++ llvm/test/CodeGen/X86/ms-inline-asm-avx512.ll
@@ -20,5 +20,5 @@
 ; CHECK: movq%rax, 7(%rsp)
 ; CHECK: retq
 
-attributes #0 = { noinline nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false

r369873 - [Wdocumentation] improve wording of a warning message

2019-08-25 Thread Dmitri Gribenko via cfe-commits

Author: gribozavr
Date: Sun Aug 25 11:20:18 2019
New Revision: 369873

URL: http://llvm.org/viewvc/llvm-project?rev=369873&view=rev
Log:
[Wdocumentation] improve wording of a warning message

Based on @davezarzycki remarks in D64696 improved the wording of the warning
message.

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

Patch by Mark de Wever.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
cfe/trunk/test/Sema/warn-documentation.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td?rev=369873&r1=369872&r2=369873&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td Sun Aug 25 11:20:18 
2019
@@ -156,7 +156,7 @@ def note_add_deprecation_attr : Note<
 // inline contents commands
 
 def warn_doc_inline_contents_no_argument : Warning<
-  "'%select{\\|@}0%1' command does not have an argument">,
+  "'%select{\\|@}0%1' command does not have a valid word argument">,
   InGroup, DefaultIgnore;
 
 // verbatim block commands

Modified: cfe/trunk/test/Sema/warn-documentation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-documentation.cpp?rev=369873&r1=369872&r2=369873&view=diff
==
--- cfe/trunk/test/Sema/warn-documentation.cpp (original)
+++ cfe/trunk/test/Sema/warn-documentation.cpp Sun Aug 25 11:20:18 2019
@@ -1050,42 +1050,42 @@ template 
 void test_attach38::test_attach39(int, B);
 
 // The inline comments expect a string after the command.
-// expected-warning@+1 {{'\a' command does not have an argument}}
+// expected-warning@+1 {{'\a' command does not have a valid word argument}}
 /// \a
 int test_inline_no_argument_a_bad(int);
 
 /// \a A
 int test_inline_no_argument_a_good(int);
 
-// expected-warning@+1 {{'@b' command does not have an argument}}
+// expected-warning@+1 {{'@b' command does not have a valid word argument}}
 /// @b
 int test_inline_no_argument_b_bad(int);
 
 /// @b A
 int test_inline_no_argument_b_good(int);
 
-// expected-warning@+1 {{'\c' command does not have an argument}}
+// expected-warning@+1 {{'\c' command does not have a valid word argument}}
 /// \c
 int test_inline_no_argument_c_bad(int);
 
 /// \c A
 int test_inline_no_argument_c_good(int);
 
-// expected-warning@+1 {{'\e' command does not have an argument}}
+// expected-warning@+1 {{'\e' command does not have a valid word argument}}
 /// \e
 int test_inline_no_argument_e_bad(int);
 
 /// \e A
 int test_inline_no_argument_e_good(int);
 
-// expected-warning@+1 {{'\em' command does not have an argument}}
+// expected-warning@+1 {{'\em' command does not have a valid word argument}}
 /// \em
 int test_inline_no_argument_em_bad(int);
 
 /// \em A
 int test_inline_no_argument_em_good(int);
 
-// expected-warning@+1 {{'\p' command does not have an argument}}
+// expected-warning@+1 {{'\p' command does not have a valid word argument}}
 /// \p
 int test_inline_no_argument_p_bad(int);
 


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

[PATCH] D66700: [Wdocumentation] improve wording of a warning message

2019-08-25 Thread Dmitri Gribenko via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369873: [Wdocumentation] improve wording of a warning 
message (authored by gribozavr, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66700?vs=217013&id=217057#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66700

Files:
  cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
  cfe/trunk/test/Sema/warn-documentation.cpp


Index: cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
@@ -156,7 +156,7 @@
 // inline contents commands
 
 def warn_doc_inline_contents_no_argument : Warning<
-  "'%select{\\|@}0%1' command does not have an argument">,
+  "'%select{\\|@}0%1' command does not have a valid word argument">,
   InGroup, DefaultIgnore;
 
 // verbatim block commands
Index: cfe/trunk/test/Sema/warn-documentation.cpp
===
--- cfe/trunk/test/Sema/warn-documentation.cpp
+++ cfe/trunk/test/Sema/warn-documentation.cpp
@@ -1050,42 +1050,42 @@
 void test_attach38::test_attach39(int, B);
 
 // The inline comments expect a string after the command.
-// expected-warning@+1 {{'\a' command does not have an argument}}
+// expected-warning@+1 {{'\a' command does not have a valid word argument}}
 /// \a
 int test_inline_no_argument_a_bad(int);
 
 /// \a A
 int test_inline_no_argument_a_good(int);
 
-// expected-warning@+1 {{'@b' command does not have an argument}}
+// expected-warning@+1 {{'@b' command does not have a valid word argument}}
 /// @b
 int test_inline_no_argument_b_bad(int);
 
 /// @b A
 int test_inline_no_argument_b_good(int);
 
-// expected-warning@+1 {{'\c' command does not have an argument}}
+// expected-warning@+1 {{'\c' command does not have a valid word argument}}
 /// \c
 int test_inline_no_argument_c_bad(int);
 
 /// \c A
 int test_inline_no_argument_c_good(int);
 
-// expected-warning@+1 {{'\e' command does not have an argument}}
+// expected-warning@+1 {{'\e' command does not have a valid word argument}}
 /// \e
 int test_inline_no_argument_e_bad(int);
 
 /// \e A
 int test_inline_no_argument_e_good(int);
 
-// expected-warning@+1 {{'\em' command does not have an argument}}
+// expected-warning@+1 {{'\em' command does not have a valid word argument}}
 /// \em
 int test_inline_no_argument_em_bad(int);
 
 /// \em A
 int test_inline_no_argument_em_good(int);
 
-// expected-warning@+1 {{'\p' command does not have an argument}}
+// expected-warning@+1 {{'\p' command does not have a valid word argument}}
 /// \p
 int test_inline_no_argument_p_bad(int);
 


Index: cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td
@@ -156,7 +156,7 @@
 // inline contents commands
 
 def warn_doc_inline_contents_no_argument : Warning<
-  "'%select{\\|@}0%1' command does not have an argument">,
+  "'%select{\\|@}0%1' command does not have a valid word argument">,
   InGroup, DefaultIgnore;
 
 // verbatim block commands
Index: cfe/trunk/test/Sema/warn-documentation.cpp
===
--- cfe/trunk/test/Sema/warn-documentation.cpp
+++ cfe/trunk/test/Sema/warn-documentation.cpp
@@ -1050,42 +1050,42 @@
 void test_attach38::test_attach39(int, B);
 
 // The inline comments expect a string after the command.
-// expected-warning@+1 {{'\a' command does not have an argument}}
+// expected-warning@+1 {{'\a' command does not have a valid word argument}}
 /// \a
 int test_inline_no_argument_a_bad(int);
 
 /// \a A
 int test_inline_no_argument_a_good(int);
 
-// expected-warning@+1 {{'@b' command does not have an argument}}
+// expected-warning@+1 {{'@b' command does not have a valid word argument}}
 /// @b
 int test_inline_no_argument_b_bad(int);
 
 /// @b A
 int test_inline_no_argument_b_good(int);
 
-// expected-warning@+1 {{'\c' command does not have an argument}}
+// expected-warning@+1 {{'\c' command does not have a valid word argument}}
 /// \c
 int test_inline_no_argument_c_bad(int);
 
 /// \c A
 int test_inline_no_argument_c_good(int);
 
-// expected-warning@+1 {{'\e' command does not have an argument}}
+// expected-warning@+1 {{'\e' command does not have a valid word argument}}
 /// \e
 int test_inline_no_argument_e_bad(int);
 
 /// \e A
 int test_inline_no_argument_e_good(int);
 
-// expected-warning@+1 {{'\em' command does not have an argument}}
+// expected-warning@+1 {{'\em' command does not have a valid word argument}}
 /// \em
 int test_inline_no_argument_em_bad(int);
 
 ///

[PATCH] D66706: [Wdocumentation] fixes an assertion failure with typedefed function and block pointer

2019-08-25 Thread Dmitri Gribenko via Phabricator via cfe-commits

gribozavr added inline comments.



Comment at: clang/lib/AST/Comment.cpp:151
+static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL,
+   bool testTypedefTypeLoc = false) {
   TypeLoc PrevTL;

Mordante wrote:
> gribozavr wrote:
> > Why is the new functionality turned off sometimes? It seems to me that we 
> > always want to look through typedefs.
> Setting `testTypedefTypeLoc` to `true` breaks a unit test in 
> `test/Sema/warn-documentation.cpp:358`
> 
> ```
> typedef int (*test_not_function_like_typedef1)(int aaa);
> 
> // expected-warning@+1 {{'\param' command used in a comment that is not 
> attached to a function declaration}}
> /// \param aaa Meow.  
>   
> typedef test_not_function_like_typedef1 test_not_function_like_typedef2;
> ```
> and its corresponding test using a `using` instead of `typedef`. This has 
> been introduced in:
> ```
> commit 49fdf8d3f5e114e6f8b49fde29a30b0eaaa3c5dd
> Author: Dmitri Gribenko 
> Date:   Sat Sep 15 21:13:36 2012 +
> 
> Comment parsing: don't treat typedef to a typedef to a function as a
> 'function-like' type that can be annotated with \param.
> 
> Thanks to Eli Friedman for noticing!
> 
> llvm-svn: 163985
> 
> ```
> I'm not sure whether or not we should allow this typedef documentation. I 
> just tested with Doxygen. It doesn't complain and shows the parameter 
> documentation for `test_not_function_like_typedef2`. So on that basis we 
> could remove this `expected-warning` and `testTypedefTypeLoc`.
> 
> What do you think?
Thanks for the explanation. I can't find the context for that decision, and the 
commit description does not explain the reason either.

It is really a policy decision -- when introducing a comment for function 
return value and arguments, should the declaration include the said return 
value and arguments, or can they be visible through a typedef?

Thinking about it, my inclination is to say that comments for the return value 
and arguments should be present on the declaration that introduces them. 
Otherwise, it is breaking an abstraction barrier.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66706



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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

This patch introduces a new `analyzer-config` configuration:
`-analyzer-config check-bitwise=false`
which could be used to toggle whether the bitwise (and shift) operations
should be checked. It by default set to `false` as we do not model the
bitwise operations enough well.


Repository:
  rC Clang

https://reviews.llvm.org/D66721

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/misc-ps-region-store.m

Index: clang/test/Analysis/misc-ps-region-store.m
===
--- clang/test/Analysis/misc-ps-region-store.m
+++ clang/test/Analysis/misc-ps-region-store.m
@@ -1,5 +1,22 @@
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -analyzer-checker=core,alpha.core.CastToStruct,alpha.security.ReturnPtrRange,alpha.security.ArrayBound -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks -Wno-objc-root-class %s
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -DTEST_64 -analyzer-checker=core,alpha.core.CastToStruct,alpha.security.ReturnPtrRange,alpha.security.ArrayBound -analyzer-store=region -verify -fblocks   -analyzer-opt-analyze-nested-blocks -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-checker=alpha.core.CastToStruct \
+// RUN:  -analyzer-checker=alpha.security.ReturnPtrRange \
+// RUN:  -analyzer-checker=alpha.security.ArrayBound \
+// RUN:  -analyzer-store=region \
+// RUN:  -analyzer-opt-analyze-nested-blocks \
+// RUN:  -analyzer-config check-bitwise=true \
+// RUN:  -Wno-objc-root-class -fblocks -verify %s
+
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-checker=alpha.core.CastToStruct \
+// RUN:  -analyzer-checker=alpha.security.ReturnPtrRange \
+// RUN:  -analyzer-checker=alpha.security.ArrayBound \
+// RUN:  -analyzer-store=region \
+// RUN:  -analyzer-opt-analyze-nested-blocks \
+// RUN:  -analyzer-config check-bitwise=true \
+// RUN:  -Wno-objc-root-class -fblocks -DTEST_64 -verify %s
 
 typedef long unsigned int size_t;
 void *memcpy(void *, const void *, size_t);
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,14 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin13 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -Wno-shift-count-overflow \
+// RUN:  -analyzer-config check-bitwise=true \
+// RUN:  -std=c++17 -verify=expected,cxx17 %s
+
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin13 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -Wno-shift-count-overflow \
+// RUN:  -analyzer-config check-bitwise=true \
+// RUN:  -std=c++2a -verify=expected,cxx2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -x c++ -analyzer-checker=core -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config check-bitwise=true \
+// RUN:  -analyzer-output=text -verify %s
 
 typedef unsigned char uint8_t;
 
@@ -38,7 +40,7 @@
 
   for (int i = 0; i < 3; ++i)
 // expected-note@-1 3{{Loop condition is true.  Entering loop body}}
-// expected-note@-2 {{Loop condition is false. Execution continues on line 43}}
+// expected-note@-2 {{Loop condition is false. Execution continues on line 45}}
 arr[i] = 0;
   int x = getInt();
   int n = getIndex(x); // expected-note {{Calling 'getIndex'}}
@@ -70,7 +72,7 @@
 
   for (int i = 0; i < 3; ++i)
 // expected-note@-1 3{{Loop condition is true.  Entering loop body}}
-// expected-note@-2 {{Loop condition i

[PATCH] D66722: [clang] Ensure that comment classes are trivially destructible

2019-08-25 Thread Bruno Ricci via Phabricator via cfe-commits

riccibruno created this revision.
riccibruno added reviewers: aaron.ballman, Mordante.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

As in D66646 , these classes are also 
allocated with a `BumpPtrAllocator`, and therefore should be trivially 
destructible.


Repository:
  rC Clang

https://reviews.llvm.org/D66722

Files:
  clang/lib/AST/Comment.cpp


Index: clang/lib/AST/Comment.cpp
===
--- clang/lib/AST/Comment.cpp
+++ clang/lib/AST/Comment.cpp
@@ -13,10 +13,21 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/ErrorHandling.h"
+#include 
 
 namespace clang {
 namespace comments {
 
+// Check that no comment class has a non-trival destructor. They are allocated
+// with a BumpPtrAllocator and therefore their destructor is not executed.
+#define ABSTRACT_COMMENT(COMMENT)
+#define COMMENT(CLASS, PARENT) 
\
+  static_assert(std::is_trivially_destructible::value,  
\
+#CLASS " should be trivially destructible!");
+#include "clang/AST/CommentNodes.inc"
+#undef COMMENT
+#undef ABSTRACT_COMMENT
+
 const char *Comment::getCommentKindName() const {
   switch (getCommentKind()) {
   case NoCommentKind: return "NoCommentKind";


Index: clang/lib/AST/Comment.cpp
===
--- clang/lib/AST/Comment.cpp
+++ clang/lib/AST/Comment.cpp
@@ -13,10 +13,21 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/ErrorHandling.h"
+#include 
 
 namespace clang {
 namespace comments {
 
+// Check that no comment class has a non-trival destructor. They are allocated
+// with a BumpPtrAllocator and therefore their destructor is not executed.
+#define ABSTRACT_COMMENT(COMMENT)
+#define COMMENT(CLASS, PARENT) \
+  static_assert(std::is_trivially_destructible::value,  \
+#CLASS " should be trivially destructible!");
+#include "clang/AST/CommentNodes.inc"
+#undef COMMENT
+#undef ABSTRACT_COMMENT
+
 const char *Comment::getCommentKindName() const {
   switch (getCommentKind()) {
   case NoCommentKind: return "NoCommentKind";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66722: [clang] Ensure that comment classes are trivially destructible

2019-08-25 Thread Mark de Wever via Phabricator via cfe-commits

Mordante added a reviewer: gribozavr.
Mordante added a subscriber: gribozavr.
Mordante added a comment.

Please also add a test for the `DeclInfo` struct. It isn't a `CommentNode` but 
also allocated with the `BumpPtrAllocator`.
I also like @gribozavr's input.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66722



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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

No-no, you're disabling the checkers here but you should be silencing them. 
This will be crashing because modeling is disabled.

I also recommend checker options, even if they apply to multiple checkers 
(@Szelethus mentioned that package options are a thing, you might use these if 
you don't want to make multiple options).




Comment at: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:82-88
 // Do not report assignments of uninitialized values inside swap functions.
 // This should allow to swap partially uninitialized structs
 // (radar://14129997)
 if (const FunctionDecl *EnclosingFunctionDecl =
 dyn_cast(C.getStackFrame()->getDecl()))
   if (C.getCalleeName(EnclosingFunctionDecl) == "swap")
 return;

Ugh. This part is also wrong for the same reason.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66721



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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:305
+"Whether the bitwise (and shift) operations should be 
checked.",
+false)
+

We didn't do enough debugging to demonstrate that the problems are specific to 
the checker. I suggest we keep this on by default and only disable it on LLVM 
as an overfitting effort.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66721



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

[PATCH] D66722: [clang] Ensure that comment classes are trivially destructible

2019-08-25 Thread Dmitri Gribenko via Phabricator via cfe-commits

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

Regarding adding a test for `DeclInfo` -- SGTM as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66722



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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso added inline comments.

Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:305
+"Whether the bitwise (and shift) operations should be 
checked.",
+false)
+

NoQ wrote:
> We didn't do enough debugging to demonstrate that the problems are specific 
> to the checker. I suggest we keep this on by default and only disable it on 
> LLVM as an overfitting effort.
Well, okay.

Comment at: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:82-88
 // Do not report assignments of uninitialized values inside swap functions.
 // This should allow to swap partially uninitialized structs
 // (radar://14129997)
 if (const FunctionDecl *EnclosingFunctionDecl =
 dyn_cast(C.getStackFrame()->getDecl()))
   if (C.getCalleeName(EnclosingFunctionDecl) == "swap")
 return;

NoQ wrote:
> Ugh. This part is also wrong for the same reason.
If I do not return here then `clang/test/Analysis/uninit-vals.m:113: Returning 
from 'swap'` related notes are gone, so I will leave it untouched.

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

https://reviews.llvm.org/D66721

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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso updated this revision to Diff 217062.
Charusso marked 4 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.

- Fix.


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

https://reviews.llvm.org/D66721

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/test/Analysis/analyzer-config.c


Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -25,6 +25,7 @@
 // CHECK-NEXT: cfg-rich-constructors = true
 // CHECK-NEXT: cfg-scopes = false
 // CHECK-NEXT: cfg-temporary-dtors = true
+// CHECK-NEXT: check-bitwise = true
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
@@ -93,4 +94,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 90
+// CHECK-NEXT: num-entries = 91
Index: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -75,6 +75,10 @@
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   if (C.getSVal(B).isUndef()) {
+AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
+if (B->isBitwiseOrShiftOp() && !Opts.CheckBitwise)
+  Opts.SilencedCheckersAndPackages.push_back(
+  "core.UndefinedBinaryOperatorResult");
 
 // Do not report assignments of uninitialized values inside swap functions.
 // This should allow to swap partially uninitialized structs
Index: clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
@@ -60,6 +60,11 @@
   CheckerContext &Ctx) const {
   SVal X = Ctx.getSVal(Condition);
   if (X.isUndef()) {
+AnalyzerOptions &Opts = Ctx.getAnalysisManager().getAnalyzerOptions();
+if (const auto *BO = dyn_cast(Condition))
+  if (BO->isBitwiseOrShiftOp() && !Opts.CheckBitwise)
+
Opts.SilencedCheckersAndPackages.push_back("core.uninitialized.Branch");
+
 // Generate a sink node, which implicitly marks both outgoing branches as
 // infeasible.
 ExplodedNode *N = Ctx.generateErrorNode();
Index: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
===
--- clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -300,6 +300,14 @@
 "Whether to place an event at each tracked condition.",
 false)
 
+ANALYZER_OPTION(bool, CheckBitwise, "check-bitwise",
+"Whether the bitwise (and shift) operations should be 
checked.",
+true)
+
+//===--===//
+// Unsinged analyzer options.
+//===--===//
+
 ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
 "The maximal amount of translation units that is considered "
 "for import when inlining functions during CTU analysis. "
@@ -308,10 +316,6 @@
 "various translation units.",
 100u)
 
-//===--===//
-// Unsinged analyzer options.
-//===--===//
-
 ANALYZER_OPTION(
 unsigned, AlwaysInlineSize, "ipa-always-inline-size",
 "The size of the functions (in basic blocks), which should be considered "
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -3485,6 +3485,11 @@
   static bool isBitwiseOp(Opcode Opc) { return Opc >= BO_And && Opc <= BO_Or; }
   bool isBitwiseOp() const { return isBitwiseOp(getOpcode()); }
 
+  static bool isBitwiseOrShiftOp(Opcode Opc) {
+return isBitwiseOp(Opc) || isShiftOp(Opc);
+  }
+  bool isBitwiseOrShiftOp() const { return isBitwiseOrShiftOp(getOpcode()); }
+
   static bool isRelationalOp(Opcode Opc) { return Opc >= BO_LT && Opc<=BO_GE; }
   bool isRelationalOp() const { return isRelationalOp(getOpcode()); }
 


Index: clang/test/Analysis/analyzer-config.

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

Now you're silencing the *whole* checker. This is equivalent to passing an 
`-analyzer-silence-checker` flag.


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

https://reviews.llvm.org/D66721



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

[PATCH] D66722: [clang] Ensure that comment classes are trivially destructible

2019-08-25 Thread Bruno Ricci via Phabricator via cfe-commits

riccibruno updated this revision to Diff 217063.
riccibruno added a comment.

Also test that `DeclInfo` is trivially destructible.

Thanks !


Repository:
  rC Clang

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

https://reviews.llvm.org/D66722

Files:
  clang/lib/AST/Comment.cpp


Index: clang/lib/AST/Comment.cpp
===
--- clang/lib/AST/Comment.cpp
+++ clang/lib/AST/Comment.cpp
@@ -13,10 +13,25 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/ErrorHandling.h"
+#include 
 
 namespace clang {
 namespace comments {
 
+// Check that no comment class has a non-trival destructor. They are allocated
+// with a BumpPtrAllocator and therefore their destructor is not executed.
+#define ABSTRACT_COMMENT(COMMENT)
+#define COMMENT(CLASS, PARENT) 
\
+  static_assert(std::is_trivially_destructible::value,  
\
+#CLASS " should be trivially destructible!");
+#include "clang/AST/CommentNodes.inc"
+#undef COMMENT
+#undef ABSTRACT_COMMENT
+
+// DeclInfo is also allocated with a BumpPtrAllocator.
+static_assert(std::is_trivially_destructible::value,
+  "DeclInfo should be trivially destructible!");
+
 const char *Comment::getCommentKindName() const {
   switch (getCommentKind()) {
   case NoCommentKind: return "NoCommentKind";


Index: clang/lib/AST/Comment.cpp
===
--- clang/lib/AST/Comment.cpp
+++ clang/lib/AST/Comment.cpp
@@ -13,10 +13,25 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/ErrorHandling.h"
+#include 
 
 namespace clang {
 namespace comments {
 
+// Check that no comment class has a non-trival destructor. They are allocated
+// with a BumpPtrAllocator and therefore their destructor is not executed.
+#define ABSTRACT_COMMENT(COMMENT)
+#define COMMENT(CLASS, PARENT) \
+  static_assert(std::is_trivially_destructible::value,  \
+#CLASS " should be trivially destructible!");
+#include "clang/AST/CommentNodes.inc"
+#undef COMMENT
+#undef ABSTRACT_COMMENT
+
+// DeclInfo is also allocated with a BumpPtrAllocator.
+static_assert(std::is_trivially_destructible::value,
+  "DeclInfo should be trivially destructible!");
+
 const char *Comment::getCommentKindName() const {
   switch (getCommentKind()) {
   case NoCommentKind: return "NoCommentKind";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso added a comment.

In D66721#1644531 , @NoQ wrote:

> Now you're silencing the *whole* checker. This is equivalent to passing an 
> `-analyzer-silence-checker` flag.

I am silencing the *whole* checker if and only if a bitwise operation or a 
shift operation occurs. That is the correct behavior which is not equal to 
`-analyzer-silence-checker` which disables the *whole* checker during the 
*whole* analysis.

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

https://reviews.llvm.org/D66721

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

[PATCH] D66723: [clangd] Add a distinct highlighting for local variables

2019-08-25 Thread Nathan Ridge via Phabricator via cfe-commits

nridge created this revision.
nridge added reviewers: hokein, jvikstrom.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

It's useful to be able to distinguish local variables from namespace
scope variables.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66723

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -41,7 +41,8 @@
   {HighlightingKind::Field, "Field"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
-  {HighlightingKind::Primitive, "Primitive"}};
+  {HighlightingKind::Primitive, "Primitive"},
+  {HighlightingKind::LocalVariable, "LocalVariable"}};
   std::vector ExpectedTokens;
   for (const auto &KindString : KindToString) {
 std::vector Toks = makeHighlightingTokens(
@@ -99,31 +100,31 @@
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
-R"cpp(
+  R"cpp(
   struct $Class[[AS]] {
 $Primitive[[double]] $Field[[SomeMember]];
   };
   struct {
   } $Variable[[S]];
   $Primitive[[void]] $Function[[foo]]($Primitive[[int]] $Parameter[[A]], $Class[[AS]] $Parameter[[As]]) {
-$Primitive[[auto]] $Variable[[VeryLongVariableName]] = 12312;
-$Class[[AS]] $Variable[[AA]];
-$Primitive[[auto]] $Variable[[L]] = $Variable[[AA]].$Field[[SomeMember]] + $Parameter[[A]];
-auto $Variable[[FN]] = [ $Variable[[AA]]]($Primitive[[int]] $Parameter[[A]]) -> $Primitive[[void]] {};
-$Variable[[FN]](12312);
+$Primitive[[auto]] $LocalVariable[[VeryLongVariableName]] = 12312;
+$Class[[AS]] $LocalVariable[[AA]];
+$Primitive[[auto]] $LocalVariable[[L]] = $LocalVariable[[AA]].$Field[[SomeMember]] + $Parameter[[A]];
+auto $LocalVariable[[FN]] = [ $LocalVariable[[AA]]]($Primitive[[int]] $Parameter[[A]]) -> $Primitive[[void]] {};
+$LocalVariable[[FN]](12312);
   }
 )cpp",
-R"cpp(
+  R"cpp(
   $Primitive[[void]] $Function[[foo]]($Primitive[[int]]);
   $Primitive[[void]] $Function[[Gah]]();
   $Primitive[[void]] $Function[[foo]]() {
-auto $Variable[[Bou]] = $Function[[Gah]];
+auto $LocalVariable[[Bou]] = $Function[[Gah]];
   }
   struct $Class[[A]] {
 $Primitive[[void]] $Method[[abc]]();
   };
 )cpp",
-R"cpp(
+  R"cpp(
   namespace $Namespace[[abc]] {
 template
 struct $Class[[A]] {
@@ -145,12 +146,12 @@
   $Class[[B]]::$Class[[B]]() {}
   $Class[[B]]::~$Class[[B]]() {}
   $Primitive[[void]] $Function[[f]] () {
-$Class[[B]] $Variable[[BB]] = $Class[[B]]();
-$Variable[[BB]].~$Class[[B]]();
+$Class[[B]] $LocalVariable[[BB]] = $Class[[B]]();
+$LocalVariable[[BB]].~$Class[[B]]();
 $Class[[B]]();
   }
 )cpp",
-R"cpp(
+  R"cpp(
   enum class $Enum[[E]] {
 $EnumConstant[[A]],
 $EnumConstant[[B]],
@@ -165,7 +166,7 @@
   $Primitive[[int]] $Variable[[I]] = $EnumConstant[[Hi]];
   $Enum[[E]] $Variable[[L]] = $Enum[[E]]::$EnumConstant[[B]];
 )cpp",
-R"cpp(
+  R"cpp(
   namespace $Namespace[[abc]] {
 namespace {}
 namespace $Namespace[[bcd]] {
@@ -188,7 +189,7 @@
   ::$Namespace[[vwz]]::$Class[[A]] $Variable[[B]];
   ::$Namespace[[abc]]::$Namespace[[bcd]]::$Class[[A]] $Variable[[BB]];
 )cpp",
-R"cpp(
+  R"cpp(
   struct $Class[[D]] {
 $Primitive[[double]] $Field[[C]];
   };
@@ -205,21 +206,21 @@
 }
   };
   $Primitive[[void]] $Function[[foo]]() {
-$Class[[A]] $Variable[[AA]];
-$Variable[[AA]].$Field[[B]] += 2;
-$Variable[[AA]].$Method[[foo]]();
-$Variable[[AA]].$Field[[E]].$Field[[C]];
+$Class[[A]] $LocalVariable[[AA]];
+$LocalVariable[[AA]].$Field[[B]] += 2;
+$LocalVariable[[AA]].$Method[[foo]]();
+$LocalVariable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
 )cpp",
-R"cpp(
+  R"cpp(
   struct $Class[[AA]] {
 $Primitive[[int]] $Field[[A]];
   }
   $Primitive[[int]] $Variable[[B]];
   $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp",
-R"cpp(
+  R"cpp(
   namespace $Namespace[[a]] {
 struct $Class[[A]] {};
 typedef $Primitive[[char]] $Primitive[[C]];
@@ -235,7 +236,7 @@
   typedef $Namespace[[a]]::$Primitive[[C]] $Prim

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

In D66721#1644514 , @NoQ wrote:

> No-no, you're disabling the checkers here but you should be silencing them. 
> This will be crashing because modeling is disabled.
>
> I also recommend checker options, even if they apply to multiple checkers 
> (@Szelethus mentioned that package options are a thing, you might use these 
> if you don't want to make multiple options).

Just to be clear, I object to nothing as long as it is an off-by-default 
developer-only feature.

Comment at: clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp:66
+  if (BO->isBitwiseOrShiftOp() && !Opts.CheckBitwise)
+
Opts.SilencedCheckersAndPackages.push_back("core.uninitialized.Branch");
+

This is one of the reasons why I believe `AnalyzerOptions` should be immutable 
once analysis begins. Say that you'd like to list the checkers currently 
enabled with `-analyzer-list-enabled-checkers`, and that list is compiled 
around the time `CheckerRegistry` is active (which is a very brief period 
during the initialization of `CheckerManager`). Wouldn't it be super 
infuriating to learn that this lislt may change *during* analysis?

I feel somewhat the same way about this, this should either be moved to 
`shouldRegister*()` or be handled outside the analyzer.

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

https://reviews.llvm.org/D66721

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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

Until we do enough debugging to understand the nature of these false positives, 
i recommend silencing the checker when it has false positives. Later we can 
improve the granularity of our checkers, so that to be able to disable smaller 
portions of them, or make constraint solver improvements, depending on what we 
see.


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

https://reviews.llvm.org/D66721



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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Fangrui Song via Phabricator via cfe-commits

MaskRay added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:308
+//===--===//
+// Unsinged analyzer options.
+//===--===//

typo: unsigned


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

https://reviews.llvm.org/D66721



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

[PATCH] D66721: [analyzer] Analysis: Prevent bitwise operation false positives

2019-08-25 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:308
+//===--===//
+// Unsinged analyzer options.
+//===--===//

MaskRay wrote:
> typo: unsigned
Yup, [[ https://reviews.llvm.org/D65182?id=214462#inline-591158 | see also ]] :)


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

https://reviews.llvm.org/D66721



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

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

2019-08-25 Thread Wei Mi via Phabricator via cfe-commits

wmi added a comment.

In D36562#1642403 , @chill wrote:

> In D36562#1641930 , @wmi wrote:
>
> > In D36562#1639441 , @chill wrote:
> >
> > > Shouldn't we disable `OPT_ffine_grained_bitfield_accesses` only if TSAN 
> > > is active?
> >
> >
> > I don't remember why it is disabled for all sanitizer modes. Seems you are 
> > right that the disabling the option is only necessary for TSAN. Do you have 
> > actual needs for the option to be functioning on other sanitizer modes?
>
>
> Well, yes and no. We have the option enabled by default and it causes a 
> warning when we use it together with `-fsanitize=memtag` (we aren't really 
> concerned with other sanitizers). That warning broke a few builds (e.g. CMake 
> doing tests and not wanting to see *any* diagnostics. We can work around that 
> in a number of ways, e.g. we can leave the default off for AArch64.
>
> I'd prefer though to have an upstream solution, if that's considered 
> beneficial for all LLVM users and this one seems like such a case: let anyone 
> use the option with sanitizers, unless it's known that some 
> sanitizers'utility is affected negatively (as with TSAN).


Thanks for providing the background in detail. I sent out a patch for it: 
https://reviews.llvm.org/D66726


Repository:
  rL LLVM

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

https://reviews.llvm.org/D36562



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

[PATCH] D66726: [Bitfield] Disable -ffine-grained-bitfield-accesses only when thread sanitizer is enabled

2019-08-25 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

I suppose this is fine for ubsan/lsan/asan, but does msan track things on 
smaller-than-byte level?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66726



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

41 matches

Mail list logo