[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

Hi @NoQ . Would you have time to look if the changes I did to this PR solve 
your concerns?


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

https://reviews.llvm.org/D126534

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

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

@rnk , does this change answer your points?


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

https://reviews.llvm.org/D130709

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

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 448549.
frederic-tingaud-sonarsource added a comment.

Use getAs to see through ElaboratedType as recommended by 
https://reviews.llvm.org/D112374


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

https://reviews.llvm.org/D130709

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp


Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- clang/test/SemaTemplate/ms-unqualified-base-class.cpp
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -83,3 +83,37 @@
 
   return I;
 }
+
+template  class Vec {}; // expected-note {{template 
is declared here}}
+
+template  class Index : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data 
member or base class}}
+  Index() : Vec() {} // before-warning {{unqualified base initializer of class 
templates is a Microsoft extension}}
+};
+
+template class Index<0>;
+
+template  class Array : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data 
member or base class}}
+  Array() : Vec() {} // before-warning {{unqualified base initializer of class 
templates is a Microsoft extension}}
+};
+
+template class Array;
+
+template  class Wrong : public Vec {
+  Wrong() : NonExistent() {} // expected-error {{member initializer 
'NonExistent' does not name a non-static data member or base class}}
+};
+
+template class Wrong;
+
+template  class Wrong2 : public Vec {
+  Wrong2() : Vec() {} // expected-error {{too few template arguments for 
class template 'Vec'}}
+};
+
+template class Wrong2;
+
+template  class Wrong3 : public Vec {
+  Wrong3() : Base() {} // expected-error {{member initializer 'Base' does not 
name a non-static data member or base class}}
+};
+
+template class Wrong3;
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4308,11 +4308,21 @@
   }
 
   if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
-auto UnqualifiedBase = R.getAsSingle();
-if (UnqualifiedBase) {
-  Diag(IdLoc, diag::ext_unqualified_base_class)
-  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
-  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+if (auto UnqualifiedBase = R.getAsSingle()) {
+  auto *TempSpec = cast(
+  UnqualifiedBase->getInjectedClassNameSpecialization());
+  TemplateName TN = TempSpec->getTemplateName();
+  for (auto const &Base : ClassDecl->bases()) {
+auto BaseTemplate =
+Base.getType()->getAs();
+if (BaseTemplate && Context.hasSameTemplateName(
+BaseTemplate->getTemplateName(), TN)) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = Base.getType();
+  break;
+}
+  }
 }
   }
 


Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- clang/test/SemaTemplate/ms-unqualified-base-class.cpp
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -83,3 +83,37 @@
 
   return I;
 }
+
+template  class Vec {}; // expected-note {{template is declared here}}
+
+template  class Index : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data member or base class}}
+  Index() : Vec() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template class Index<0>;
+
+template  class Array : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data member or base class}}
+  Array() : Vec() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template class Array;
+
+template  class Wrong : public Vec {
+  Wrong() : NonExistent() {} // expected-error {{member initializer 'NonExistent' does not name a non-static data member or base class}}
+};
+
+template class Wrong;
+
+template  class Wrong2 : public Vec {
+  Wrong2() : Vec() {} // expected-error {{too few template arguments for class template 'Vec'}}
+};
+
+template class Wrong2;
+
+template  class Wrong3 : public Vec {
+  Wrong3() : Base() {} // expected-error {{member initializer 'Base' does not name a non-static data member or base class}}
+};
+
+template class Wrong3;
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4308,11 +4308,21 @@
   }
 
   if (getLangOpts().MSVCCompat && !getLangOpt

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I introduced a patch to handle unqualified templated base class initialization 
in MSVC compatibility mode: 
https://reviews.llvm.org/rGc894e85fc64dd8d83b460de81080fff93c5ca334
We identified a problem with this patch in the case where the base class is 
partially specialized, which can lead to triggering an assertion in the case of 
a mix between types and values.
The minimal test case is:

  cpp
  template  class Vec {};
  
  template  class Index : public Vec {
Index() : Vec() {}
  };
  template class Index<0>;

The detailed problem is that I was using the InjectedClassNameSpecialization, 
to which the class template arguments were then applied in order. But in the 
process, we were losing all the partial specializations of the base class and 
creating an index mismatch between the expected and passed arguments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130709

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp


Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- clang/test/SemaTemplate/ms-unqualified-base-class.cpp
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -83,3 +83,37 @@
 
   return I;
 }
+
+template  class Vec {}; // expected-note {{template 
is declared here}}
+
+template  class Index : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data 
member or base class}}
+  Index() : Vec() {} // before-warning {{unqualified base initializer of class 
templates is a Microsoft extension}}
+};
+
+template class Index<0>;
+
+template  class Array : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data 
member or base class}}
+  Array() : Vec() {} // before-warning {{unqualified base initializer of class 
templates is a Microsoft extension}}
+};
+
+template class Array;
+
+template  class Wrong : public Vec {
+  Wrong() : NonExistent() {} // expected-error {{member initializer 
'NonExistent' does not name a non-static data member or base class}}
+};
+
+template class Wrong;
+
+template  class Wrong2 : public Vec {
+  Wrong2() : Vec() {} // expected-error {{too few template arguments for 
class template 'Vec'}}
+};
+
+template class Wrong2;
+
+template  class Wrong3 : public Vec {
+  Wrong3() : Base() {} // expected-error {{member initializer 'Base' does not 
name a non-static data member or base class}}
+};
+
+template class Wrong3;
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4308,11 +4308,21 @@
   }
 
   if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
-auto UnqualifiedBase = R.getAsSingle();
-if (UnqualifiedBase) {
-  Diag(IdLoc, diag::ext_unqualified_base_class)
-  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
-  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+if (auto UnqualifiedBase = R.getAsSingle()) {
+  auto *TempSpec = cast(
+  UnqualifiedBase->getInjectedClassNameSpecialization());
+  TemplateName TN = TempSpec->getTemplateName();
+  for (auto const &Base : ClassDecl->bases()) {
+QualType BT = cast(Base.getType())->getNamedType();
+const auto *BaseTemplate = 
dyn_cast(BT);
+if (BaseTemplate && Context.hasSameTemplateName(
+BaseTemplate->getTemplateName(), TN)) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = BT;
+  break;
+}
+  }
 }
   }
 


Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- clang/test/SemaTemplate/ms-unqualified-base-class.cpp
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -83,3 +83,37 @@
 
   return I;
 }
+
+template  class Vec {}; // expected-note {{template is declared here}}
+
+template  class Index : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data member or base class}}
+  Index() : Vec() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template class Index<0>;
+
+template  class Array : public Vec {
+  // after-error@+1 {{member initializer 'Vec' does not name a non-static data member or base class}}
+  Array() : Vec() {} // before-warning {{unqualified base initializer of class 

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource marked an inline comment as done.
frederic-tingaud-sonarsource added a comment.

Hi @NoQ ,
Do the changes correspond to what you had in mind?


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

https://reviews.llvm.org/D126534

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

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 439722.
frederic-tingaud-sonarsource added a comment.

Adding an additional false negative fixed by this patch.


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

https://reviews.llvm.org/D126534

Files:
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
 // RUN:  -verify=non-nested,nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -verify=non-nested,nested %s
 
 //===--===//
 // Basic dead store checking (but in C++ mode).
@@ -33,6 +37,8 @@
   (void)y;
   if ((y = make_int())) // nested-warning {{Although the value stored}}
 return;
+
+  auto z = "text"; // non-nested-warning {{never read}}
 }
 
 //===--===//
@@ -52,6 +58,18 @@
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  // All these calls might have side effects in the opaque constructor
+  TestConstructor tc1 = x;// no-warning potential side effects
+  TestConstructor tc2 = TestConstructor(x);   // no-warning potential side effects
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning potential side effects
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning potential side effects
+}
+
 //===--===//
 // Dead store checking involving CXXTemporaryExprs
 //===--===//
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,8 @@
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
-const BinaryOperator *BO =
-  dyn_cast(Ex->IgnoreParenCasts());
+Ex = Ex->IgnoreParenCasts();
+const BinaryOperator *BO = dyn_cast(Ex);
 if (!BO)
   break;
 BinaryOperatorKind Op = BO->getOpcode();
@@ -331,8 +331,7 @@
   // Special case: check for assigning null to a pointer.
   //  This is a common form of defensive programming.
   const Expr *RHS =
-LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-  RHS = RHS->IgnoreParenCasts();
+  LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
 
   QualType T = VD->getType();
   if (T.isVolatileQualified())
@@ -415,8 +414,7 @@
   if (isConstant(E))
 return;
 
-  if (const DeclRefExpr *DRE =
-  dyn_cast(E->IgnoreParenCasts()))
+  if (const DeclRefExpr *DRE = dyn_cast(E))
 if (const VarDecl *VD = dyn_cast(DRE->getDecl())) {
   // Special case: check for initialization from constant
   //  variables.
@@ -438,7 +436,7 @@
 
   PathDiagnosticLocation Loc =
 PathDiagnosticLocation::create(V, BR.getSourceManager());
-  Report(V, DeadInit, Loc, E->getSourceRange());
+  Report(V, DeadInit, Loc, V->getInit()->getSourceRange());
 }
   }
 }
@@ -450,8 +448,9 @@
   bool isConstant(const InitListExpr *Candidate) const {
 // We consider init list to be constant if each member of the list can be
 // interpreted as constant.
-return llvm::all_of(Candidate->inits(),
-[this](const Expr *Init) { return isConstant(Init); });
+return llvm::all_of(Candidate->inits(), [this](const Expr *Init) {
+  return isConstant(Init->IgnoreParenCasts());
+});
   }
 
   /// Return true if the given expression can be interpreted as constant
@@ -461,7 +460,7 @@
   return true;
 
 // We should also allow defensive initialization of structs, i.e. { 0 }
-if (const auto *ILE = dyn_cast(E->IgnoreParenCasts())) {
+if (const auto *ILE = dyn_cast(E)) {
   return isConstant(ILE);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 436766.
frederic-tingaud-sonarsource added a comment.

Update diagnostic location to highlight the whole content of the initialization.


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

https://reviews.llvm.org/D126534

Files:
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/dead-stores.cpp


Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11   
 \
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code   
 \
 // RUN:  -verify=non-nested,nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17   
 \
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code   
 \
+// RUN:  -verify=non-nested,nested %s
 
 
//===--===//
 // Basic dead store checking (but in C++ mode).
@@ -52,6 +56,18 @@
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  // All these calls might have side effects in the opaque constructor
+  TestConstructor tc1 = x;// no-warning potential side 
effects
+  TestConstructor tc2 = TestConstructor(x);   // no-warning potential side 
effects
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning potential side 
effects
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning potential side 
effects
+}
+
 
//===--===//
 // Dead store checking involving CXXTemporaryExprs
 
//===--===//
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,8 @@
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
-const BinaryOperator *BO =
-  dyn_cast(Ex->IgnoreParenCasts());
+Ex = Ex->IgnoreParenCasts();
+const BinaryOperator *BO = dyn_cast(Ex);
 if (!BO)
   break;
 BinaryOperatorKind Op = BO->getOpcode();
@@ -331,8 +331,7 @@
   // Special case: check for assigning null to a pointer.
   //  This is a common form of defensive programming.
   const Expr *RHS =
-LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-  RHS = RHS->IgnoreParenCasts();
+  LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
 
   QualType T = VD->getType();
   if (T.isVolatileQualified())
@@ -415,8 +414,7 @@
   if (isConstant(E))
 return;
 
-  if (const DeclRefExpr *DRE =
-  dyn_cast(E->IgnoreParenCasts()))
+  if (const DeclRefExpr *DRE = dyn_cast(E))
 if (const VarDecl *VD = dyn_cast(DRE->getDecl())) {
   // Special case: check for initialization from constant
   //  variables.
@@ -438,7 +436,7 @@
 
   PathDiagnosticLocation Loc =
 PathDiagnosticLocation::create(V, BR.getSourceManager());
-  Report(V, DeadInit, Loc, E->getSourceRange());
+  Report(V, DeadInit, Loc, V->getInit()->getSourceRange());
 }
   }
 }
@@ -450,8 +448,9 @@
   bool isConstant(const InitListExpr *Candidate) const {
 // We consider init list to be constant if each member of the list can be
 // interpreted as constant.
-return llvm::all_of(Candidate->inits(),
-[this](const Expr *Init) { return isConstant(Init); });
+return llvm::all_of(Candidate->inits(), [this](const Expr *Init) {
+  return isConstant(Init->IgnoreParenCasts());
+});
   }
 
   /// Return true if the given expression can be interpreted as constant
@@ -461,7 +460,7 @@
   return true;
 
 // We should also allow defensive initialization of structs, i.e. { 0 }
-if (const auto *ILE = dyn_cast(E->IgnoreParenCasts())) {
+if (const auto *ILE = dyn_cast(E)) {
   return isConstant(ILE);
 }
 


Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
 // RUN:  -verify=non-nested,nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -s

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

It might also be interesting to think about cases that went through transitive 
assignment stripping when talking about the diagnostic position:
Before my patch, a dead store on variable `a` was displayed as follows:

  B b;
  A a = static_cast(b = createB());
   ^ 

(I'm switching back to C++ because I'm not sure exactly how legal such a thing 
would be in objective-C).
With the current fix, that doesn't change.
It might be clearer for the user to either put the diagnostic on the whole 
assignment or at least the whole initialization.


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

https://reviews.llvm.org/D126534

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

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

Thanks @NoQ!

The change in behavior is indeed what I was mentioning in the summary. After 
discussing it with a colleague, it seemed to us that pointing to a more precise 
range where the object is allocated was an improvement. I understand your point 
about pointing at something that is nearer to the assignment, but in that case, 
wouldn't it make sense to point at the whole assignment?

  id obj1 = (__bridge_transfer id)CFCreateSomething(); // 
expected-warning{{never read}}
  ^~~~

We already detect the presence of a constructor, but to skip the report 
altogether as constructors could have undetected side effects.


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

https://reviews.llvm.org/D126534

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

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource marked an inline comment as done.
frederic-tingaud-sonarsource added a comment.

Thanks for looking at the PR!

In D126534#3542402 , @steakhal wrote:

> You also mentioned this copy elision, but I don't see anywhere checks for the 
> standard version in code. I would expect that your change should apply only 
> to C++17 and above.

The false-positive due to copy elision in C++17 is the main pain point that 
this patch fixes, which is why I mention it in the title, but it does in fact 
also solve other cases for all C++ versions if there are parentheses or casts 
involved.


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

https://reviews.llvm.org/D126534

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

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 432558.
frederic-tingaud-sonarsource added a comment.

Document tests better


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

https://reviews.llvm.org/D126534

Files:
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
 // RUN:  -verify=non-nested,nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -verify=non-nested,nested %s
 
 //===--===//
 // Basic dead store checking (but in C++ mode).
@@ -52,6 +56,18 @@
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  // All these calls might have side effects in the opaque constructor
+  TestConstructor tc1 = x;// no-warning potential side effects
+  TestConstructor tc2 = TestConstructor(x);   // no-warning potential side effects
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning potential side effects
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning potential side effects
+}
+
 //===--===//
 // Dead store checking involving CXXTemporaryExprs
 //===--===//
Index: clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
@@ -432,7 +432,7 @@

 
  line139
- col13
+ col35
  file0
 
 
@@ -500,7 +500,7 @@

 
  line144
- col13
+ col33
  file0
 
 
@@ -568,7 +568,7 @@

 
  line145
- col13
+ col26
  file0
 
 
@@ -636,7 +636,7 @@

 
  line146
- col13
+ col33
  file0
 
 
@@ -1046,7 +1046,7 @@

 
  line150
- col19
+ col48
  file0
 
 
@@ -1114,7 +1114,7 @@

 
  line151
- col21
+ col52
  file0
 
 
@@ -1182,7 +1182,7 @@

 
  line152
- col19
+ col39
  file0
 
 
@@ -1250,7 +1250,7 @@

 
  line153
- col21
+ col43
  file0
 
 
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,8 @@
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
-const BinaryOperator *BO =
-  dyn_cast(Ex->IgnoreParenCasts());
+Ex = Ex->IgnoreParenCasts();
+const BinaryOperator *BO = dyn_cast(Ex);
 if (!BO)
   break;
 BinaryOperatorKind Op = BO->getOpcode();
@@ -331,8 +331,7 @@
   // Special case: check for assigning null to a pointer.
   //  This is a common form of defensive programming.
   const Expr *RHS =
-LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-  RHS = RHS->IgnoreParenCasts();
+  LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
 
   QualType T = VD->getType();
   if (T.isVolatileQualified())
@@ -415,8 +414,7 @@
   if (isConstant(E))
 return;
 
-  if (const DeclRefExpr *DRE =
-  dyn_cast(E->IgnoreParenCasts()))
+  if (const DeclRefExpr *DRE = dyn_cast(E))
 if (const VarDecl *VD = dyn_cast(DRE->getDecl())) {
   // Special case: check for initialization from constant
   //  variables.
@@ -450,8 +448,9 @@
   bool isConstant(const InitListExpr *Candidate) const {
 // We consider init list to be constant if each member of the list can be
 // interpreted as constant.
-return llvm::all_of(Candidate->inits(),
-[this](const Expr *Init) { return isConstant(Init); });
+return llvm::al

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 432552.
frederic-tingaud-sonarsource added a comment.

Fix formatting


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

https://reviews.llvm.org/D126534

Files:
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
 // RUN:  -verify=non-nested,nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -verify=non-nested,nested %s
 
 //===--===//
 // Basic dead store checking (but in C++ mode).
@@ -52,6 +56,17 @@
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  TestConstructor tc1 = x;// no-warning
+  TestConstructor tc2 = TestConstructor(x);   // no-warning
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning
+}
+
 //===--===//
 // Dead store checking involving CXXTemporaryExprs
 //===--===//
Index: clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
@@ -432,7 +432,7 @@

 
  line139
- col13
+ col35
  file0
 
 
@@ -500,7 +500,7 @@

 
  line144
- col13
+ col33
  file0
 
 
@@ -568,7 +568,7 @@

 
  line145
- col13
+ col26
  file0
 
 
@@ -636,7 +636,7 @@

 
  line146
- col13
+ col33
  file0
 
 
@@ -1046,7 +1046,7 @@

 
  line150
- col19
+ col48
  file0
 
 
@@ -1114,7 +1114,7 @@

 
  line151
- col21
+ col52
  file0
 
 
@@ -1182,7 +1182,7 @@

 
  line152
- col19
+ col39
  file0
 
 
@@ -1250,7 +1250,7 @@

 
  line153
- col21
+ col43
  file0
 
 
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,8 @@
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
-const BinaryOperator *BO =
-  dyn_cast(Ex->IgnoreParenCasts());
+Ex = Ex->IgnoreParenCasts();
+const BinaryOperator *BO = dyn_cast(Ex);
 if (!BO)
   break;
 BinaryOperatorKind Op = BO->getOpcode();
@@ -331,8 +331,7 @@
   // Special case: check for assigning null to a pointer.
   //  This is a common form of defensive programming.
   const Expr *RHS =
-LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-  RHS = RHS->IgnoreParenCasts();
+  LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
 
   QualType T = VD->getType();
   if (T.isVolatileQualified())
@@ -415,8 +414,7 @@
   if (isConstant(E))
 return;
 
-  if (const DeclRefExpr *DRE =
-  dyn_cast(E->IgnoreParenCasts()))
+  if (const DeclRefExpr *DRE = dyn_cast(E))
 if (const VarDecl *VD = dyn_cast(DRE->getDecl())) {
   // Special case: check for initialization from constant
   //  variables.
@@ -450,8 +448,9 @@
   bool isConstant(const InitListExpr *Candidate) const {
 // We consider init list to be constant if each member of the list can be
 // interpreted as constant.
-return llvm::all_of(Candidate->inits(),
-[this](const Expr *Init) { return isConstant(Init); });
+return llvm::all_of(Candidate->inits(), [this](const Expr *Init) {
+  return isConstant(Init->IgnoreParenCasts());
+});
   }
 
   /// Return true if the given expression can be i

[PATCH] D126534: Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: dcoughlin.
Herald added a subscriber: martong.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Dead store detection automatically checks that an expression is a 
CXXConstructor and skips it because of potential side effects. In C++17, with 
guaranteed copy elision, this check can fail because we actually receive the 
implicit cast of a CXXConstructor.
Most checks in the dead store analysis were already stripping all casts and 
parenthesis and those that weren't were either forgotten (like the constructor) 
or would not suffer from it, so this patch proposes to factorize the stripping.
It has an impact on where the dead store warning is reported in the case of an 
explicit cast, from

  auto a = static_cast(A());
   ^~~

to

  auto a = static_cast(A());
  ^~~

which we think is an improvement.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126534

Files:
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -12,6 +12,10 @@
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
 // RUN:  -verify=non-nested,nested %s
 
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -verify=non-nested,nested %s
+
 //===--===//
 // Basic dead store checking (but in C++ mode).
 //===--===//
@@ -52,6 +56,17 @@
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  TestConstructor tc1 = x; // no-warning
+  TestConstructor tc2 = TestConstructor(x);   // no-warning
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning
+}
+
 //===--===//
 // Dead store checking involving CXXTemporaryExprs
 //===--===//
Index: clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
@@ -432,7 +432,7 @@

 
  line139
- col13
+ col35
  file0
 
 
@@ -500,7 +500,7 @@

 
  line144
- col13
+ col33
  file0
 
 
@@ -568,7 +568,7 @@

 
  line145
- col13
+ col26
  file0
 
 
@@ -636,7 +636,7 @@

 
  line146
- col13
+ col33
  file0
 
 
@@ -1046,7 +1046,7 @@

 
  line150
- col19
+ col48
  file0
 
 
@@ -1114,7 +1114,7 @@

 
  line151
- col21
+ col52
  file0
 
 
@@ -1182,7 +1182,7 @@

 
  line152
- col19
+ col39
  file0
 
 
@@ -1250,7 +1250,7 @@

 
  line153
- col21
+ col43
  file0
 
 
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,9 @@
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
+Ex = Ex->IgnoreParenCasts();
 const BinaryOperator *BO =
-  dyn_cast(Ex->IgnoreParenCasts());
+  dyn_cast(Ex);
 if (!BO)
   break;
 BinaryOperatorKind Op = BO->getOpcode();
@@ -332,7 +333,6 @@
   //  This is a common form of defensive programming.
   const Expr *RHS =
 LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-  RHS = RHS->IgnoreParenCasts();
 
   QualType T = VD->getType();
   if (T.isVolatileQualified())
@@ -416,7 +416,7 @@
 return;
 
   if (const DeclRefExpr *DRE =
-  dyn_cast(E->IgnoreParenCasts()))
+  dyn_cast(E))
 if (c

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

@rnk Here is the test case that was broken: https://godbolt.org/z/Gef7PcqMf

One point where I was wrong in my initial analysis is that the behavior doesn't 
actually change based on `/std:c++17` VS `/std:c++latest`, but based on 
`/permissive` VS `/permissive-` (which is related because `/std:c++latest` 
automatically defaults on `/permissive-`). Other MSVC extensions should also 
depend on `/permissive`, such as clang-cl handles friend classes visibility or 
`ext_unqualified_base_class.`

I saw that clang-cl already parses the `/permissive` flag and translates it to 
`OPT__SLASH_Zc_twoPhase_` and `OPT_fno_operator_names`. Would it make sense to 
cover these extensions too? That would ensure conformant code is not broken by 
accident while giving people the possibility to parse their MSVC-specific code 
with more of its non-conformant specificities. But it does indeed increase the 
testing complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

I realized that this current patch did in fact mimic MSVC behavior up to its 
mishandling on ADL. I understand that it is not in the spirit of clang's 
philosophy for MSVC compatibility to have that, but the fixed version I was 
about to propose would stray away from MSVC behavior, which doesn't really make 
sense either.
So instead I think the right approach should be to revert and see later whether 
we could put it back under the hood of another flag.
Is there currently a flag that could be used for "potentially C++ compliant 
breaking MSVC compatibility"? If there isn't, would it make sense to propose 
one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D125529: Revert MSVC compatibility feature that breaks conformant code

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added reviewers: rnk, thakis.
Herald added a reviewer: shafik.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The fix at https://reviews.llvm.org/D124613 mimics MSVC behavior with friend 
declarations, but it does break some C++ conformant code that was built with 
-fms-compatibility. This fix reverts it to avoid causing more problems.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125529

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -2658,10 +2658,7 @@
   getTuDecl("struct X { friend void f(); };", Lang_CXX03, "input0.cc");
   auto *FromD = FirstDeclMatcher().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromD->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  // Before CXX20, MSVC treats friend function declarations as function
-  // declarations
-  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
-FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  ASSERT_FALSE(FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   {
 auto FromName = FromD->getDeclName();
 auto *Class = FirstDeclMatcher().match(FromTU, ClassPattern);
@@ -2705,10 +2702,7 @@
   auto *FromNormal =
   LastDeclMatcher().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  // Before CXX20, MSVC treats friend function declarations as function
-  // declarations
-  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
-FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  ASSERT_FALSE(FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   ASSERT_TRUE(FromNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
 
@@ -2799,10 +2793,7 @@
 
   ASSERT_TRUE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  // Before CXX20, MSVC treats friend function declarations as function
-  // declarations
-  ASSERT_EQ(FromFriendTU->getLangOpts().MSVCCompat,
-FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  ASSERT_FALSE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_TRUE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   auto LookupRes = FromNormalTU->noload_lookup(FromNormalName);
   ASSERT_TRUE(LookupRes.isSingleResult());
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- clang/test/SemaCXX/ms-friend-function-decl.cpp
+++ /dev/null
@@ -1,45 +0,0 @@
-// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
-#if __cplusplus < 202002L
-// expected-no-diagnostics
-#endif
-
-namespace ns {
-
-class C {
-public:
-  template 
-  friend void funtemp();
-
-  friend void fun();
-
-  void test() {
-::ns::fun(); // modern-error {{no member named 'fun' in namespace 'ns'}}
-
-// modern-error@+3 {{no member named 'funtemp' in namespace 'ns'}}
-// modern-error@+2 {{expected '(' for function-style cast or type construction}}
-// modern-error@+1 {{expected expression}}
-::ns::funtemp();
-  }
-};
-
-void fun() {
-}
-
-template 
-void funtemp() {}
-
-} // namespace ns
-
-class Glob {
-public:
-  friend void funGlob();
-
-  void test() {
-funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
-  }
-};
-
-void funGlob() {
-}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9632,15 +9632,11 @@
 }
 
 if (isFriend) {
-  // In MSVC mode for older versions of the standard, friend function
-  // declarations behave as declarations
-  bool PerformFriendInjection =
-  getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20;
   if (FunctionTemplate) {
-FunctionTemplate->setObjectOfFriendDecl(PerformFriendInjection);
+FunctionTemplate->setObjectOfFriendDecl();
 FunctionTemplate->setAccess(AS_public);
   }
-  NewFD->setObjectOfFriendDecl(PerformFriendInjection);
+  NewFD->setObjectOfFriendDecl();
   NewFD->setAccess(AS_public);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailma

[PATCH] D125526: Fix ADL leakage due to MSVC compatibility flag

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource abandoned this revision.
frederic-tingaud-sonarsource added a comment.

After checking, I realize that the previous fix was really mimicking MSVC 
behavior up to its mishandling of standard compliant code.
https://godbolt.org/z/cEPGfMz3f
I'll revert it for now and try to see if there is a way to have it under 
another flag or something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125526

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

Sorry for the delay, the fix is here: https://reviews.llvm.org/D125526
If there is any risk or problem identified with this new fix that could delay 
it, I'll propose a revert of this one to avoid breaking more legitimate code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D125526: Fix ADL leakage due to MSVC compatibility flag

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Patch https://reviews.llvm.org/D124613 introduced an ADL leakage of friend 
functions when in MSVC compatibility mode. It broke some C++ compliant code. 
That patch fixes the problem introduced.

The original fix flagged friend function declarations as declarations, to 
follow MSVC behavior. The problem it introduced is that this same flag is used 
to do ADL and that made friend functions visible at the namespace level instead 
of them being visible at the class level only.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125526

Files:
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp


Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- clang/test/SemaCXX/ms-friend-function-decl.cpp
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -1,9 +1,5 @@
-// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern 
%s
-#if __cplusplus < 202002L
-// expected-no-diagnostics
-#endif
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only 
-verify=expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only 
-verify=modern,expected %s
 
 namespace ns {
 
@@ -43,3 +39,43 @@
 
 void funGlob() {
 }
+
+// At some point, considering friend declaration as full declarations broke 
ADL detection.
+// The following test checks that the friend declaration is only visible 
inside the class it's declared in.
+namespace check_adl {
+template 
+struct A {};
+
+struct NotADLVisible {
+  template 
+  friend constexpr bool operator==(A, NotADLVisible) noexcept {
+static_assert(sizeof(T) < 0, "This should never be instantiated");
+return false;
+  }
+
+  constexpr NotADLVisible(int) {}
+};
+} // namespace check_adl
+
+void shouldNotSeeAnyOperator() {
+  bool b = check_adl::A() == 2; // expected-error {{invalid operands to 
binary expression ('check_adl::A' and 'int')}}
+}
+
+// Check that we don't break the case of a function that is both visible to 
ADL and a friend
+namespace check_adl_compliant {
+struct A {};
+
+struct B {
+  friend constexpr bool operator==(A, B) noexcept;
+
+  constexpr B(int) {}
+};
+
+constexpr bool operator==(check_adl_compliant::A, check_adl_compliant::B) 
noexcept {
+  return false;
+}
+} // namespace check_adl_compliant
+
+void shouldSeeTheOperator() {
+  bool b = check_adl_compliant::A() == 2;
+}
Index: clang/lib/Sema/SemaLookup.cpp
===
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -3634,14 +3634,17 @@
   bool Visible = false;
   for (D = D->getMostRecentDecl(); D;
D = cast_or_null(D->getPreviousDecl())) {
-if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
-  if (isVisible(D)) {
+// We check the friend case first because in case of MSVC compatibility
+// a friend declaration is also flagged as Ordinary but we don't want
+// it to be visible outside its associated class.
+if (D->getFriendObjectKind()) {
+  auto *RD = cast(D->getLexicalDeclContext());
+  if (AssociatedClasses.count(RD) && isVisible(D)) {
 Visible = true;
 break;
   }
-} else if (D->getFriendObjectKind()) {
-  auto *RD = cast(D->getLexicalDeclContext());
-  if (AssociatedClasses.count(RD) && isVisible(D)) {
+} else if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
+  if (isVisible(D)) {
 Visible = true;
 break;
   }


Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- clang/test/SemaCXX/ms-friend-function-decl.cpp
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -1,9 +1,5 @@
-// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
-#if __cplusplus < 202002L
-// expected-no-diagnostics
-#endif
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern,expected %s
 
 namespace ns {
 
@@ -43,3 +39,43 @@
 
 void funGlob() {
 }
+
+// At some point, considering friend declaration as full declarations broke ADL detection.
+// The following test checks that the friend declaration is only visible inside the class it's declared in.

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

Regarding the "why":
Our tools (SonarQube / SonarCloud / SonarLint) use the clang front-end to parse 
our customer's code and find bugs/bad patterns in it. Said customer code can 
target various compilers including MSVC and we need to handle it as gracefully 
as possible.
When we find gaps between MSVC and clang-cl, we try to fix them and if we think 
the fix will have negligible memory/performance/complexity impact on clang and 
be useful to the community, we try to upstream them. It's true that this fix 
purpose is not to fix handling of MSVC standard headers, but there are more and 
more tools that also target existing MSVC code (clang-tidy, Clang Power Tools, 
etc.) thanks to this compatibility feature and we felt that it could be 
valuable.
We also try to always have a warning attached to MSVC extensions, but this one 
has two particularities: it builds on top of an already existing MSVC 
compatibility feature (PerformFriendInjection in Decl::setObjectOfFriendDecl) 
and there is no way (without a big refactoring), that would allow to warn 
correctly when the extension is actually used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

In D124613#3506740 , @thakis wrote:

> Should we revert this, at least for now, until the investigation is complete?

I started typing my message before you sent yours so it was not a direct 
answer. We are working on a fork of LLVM, so if you revert the fix for the 
moment, it will not put us in a bad position anyway. I'll let you judge on 
whether the current problem can remain until my fixing PR is reviewed or 
whether you prefer to revert immediately and for me to make a clean PR with the 
corrected version afterward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

Thanks a lot for pointing out the reproducer, it made the debug a lot easier.
I have pinpointed the problem to an unintended increase in ADL visibility for 
friend classes. I do have a test-case and fix for that. I'll make a last check 
tomorrow to make sure I'm not missing something important and push a 
pull-request.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

That is definitely not expected. I'm looking into it, but as this is my first 
time building chromium, I'm taking a bit more time finding my way around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 428044.
frederic-tingaud-sonarsource added a comment.

Fix formatting


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

https://reviews.llvm.org/D124845

Files:
  clang/include/clang/Analysis/ConstructionContext.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/cxxnewexpr-callback-inline.cpp
  clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
  clang/test/Analysis/cxxnewexpr-callback.cpp
  clang/test/Analysis/dtor.cpp

Index: clang/test/Analysis/dtor.cpp
===
--- clang/test/Analysis/dtor.cpp
+++ clang/test/Analysis/dtor.cpp
@@ -570,3 +570,32 @@
   // no-crash
 }
 } // namespace dtor_over_loc_concrete_int
+
+// Test overriden new/delete operators
+struct CustomOperators {
+  void *operator new(size_t count) {
+return malloc(count);
+  }
+
+  void operator delete(void *addr) {
+free(addr);
+  }
+
+private:
+  int i;
+};
+
+void compliant() {
+  auto *a = new CustomOperators();
+  delete a;
+}
+
+void overrideLeak() {
+  auto *a = new CustomOperators();
+} // expected-warning{{Potential leak of memory pointed to by 'a'}}
+
+void overrideDoubleDelete() {
+  auto *a = new CustomOperators();
+  delete a;
+  delete a; // expected-warning@581 {{Attempt to free released memory}}
+}
Index: clang/test/Analysis/cxxnewexpr-callback.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxxnewexpr-callback.cpp
@@ -0,0 +1,63 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=true,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-INLINE
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s  --check-prefixes=CHECK,CHECK-NO-INLINE
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+void *malloc(size_t);
+void free(void *);
+} // namespace std
+
+void *operator new(size_t size) { return std::malloc(size); }
+void operator delete(void *ptr) { std::free(ptr); }
+
+struct S {
+  S() {}
+  ~S() {}
+};
+
+void foo();
+
+void test() {
+  S *s = new S();
+  foo();
+  delete s;
+}
+
+/*
+void test() {
+  S *s = new S();
+// CHECK-INLINE:  PreCall (operator new)
+// CHECK-INLINE-NEXT: PreCall (std::malloc)
+// CHECK-INLINE-NEXT: PostCall (std::malloc)
+// CHECK-INLINE-NEXT: PostCall (operator new)
+// CHECK-INLINE-NEXT: NewAllocator
+// CHECK-NO-INLINE: PreCall (S::S)
+// CHECK-INLINE-NEXT: PreCall (S::S)
+// CHECK-NEXT: PostCall (S::S)
+// CHECK-NEXT: PreStmt
+// CHECK-NEXT: PostStmt
+  foo();
+// CHECK-NEXT: PreCall (foo)
+// CHECK-NEXT: PostCall (foo)
+  delete s;
+// CHECK-NEXT: PreCall (S::~S)
+// CHECK-NEXT: PostCall (S::~S)
+// CHECK-NEXT: PreCall (operator delete)
+// CHECK-INLINE-NEXT: PreCall (std::free)
+// CHECK-INLINE-NEXT: PostCall (std::free)
+// CHECK-NEXT: PostCall (operator delete)
+}
+
+void operator delete(void *ptr) {
+  std::free(ptr);
+// CHECK-NO-INLINE-NEXT: PreCall (std::free)
+// CHECK-NO-INLINE-NEXT: PostCall (std::free)
+}
+
+void *operator new(size_t size) {
+  return std::malloc(size);
+// CHECK-NO-INLINE-NEXT: PreCall (std::malloc)
+// CHECK-NO-INLINE-NEXT: PostCall (std::malloc)
+}
+*/
Index: clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
===
--- clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
+++ /dev/null
@@ -1,29 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s
-
-#include "Inputs/system-header-simulator-cxx.h"
-
-namespace std {
-  void *malloc(size_t);
-}
-
-void *operator new(size_t size) { return std::malloc(size); }
-
-struct S {
-  S() {}
-};
-
-void foo();
-
-void test() {
-  S *s = new S();
-  foo();
-}
-
-// CHECK:  PreCall (S::S)
-// CHECK-NEXT: PostCall (S::S)
-// CHECK-NEXT: PreStmt
-// CHECK-NEXT: PostStmt
-// CHECK-NEXT: PreCall (foo)
-// CHECK-NEXT: PostCall (foo)
-// CHECK-NEXT: PreCall (std::malloc)
-// CHECK-NEXT: PostCall (std::malloc)
Index: clang/test/Analysis/cxxnewexpr-callback-inl

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource marked an inline comment as not done.
frederic-tingaud-sonarsource added a comment.

As I don't have merge rights, would it be possible for you to merge it?


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

https://reviews.llvm.org/D124845

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

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource marked an inline comment as done.
frederic-tingaud-sonarsource added inline comments.



Comment at: clang/include/clang/Analysis/ConstructionContext.h:122-124
 assert(isa(E) || isa(E) ||
-   isa(E) || isa(E));
+   isa(E) || isa(E) ||
+   isa(E));

martong wrote:
> I think we could use the variadic `isa` template .
Because assert is a macro, the commas of variadic templates break it.


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

https://reviews.llvm.org/D124845

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

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 427332.
frederic-tingaud-sonarsource added a comment.

Applying recommendations


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

https://reviews.llvm.org/D124845

Files:
  clang/include/clang/Analysis/ConstructionContext.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/cxxnewexpr-callback-inline.cpp
  clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
  clang/test/Analysis/cxxnewexpr-callback.cpp
  clang/test/Analysis/dtor.cpp

Index: clang/test/Analysis/dtor.cpp
===
--- clang/test/Analysis/dtor.cpp
+++ clang/test/Analysis/dtor.cpp
@@ -570,3 +570,32 @@
   // no-crash
 }
 } // namespace dtor_over_loc_concrete_int
+
+// Test overriden new/delete operators
+struct CustomOperators {
+  void *operator new(size_t count) {
+return malloc(count);
+  }
+
+  void operator delete(void *addr) {
+free(addr);
+  }
+
+private:
+  int i;
+};
+
+void compliant() {
+  auto *a = new CustomOperators();
+  delete a;
+}
+
+void overrideLeak() {
+  auto *a = new CustomOperators();
+} // expected-warning{{Potential leak of memory pointed to by 'a'}}
+
+void overrideDoubleDelete() {
+  auto *a = new CustomOperators();
+  delete a;
+  delete a; // expected-warning@581 {{Attempt to free released memory}}
+}
Index: clang/test/Analysis/cxxnewexpr-callback.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxxnewexpr-callback.cpp
@@ -0,0 +1,63 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=true,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-INLINE
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s  --check-prefixes=CHECK,CHECK-NO-INLINE
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+  void *malloc(size_t);
+  void free(void *);
+}
+
+void *operator new(size_t size) { return std::malloc(size); }
+void operator delete(void *ptr) { std::free(ptr); }
+
+struct S {
+  S() {}
+  ~S() {}
+};
+
+void foo();
+
+void test() {
+  S *s = new S();
+  foo();
+  delete s;
+}
+
+/*
+void test() {
+  S *s = new S();
+// CHECK-INLINE:  PreCall (operator new)
+// CHECK-INLINE-NEXT: PreCall (std::malloc)
+// CHECK-INLINE-NEXT: PostCall (std::malloc)
+// CHECK-INLINE-NEXT: PostCall (operator new)
+// CHECK-INLINE-NEXT: NewAllocator
+// CHECK-NO-INLINE: PreCall (S::S)
+// CHECK-INLINE-NEXT: PreCall (S::S)
+// CHECK-NEXT: PostCall (S::S)
+// CHECK-NEXT: PreStmt
+// CHECK-NEXT: PostStmt
+  foo();
+// CHECK-NEXT: PreCall (foo)
+// CHECK-NEXT: PostCall (foo)
+  delete s;
+// CHECK-NEXT: PreCall (S::~S)
+// CHECK-NEXT: PostCall (S::~S)
+// CHECK-NEXT: PreCall (operator delete)
+// CHECK-INLINE-NEXT: PreCall (std::free)
+// CHECK-INLINE-NEXT: PostCall (std::free)
+// CHECK-NEXT: PostCall (operator delete)
+}
+
+void operator delete(void *ptr) {
+  std::free(ptr);
+// CHECK-NO-INLINE-NEXT: PreCall (std::free)
+// CHECK-NO-INLINE-NEXT: PostCall (std::free)
+}
+
+void *operator new(size_t size) {
+  return std::malloc(size);
+// CHECK-NO-INLINE-NEXT: PreCall (std::malloc)
+// CHECK-NO-INLINE-NEXT: PostCall (std::malloc)
+}
+*/
Index: clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
===
--- clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
+++ /dev/null
@@ -1,29 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s
-
-#include "Inputs/system-header-simulator-cxx.h"
-
-namespace std {
-  void *malloc(size_t);
-}
-
-void *operator new(size_t size) { return std::malloc(size); }
-
-struct S {
-  S() {}
-};
-
-void foo();
-
-void test() {
-  S *s = new S();
-  foo();
-}
-
-// CHECK:  PreCall (S::S)
-// CHECK-NEXT: PostCall (S::S)
-// CHECK-NEXT: PreStmt
-// CHECK-NEXT: PostStmt
-// CHECK-NEXT: PreCall (foo)
-// CHECK-NEXT: PostCall (foo)
-// CHECK-NEXT: PreCall (std::malloc)
-// CHECK-NEXT: PostCall (std::malloc)
Index: clang/test/Analysis/cxxnewexpr-callback-inline

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

By the way, in the current state of the code, there is no risk of such conflict 
between typo correction and UnqualifiedBase, because when an unqualified base 
exists the lookup result will not be empty.
That's why the test was not failing even without moving the compatibility first.


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

https://reviews.llvm.org/D124666

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

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 426944.
frederic-tingaud-sonarsource added a comment.

Sorry about the confusion, it makes sense. Fixed.


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

https://reviews.llvm.org/D124666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp

Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -0,0 +1,85 @@
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=after,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=after,expected %s
+
+template 
+class Base {
+};
+
+template 
+class Based {}; // Trying to trick the typo detection
+
+template 
+class Derived : public Base {
+public:
+  // after-error@+1 {{member initializer 'Base' does not name a non-static data member or base class}}
+  Derived() : Base() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+private:
+  int Baze; // Trying to trick the typo detection
+};
+
+template  struct AggregateBase {
+  T i;
+};
+
+template 
+struct AggregateDerived : public AggregateBase {
+  int i;
+
+  // after-error@+1 {{member initializer 'AggregateBase' does not name a non-static data member or base class}}
+  AggregateDerived(T j) : AggregateBase{4}, i{j} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+  int f() {
+return i + AggregateBase::i; // expected-warning {{use of undeclared identifier 'AggregateBase'; unqualified lookup into dependent bases of class template 'AggregateDerived' is a Microsoft extension}}
+  }
+};
+
+template  struct MultiTypesBase {
+};
+
+template 
+struct MultiTypesDerived : public MultiTypesBase {
+  // after-error@+1 {{member initializer 'MultiTypesBase' does not name a non-static data member or base class}}
+  MultiTypesDerived() : MultiTypesBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct IntegerBase {
+};
+
+template 
+struct IntegerDerived : public IntegerBase {
+  // after-error@+1 {{member initializer 'IntegerBase' does not name a non-static data member or base class}}
+  IntegerDerived() : IntegerBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct ConformingBase {
+  T i;
+};
+
+template 
+struct ConformingDerived : public ConformingBase {
+  int i;
+
+  ConformingDerived(T j) : ConformingBase{4}, i{j} {}
+  int f() {
+return i + ConformingBase::i;
+  }
+};
+
+int main() {
+  int I;
+  Derived t;
+
+  AggregateDerived AD{2};
+  AD.AggregateBase::i = 3;
+  I = AD.f();
+
+  MultiTypesDerived MTD;
+
+  IntegerDerived<4> ID;
+
+  ConformingDerived CD{2};
+  I = CD.f();
+
+  return I;
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4307,6 +4307,15 @@
 }
   }
 
+  if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
+auto UnqualifiedBase = R.getAsSingle();
+if (UnqualifiedBase) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+}
+  }
+
   // If no results were found, try to correct typos.
   TypoCorrection Corr;
   MemInitializerValidatorCCC CCC(ClassDecl);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5508,6 +5508,9 @@
 def ext_found_later_in_class : ExtWarn<
   "use of member %0 before its declaration is a Microsoft extension">,
   InGroup;
+def ext_unqualified_base_class : ExtWarn<
+  "unqualified base initializer of class templates is a Microsoft extension">,
+  InGroup;
 def note_dependent_member_use : Note<
   "must qualify identifier to find this declaration in dependent base class">;
 def err_not_found_by_two_phase_lookup : Error<"call to function %0 that is neither "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 426759.
frederic-tingaud-sonarsource added a comment.

Added two similar names in the tests, to try to trick the typo correction.


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

https://reviews.llvm.org/D124666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp

Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -0,0 +1,85 @@
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=after,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=after,expected %s
+
+template 
+class Base {
+};
+
+template 
+class Based {}; // Trying to trick the typo detection
+
+template 
+class Derived : public Base {
+public:
+  // after-error@+1 {{member initializer 'Base' does not name a non-static data member or base class}}
+  Derived() : Base() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+private:
+  int Baze; // Trying to trick the typo detection
+};
+
+template  struct AggregateBase {
+  T i;
+};
+
+template 
+struct AggregateDerived : public AggregateBase {
+  int i;
+
+  // after-error@+1 {{member initializer 'AggregateBase' does not name a non-static data member or base class}}
+  AggregateDerived(T j) : AggregateBase{4}, i{j} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+  int f() {
+return i + AggregateBase::i; // expected-warning {{use of undeclared identifier 'AggregateBase'; unqualified lookup into dependent bases of class template 'AggregateDerived' is a Microsoft extension}}
+  }
+};
+
+template  struct MultiTypesBase {
+};
+
+template 
+struct MultiTypesDerived : public MultiTypesBase {
+  // after-error@+1 {{member initializer 'MultiTypesBase' does not name a non-static data member or base class}}
+  MultiTypesDerived() : MultiTypesBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct IntegerBase {
+};
+
+template 
+struct IntegerDerived : public IntegerBase {
+  // after-error@+1 {{member initializer 'IntegerBase' does not name a non-static data member or base class}}
+  IntegerDerived() : IntegerBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct ConformingBase {
+  T i;
+};
+
+template 
+struct ConformingDerived : public ConformingBase {
+  int i;
+
+  ConformingDerived(T j) : ConformingBase{4}, i{j} {}
+  int f() {
+return i + ConformingBase::i;
+  }
+};
+
+int main() {
+  int I;
+  Derived t;
+
+  AggregateDerived AD{2};
+  AD.AggregateBase::i = 3;
+  I = AD.f();
+
+  MultiTypesDerived MTD;
+
+  IntegerDerived<4> ID;
+
+  ConformingDerived CD{2};
+  I = CD.f();
+
+  return I;
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4345,6 +4345,15 @@
 }
   }
 
+  if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
+auto UnqualifiedBase = R.getAsSingle();
+if (UnqualifiedBase) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+}
+  }
+
   if (!TyD && BaseType.isNull()) {
 Diag(IdLoc, diag::err_mem_init_not_member_or_class)
   << MemberOrBase << SourceRange(IdLoc,Init->getSourceRange().getEnd());
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5508,6 +5508,9 @@
 def ext_found_later_in_class : ExtWarn<
   "use of member %0 before its declaration is a Microsoft extension">,
   InGroup;
+def ext_unqualified_base_class : ExtWarn<
+  "unqualified base initializer of class templates is a Microsoft extension">,
+  InGroup;
 def note_dependent_member_use : Note<
   "must qualify identifier to find this declaration in dependent base class">;
 def err_not_found_by_two_phase_lookup : Error<"call to function %0 that is neither "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: dcoughlin.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, 
Szelethus, a.sidorin, baloghadamsoftware.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Currently, when encountering an overridden new operator, the StaticAnalyzer 
will inline it when possible, while an overridden delete operator will never 
be, which leads us to false positives like the following:

  struct CustomOperators {
void *operator new(size_t count) {
  return malloc(count);
}
  
void operator delete(void *addr) {
  free(addr);
}
  };
  
  void compliant() {
auto *a = new CustomOperators();
delete a; // warning{{Potential leak of memory pointed to by 'a'}}
  }

This patch restores the symmetry between how operator new and operator delete 
are handled by also inlining the content of operator delete when possible.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124845

Files:
  clang/include/clang/Analysis/ConstructionContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/cxxnewexpr-callback-inline.cpp
  clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
  clang/test/Analysis/dtor.cpp

Index: clang/test/Analysis/dtor.cpp
===
--- clang/test/Analysis/dtor.cpp
+++ clang/test/Analysis/dtor.cpp
@@ -570,3 +570,32 @@
   // no-crash
 }
 } // namespace dtor_over_loc_concrete_int
+
+// Test overriden new/delete operators
+struct CustomOperators {
+  void *operator new(size_t count) {
+return malloc(count);
+  }
+
+  void operator delete(void *addr) {
+free(addr);
+  }
+
+private:
+  int i;
+};
+
+void compliant() {
+  auto *a = new CustomOperators();
+  delete a;
+}
+
+void overrideLeak() {
+  auto *a = new CustomOperators();
+} // expected-warning{{Potential leak of memory pointed to by 'a'}}
+
+void overrideDoubleDelete() {
+  auto *a = new CustomOperators();
+  delete a;
+  delete a; // expected-warning@581 {{Attempt to free released memory}}
+}
Index: clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
===
--- clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
+++ clang/test/Analysis/cxxnewexpr-callback-noinline.cpp
@@ -3,13 +3,16 @@
 #include "Inputs/system-header-simulator-cxx.h"
 
 namespace std {
-  void *malloc(size_t);
-}
+void *malloc(size_t);
+void free(void *);
+} // namespace std
 
 void *operator new(size_t size) { return std::malloc(size); }
+void operator delete(void *ptr) { std::free(ptr); }
 
 struct S {
   S() {}
+  ~S() {}
 };
 
 void foo();
@@ -17,13 +20,35 @@
 void test() {
   S *s = new S();
   foo();
+  delete s;
 }
 
+/*
+void test() {
+  S *s = new S();
 // CHECK:  PreCall (S::S)
 // CHECK-NEXT: PostCall (S::S)
 // CHECK-NEXT: PreStmt
 // CHECK-NEXT: PostStmt
+  foo();
 // CHECK-NEXT: PreCall (foo)
 // CHECK-NEXT: PostCall (foo)
+  delete s;
+// CHECK-NEXT: PreCall (S::~S)
+// CHECK-NEXT: PostCall (S::~S)
+// CHECK-NEXT: PreCall (operator delete)
+// CHECK-NEXT: PostCall (operator delete)
+}
+
+void operator delete(void *ptr) {
+  std::free(ptr);
+// CHECK-NEXT: PreCall (std::free)
+// CHECK-NEXT: PostCall (std::free)
+}
+
+void *operator new(size_t size) {
+  return std::malloc(size);
 // CHECK-NEXT: PreCall (std::malloc)
 // CHECK-NEXT: PostCall (std::malloc)
+}
+*/
Index: clang/test/Analysis/cxxnewexpr-callback-inline.cpp
===
--- clang/test/Analysis/cxxnewexpr-callback-inline.cpp
+++ clang/test/Analysis/cxxnewexpr-callback-inline.cpp
@@ -4,12 +4,15 @@
 
 namespace std {
   void *malloc(size_t);
+  void free(void *);
 }
 
 void *operator new(size_t size) { return std::malloc(size); }
+void operator delete(void *ptr) { std::free(ptr); }
 
 struct S {
   S() {}
+  ~S() {}
 };
 
 void foo();
@@ -17,8 +20,12 @@
 void test() {
   S *s = new S();
   foo();
+  delete s;
 }
 
+/*
+void test() {
+  S *s = new S();
 // CHECK:  PreCall (operator new)
 // CHECK-NEXT: PreCall (std::malloc)
 // CHECK-NEXT: PostCall (std::malloc)
@@ -28,5 +35,15 @@
 // CHECK-NEXT: PostCall (S::S)
 // CHECK-NEXT: PreStmt
 // CHECK-NEXT: PostStmt
+  foo();
 // CHECK-NEXT: PreCall (foo)
 // CHECK-NEXT: PostCall (foo)
+  delete s;
+// CHECK-NEXT: PreCall (S::~S)
+// CHECK-NEXT: PostCall (S::~S)
+// CHECK-NEXT: PreCall (operator delete)
+// CHECK-NEXT: PreCall (std::free)
+// CHECK-NEXT: PostCall (std::free)
+// CHECK-NEXT: PostCall (operator delete)
+}
+*/
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEn

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 426022.

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

https://reviews.llvm.org/D124666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp

Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=after,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=after,expected %s
+
+template 
+class Base {
+};
+
+template 
+class Derived : public Base {
+public:
+  // after-error@+1 {{member initializer 'Base' does not name a non-static data member or base class}}
+  Derived() : Base() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct AggregateBase {
+  T i;
+};
+
+template 
+struct AggregateDerived : public AggregateBase {
+  int i;
+
+  // after-error@+1 {{member initializer 'AggregateBase' does not name a non-static data member or base class}}
+  AggregateDerived(T j) : AggregateBase{4}, i{j} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+  int f() {
+return i + AggregateBase::i; // expected-warning {{use of undeclared identifier 'AggregateBase'; unqualified lookup into dependent bases of class template 'AggregateDerived' is a Microsoft extension}}
+  }
+};
+
+template  struct MultiTypesBase {
+};
+
+template 
+struct MultiTypesDerived : public MultiTypesBase {
+  // after-error@+1 {{member initializer 'MultiTypesBase' does not name a non-static data member or base class}}
+  MultiTypesDerived() : MultiTypesBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct IntegerBase {
+};
+
+template 
+struct IntegerDerived : public IntegerBase {
+  // after-error@+1 {{member initializer 'IntegerBase' does not name a non-static data member or base class}}
+  IntegerDerived() : IntegerBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct ConformingBase {
+  T i;
+};
+
+template 
+struct ConformingDerived : public ConformingBase {
+  int i;
+
+  ConformingDerived(T j) : ConformingBase{4}, i{j} {}
+  int f() {
+return i + ConformingBase::i;
+  }
+};
+
+int main() {
+  int I;
+  Derived t;
+
+  AggregateDerived AD{2};
+  AD.AggregateBase::i = 3;
+  I = AD.f();
+
+  MultiTypesDerived MTD;
+
+  IntegerDerived<4> ID;
+
+  ConformingDerived CD{2};
+  I = CD.f();
+
+  return I;
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4345,6 +4345,15 @@
 }
   }
 
+  if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
+auto UnqualifiedBase = R.getAsSingle();
+if (UnqualifiedBase) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+}
+  }
+
   if (!TyD && BaseType.isNull()) {
 Diag(IdLoc, diag::err_mem_init_not_member_or_class)
   << MemberOrBase << SourceRange(IdLoc,Init->getSourceRange().getEnd());
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5508,6 +5508,9 @@
 def ext_found_later_in_class : ExtWarn<
   "use of member %0 before its declaration is a Microsoft extension">,
   InGroup;
+def ext_unqualified_base_class : ExtWarn<
+  "unqualified base initializer of class templates is a Microsoft extension">,
+  InGroup;
 def note_dependent_member_use : Note<
   "must qualify identifier to find this declaration in dependent base class">;
 def err_not_found_by_two_phase_lookup : Error<"call to function %0 that is neither "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 426021.

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

https://reviews.llvm.org/D124666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp

Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=after,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=after,expected %s
+
+template 
+class Base {
+};
+
+template 
+class Derived : public Base {
+public:
+  // after-error@+1 {{member initializer 'Base' does not name a non-static data member or base class}}
+  Derived() : Base() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template struct AggregateBase {
+  T i;
+};
+
+template
+struct  AggregateDerived : public AggregateBase {
+  int i;
+
+  // after-error@+1 {{member initializer 'AggregateBase' does not name a non-static data member or base class}}
+  AggregateDerived(T j) : AggregateBase{ 4 }, i{ j } {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+  int f() {
+return i + AggregateBase::i; // expected-warning {{use of undeclared identifier 'AggregateBase'; unqualified lookup into dependent bases of class template 'AggregateDerived' is a Microsoft extension}}
+  }
+};
+
+template  struct MultiTypesBase {
+};
+
+template 
+struct MultiTypesDerived : public MultiTypesBase {
+  // after-error@+1 {{member initializer 'MultiTypesBase' does not name a non-static data member or base class}}
+  MultiTypesDerived() : MultiTypesBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct IntegerBase {
+};
+
+template 
+struct IntegerDerived : public IntegerBase {
+  // after-error@+1 {{member initializer 'IntegerBase' does not name a non-static data member or base class}}
+  IntegerDerived() : IntegerBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template struct ConformingBase {
+  T i;
+};
+
+template
+struct  ConformingDerived : public ConformingBase {
+  int i;
+
+  ConformingDerived(T j) : ConformingBase{ 4 }, i{ j } {}
+  int f() {
+return i + ConformingBase::i;
+  }
+};
+
+int main() {
+  int I;
+  Derived t;
+
+  AggregateDerived AD{ 2 };
+  AD.AggregateBase::i = 3;
+  I = AD.f();
+
+  MultiTypesDerived MTD;
+
+  IntegerDerived<4> ID;
+
+  ConformingDerived CD{ 2 };
+  I = CD.f();
+
+  return I;
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4345,6 +4345,15 @@
 }
   }
 
+  if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
+auto UnqualifiedBase = R.getAsSingle();
+if (UnqualifiedBase) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+<< SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+}
+  }
+
   if (!TyD && BaseType.isNull()) {
 Diag(IdLoc, diag::err_mem_init_not_member_or_class)
   << MemberOrBase << SourceRange(IdLoc,Init->getSourceRange().getEnd());
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5508,6 +5508,9 @@
 def ext_found_later_in_class : ExtWarn<
   "use of member %0 before its declaration is a Microsoft extension">,
   InGroup;
+def ext_unqualified_base_class : ExtWarn<
+  "unqualified base initializer of class templates is a Microsoft extension">,
+  InGroup;
 def note_dependent_member_use : Note<
   "must qualify identifier to find this declaration in dependent base class">;
 def err_not_found_by_two_phase_lookup : Error<"call to function %0 that is neither "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before C++20, MSVC was supporting not mentioning the template argument of the 
base class when initializing a class inheriting a templated base class.
So the following code compiled correctly:

  template 
  class Base {
  };
  
  template 
  class Derived : public Base {
  public:
Derived() : Base() {}
  };
  
  void test() {
  Derived d;
  }

See https://godbolt.org/z/Pxxe7nccx for a conformance view.

This patch adds support for such construct when in MSVC compatibility mode.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp

Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=after,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=after,expected %s
+
+template 
+class Base {
+};
+
+template 
+class Derived : public Base {
+public:
+  // after-error@+1 {{member initializer 'Base' does not name a non-static data member or base class}}
+  Derived() : Base() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template struct AggregateBase {
+  T i;
+};
+
+template
+struct  AggregateDerived : public AggregateBase {
+  int i;
+
+  // after-error@+1 {{member initializer 'AggregateBase' does not name a non-static data member or base class}}
+  AggregateDerived(T j) : AggregateBase{ 4 }, i{ j } {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+  int f() {
+return i + AggregateBase::i; // expected-warning {{use of undeclared identifier 'AggregateBase'; unqualified lookup into dependent bases of class template 'AggregateDerived' is a Microsoft extension}}
+  }
+};
+
+template struct MultiTypesBase {
+};
+
+template
+struct MultiTypesDerived : public MultiTypesBase {
+  // after-error@+1 {{member initializer 'MultiTypesBase' does not name a non-static data member or base class}}
+  MultiTypesDerived() : MultiTypesBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template struct IntegerBase {
+};
+
+template
+struct IntegerDerived : public IntegerBase {
+  // after-error@+1 {{member initializer 'IntegerBase' does not name a non-static data member or base class}}
+  IntegerDerived() : IntegerBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template struct ConformingBase {
+  T i;
+};
+
+template
+struct  ConformingDerived : public ConformingBase {
+  int i;
+
+  ConformingDerived(T j) : ConformingBase{ 4 }, i{ j } {}
+  int f() {
+return i + ConformingBase::i;
+  }
+};
+
+int main() {
+  int I;
+  Derived t;
+
+  AggregateDerived AD{ 2 };
+  AD.AggregateBase::i = 3;
+  I = AD.f();
+
+  MultiTypesDerived MTD;
+
+  IntegerDerived<4> ID;
+
+  ConformingDerived CD{ 2 };
+  I = CD.f();
+
+  return I;
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4345,6 +4345,15 @@
 }
   }
 
+  if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
+auto UnqualifiedBase = R.getAsSingle();
+if (UnqualifiedBase) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+<< SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+}
+  }
+
   if (!TyD && BaseType.isNull()) {
 Diag(IdLoc, diag::err_mem_init_not_member_or_class)
   << MemberOrBase << SourceRange(IdLoc,Init->getSourceRange().getEnd());
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5508,6 +5508,9 @@
 def ext_found_later_in_class : ExtWarn<
   "use of member %0 before its declaration is a Microsoft extension">,
   InGroup;
+def ext_unqualified_base_class : ExtWarn<
+  "unqualified base initializer of class templates is a Microsoft extensi

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

@rnk I don't have the rights to merge. Could you do it when you have the time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

I haven't searched far in the past, but I expect this behavior to go way back, 
yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

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

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a reviewer: shafik.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before C++20, MSVC treated any friend function declaration as a function 
declaration, so the following code would compile despite funGlob being declared 
after its first call:

  class Glob {
  public:
friend void funGlob();
  
void test() {
  funGlob();
}
  };
  
  void funGlob() {}

This proposed patch mimics the MSVC behavior when in MSVC compatibility mode


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124613

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -2658,7 +2658,10 @@
   getTuDecl("struct X { friend void f(); };", Lang_CXX03, "input0.cc");
   auto *FromD = FirstDeclMatcher().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromD->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
+FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   {
 auto FromName = FromD->getDeclName();
 auto *Class = FirstDeclMatcher().match(FromTU, ClassPattern);
@@ -2702,7 +2705,10 @@
   auto *FromNormal =
   LastDeclMatcher().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
+FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   ASSERT_TRUE(FromNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
 
@@ -2793,7 +2799,10 @@
 
   ASSERT_TRUE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromFriendTU->getLangOpts().MSVCCompat,
+FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_TRUE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   auto LookupRes = FromNormalTU->noload_lookup(FromNormalName);
   ASSERT_TRUE(LookupRes.isSingleResult());
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
+#if __cplusplus < 202002L
+// expected-no-diagnostics
+#endif
+
+namespace ns {
+
+class C {
+public:
+  template 
+  friend void funtemp();
+
+  friend void fun();
+
+  void test() {
+::ns::fun(); // modern-error {{no member named 'fun' in namespace 'ns'}}
+
+// modern-error@+3 {{no member named 'funtemp' in namespace 'ns'}}
+// modern-error@+2 {{expected '(' for function-style cast or type construction}}
+// modern-error@+1 {{expected expression}}
+::ns::funtemp();
+  }
+};
+
+void fun() {
+}
+
+template 
+void funtemp() {}
+
+} // namespace ns
+
+class Glob {
+public:
+  friend void funGlob();
+
+  void test() {
+funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
+  }
+};
+
+void funGlob() {
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9631,11 +9631,15 @@
 }
 
 if (isFriend) {
+  // In MSVC mode for older versions of the standard, friend function
+  // declarations behave as declarations
+  bool PerformFriendInjection =
+  getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20;
   if (FunctionTemplate) {
-FunctionTemplate->setObjectOfFriendDecl();
+FunctionTemplate->setObjectOfFriendDecl(PerformFriendInjection);
 FunctionTemplate->setAccess(AS_public);
   }
-  NewFD->setObjectOfFriendDecl();
+  NewFD->setObjectOfFriendDecl(PerformFriendInjection);
   NewFD->setAccess(AS_public);