[PATCH] D58089: Add missing library dependencies in CMakeLists.txt

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Ping.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58089



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

[PATCH] D58089: Add missing library dependencies in CMakeLists.txt

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added reviewers: jkorous, phosek.
Herald added subscribers: cfe-commits, kadircet, arphaman, ioeric, 
ilya-biryukov, mgorny.
Herald added a project: clang.

Fixes build in BUILD_SHARED_LIBS mode.

Removes the "DEPENDS clangdXpcJsonConversions" line as LINK_LIBS already 
implies a dependency AFAIK.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D58089

Files:
  clang-tools-extra/clangd/xpc/CMakeLists.txt


Index: clang-tools-extra/clangd/xpc/CMakeLists.txt
===
--- clang-tools-extra/clangd/xpc/CMakeLists.txt
+++ clang-tools-extra/clangd/xpc/CMakeLists.txt
@@ -20,10 +20,10 @@
 
 add_clang_library(clangdXpcJsonConversions
   Conversion.cpp
+  LINK_LIBS clangDaemon
   )
 
 add_clang_library(clangdXpcTransport
   XPCTransport.cpp
-  DEPENDS clangdXpcJsonConversions
-  LINK_LIBS clangdXpcJsonConversions
+  LINK_LIBS clangDaemon clangdXpcJsonConversions
   )


Index: clang-tools-extra/clangd/xpc/CMakeLists.txt
===
--- clang-tools-extra/clangd/xpc/CMakeLists.txt
+++ clang-tools-extra/clangd/xpc/CMakeLists.txt
@@ -20,10 +20,10 @@
 
 add_clang_library(clangdXpcJsonConversions
   Conversion.cpp
+  LINK_LIBS clangDaemon
   )
 
 add_clang_library(clangdXpcTransport
   XPCTransport.cpp
-  DEPENDS clangdXpcJsonConversions
-  LINK_LIBS clangdXpcJsonConversions
+  LINK_LIBS clangDaemon clangdXpcJsonConversions
   )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58089: Add missing library dependencies in CMakeLists.txt

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Oh, forgot to mention in the initial submission: I don't have commit access so 
I'll need someone else to commit this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58089



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

[PATCH] D66067: Push lambda scope earlier when transforming lambda expression

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added reviewers: rsmith, faisalv, Mordante.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When first parsing a lambda expression, `BuildDeclRefExpr` is called on the 
parameter declarations with the lambda scope already pushed.  But when 
transforming a lambda expression as part of instantiating an outer template, 
`BuildDeclRefExpr` is called again without the scope pushed.  This causes 
`tryCaptureVariable` to get confused when a lambda parameter references an 
earlier parameter, e.g.:

  [](auto c, int x = sizeof(decltype(c))) {}

Fix this by moving up the call to `PushLambdaScope` in 
`TreeTransform::TransformLambdaExpr` to match 
`Parser::ParseLambdaExpressionAfterIntroducer`.

(Note: I do not have commit access.)


Repository:
  rC Clang

https://reviews.llvm.org/D66067

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaTemplate/default-arguments-cxx0x.cpp


Index: clang/test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- clang/test/SemaTemplate/default-arguments-cxx0x.cpp
+++ clang/test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +114,13 @@
 S _a{};
   };
 }
+
+namespace lambda {
+template 
+void bar() {
+  (void) [](auto c, int x = sizeof(decltype(c))) {};
+}
+void foo() {
+  bar();
+}
+} // namespace lambda
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11318,6 +11318,10 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+  LSI->GLTemplateParameterList = TPL;
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
@@ -11348,10 +11352,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class


Index: clang/test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- clang/test/SemaTemplate/default-arguments-cxx0x.cpp
+++ clang/test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +114,13 @@
 S _a{};
   };
 }
+
+namespace lambda {
+template 
+void bar() {
+  (void) [](auto c, int x = sizeof(decltype(c))) {};
+}
+void foo() {
+  bar();
+}
+} // namespace lambda
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11318,6 +11318,10 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+  LSI->GLTemplateParameterList = TPL;
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
@@ -11348,10 +11352,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66067: Push lambda scope earlier when transforming lambda expression

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 214572.
comex added a comment.

Oops, I forgot to re-run `git diff` after fixing the patch.


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

https://reviews.llvm.org/D66067

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaTemplate/default-arguments-cxx0x.cpp


Index: clang/test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- clang/test/SemaTemplate/default-arguments-cxx0x.cpp
+++ clang/test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +114,13 @@
 S _a{};
   };
 }
+
+namespace lambda {
+template 
+void bar() {
+  (void) [](auto c, int x = sizeof(decltype(c))) {};
+}
+void foo() {
+  bar();
+}
+} // namespace lambda
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11318,10 +11318,14 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
   E->getTemplateParameterList());
+  LSI->GLTemplateParameterList = TPL;
 
   // Transform the type of the original lambda's call operator.
   // The transformation MUST be done in the CurrentInstantiationScope since
@@ -11348,10 +11352,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class


Index: clang/test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- clang/test/SemaTemplate/default-arguments-cxx0x.cpp
+++ clang/test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +114,13 @@
 S _a{};
   };
 }
+
+namespace lambda {
+template 
+void bar() {
+  (void) [](auto c, int x = sizeof(decltype(c))) {};
+}
+void foo() {
+  bar();
+}
+} // namespace lambda
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11318,10 +11318,14 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
   E->getTemplateParameterList());
+  LSI->GLTemplateParameterList = TPL;
 
   // Transform the type of the original lambda's call operator.
   // The transformation MUST be done in the CurrentInstantiationScope since
@@ -11348,10 +11352,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66067: Push lambda scope earlier when transforming lambda expression

[Nicholas Allegra via Phabricator via cfe-commits]

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



Comment at: clang/test/SemaTemplate/default-arguments-cxx0x.cpp:1
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics

Mordante wrote:
> Wouldn't it be better to keep the original C++11 test and add a second `RUN:` 
> for the C++14 test?
> Then guard the new test with `#if __cplusplus >= 201402L`
Perhaps, but most tests aren't run multiple times with different -std options, 
and I didn't see any reason this test in particular needed that.  Maybe it 
should just be renamed to default-arguments-cxx14.cpp.


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

https://reviews.llvm.org/D66067



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

[PATCH] D66404: [CFG] Make destructor calls more accurate

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added reviewers: dergachev.a, Szelethus, dcoughlin.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Note: I don't have commit access.

Various changes to reduce discrepancies in destructor calls between the
generated CFG and the actual codegen, along with a new test file.

In particular:

- Respect C++17 copy elision; previously it would generate destructor calls for 
elided temporaries, including in initialization and return statements.

- Don't generate duplicate destructor calls for statement expressions.

- Fix initialization lists.

- Fix comma operator.

- Change printing of implicit destructors to print the type instead of the 
class name directly, matching the code for temporary object destructors.  The 
class name was blank for lambdas.

Also update some existing tests which were broken by the changed formatting
and/or the fixed behavior.

Implementation notes:

- Rename BindToTemporary to ExternallyDestructed, which more accurately 
reflects what the parameter does and matches the naming in CodeGen.

- Change VisitChildrenForTemporaryDtors to take an ExternallyDestructed 
parameter, for the sake of InitListExprs, which have an arbitrary number of 
children and may or may not be externally destructed.

- Add a function VisitExternallyDestructed with the right behavior for return 
statements and statement expressions.

The new test file also includes tests for some preexisting buggy cases which
this patch does *not* fix.  What a mess...


Repository:
  rC Clang

https://reviews.llvm.org/D66404

Files:
  lib/Analysis/CFG.cpp
  test/Analysis/cfg-rich-constructors.cpp
  test/Analysis/cfg-rich-constructors.mm
  test/Analysis/cfg.cpp
  test/Analysis/missing-bind-temporary.cpp
  test/Analysis/more-dtors-cfg-output.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -830,12 +830,7 @@
   // On each branch the variable is constructed directly.
   if (coin) {
 clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
-#if __cplusplus < 201703L
 clang_analyzer_eval(y == 1); // expected-warning{{TRUE}}
-#else
-// FIXME: Destructor called twice in C++17?
-clang_analyzer_eval(y == 2); // expected-warning{{TRUE}}
-#endif
 clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
 clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
 
@@ -843,12 +838,7 @@
 clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
 clang_analyzer_eval(z == 1); // expected-warning{{TRUE}}
-#if __cplusplus < 201703L
 clang_analyzer_eval(w == 1); // expected-warning{{TRUE}}
-#else
-// FIXME: Destructor called twice in C++17?
-clang_analyzer_eval(w == 2); // expected-warning{{TRUE}}
-#endif
   }
 }
 } // namespace test_match_constructors_and_destructors
@@ -1055,16 +1045,11 @@
 #endif
 
   bar2(S(2));
-  // FIXME: Why are we losing information in C++17?
   clang_analyzer_eval(glob == 2);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-  // expected-warning@-3{{TRUE}}
-#else
-  // expected-warning@-5{{UNKNOWN}}
-#endif
+  // expected-warning@-2{{TRUE}}
 #else
-  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-4{{UNKNOWN}}
 #endif
 
   C *c = new D();
Index: test/Analysis/more-dtors-cfg-output.cpp
===
--- /dev/null
+++ test/Analysis/more-dtors-cfg-output.cpp
@@ -0,0 +1,311 @@
+// RUN: rm -f %t.14 %t.17
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++14 -DCXX2A=0 -fblocks -Wall -Wno-unused -Werror %s > %t.14 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++2a -DCXX2A=1 -fblocks -Wall -Wno-unused -Werror %s > %t.17 2>&1
+// RUN: FileCheck --input-file=%t.14 -check-prefixes=CHECK,CXX14 -implicit-check-not=destructor %s
+// RUN: FileCheck --input-file=%t.17 -check-prefixes=CHECK,CXX2A -implicit-check-not=destructor %s
+
+int puts(const char *);
+
+struct Foo {
+  Foo() = delete;
+#if CXX2A
+  // Guarantee that the elided examples are actually elided by deleting the
+  // copy constructor.
+  Foo(const Foo &) = delete;
+#else
+  // No elision support, so we need a copy constructor.
+  Foo(const Foo &);
+#endif
+  ~Foo();
+};
+
+struct TwoFoos {
+  Foo foo1, foo2;
+  ~TwoFoos();
+};
+
+Foo get_foo();
+
+struct Bar {
+  Bar();
+  Bar(const Bar &);
+  ~Bar();
+  Bar &operator=(const Bar &);
+};
+
+Bar get_bar();
+
+struct TwoBars {
+  Bar foo1, foo2;
+  ~TwoBars();
+};
+
+// Start of tests:
+
+void elided_assign() {
+  Foo x = get_foo();
+}
+// CHECK: void elided_assign()
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)
+
+void nonelided_assign() {
+  Bar x = (const Bar &)get_bar();
+}
+// CHECK: void nonelided_assign()

[PATCH] D65694: Properly instantiate a decltype in argument's default initializer

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Ping.  I'm not qualified to review this myself but I'd like to see the bug 
fixed. :)


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

https://reviews.llvm.org/D65694



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

[PATCH] D66067: Push lambda scope earlier when transforming lambda expression

[Nicholas Allegra via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372058: Push lambda scope earlier when transforming lambda 
expression (authored by comex, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66067?vs=217230&id=220419#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66067

Files:
  cfe/trunk/lib/Sema/TreeTransform.h
  cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp


Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -11325,10 +11325,14 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
   E->getTemplateParameterList());
+  LSI->GLTemplateParameterList = TPL;
 
   // Transform the type of the original lambda's call operator.
   // The transformation MUST be done in the CurrentInstantiationScope since
@@ -11355,10 +11359,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class
Index: cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp
+++ cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +115,17 @@
 S _a{};
   };
 }
+
+#if __cplusplus >= 201402L
+namespace lambda {
+  // Verify that a default argument in a lambda can refer to the type of a
+  // previous `auto` argument without crashing.
+  template 
+  void bar() {
+(void) [](auto c, int x = sizeof(decltype(c))) {};
+  }
+  void foo() {
+bar();
+  }
+} // namespace lambda
+#endif


Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -11325,10 +11325,14 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
   E->getTemplateParameterList());
+  LSI->GLTemplateParameterList = TPL;
 
   // Transform the type of the original lambda's call operator.
   // The transformation MUST be done in the CurrentInstantiationScope since
@@ -11355,10 +11359,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class
Index: cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp
+++ cfe/trunk/test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +115,17 @@
 S _a{};
   };
 }
+
+#if __cplusplus >= 201402L
+namespace lambda {
+  // Verify that a default argument in a lambda can refer to the type of a
+  // previous `auto` argument without crashing.
+  template 
+  void bar() {
+(void) [](auto c, int x = sizeof(decltype(c))) {};
+  }
+  void foo() {
+bar();
+  }
+} // namespace lambda
+#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66830: Consumed checker: various improvements

[Nicholas Allegra via Phabricator via cfe-commits]

comex abandoned this revision.
comex added a comment.
Herald added a subscriber: dmgreen.

Sounds good; I'll split this into a few separate patches.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66830



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added subscribers: cfe-commits, dmgreen, kristof.beyls.
Herald added a project: clang.

Currently, `handleCall` takes a `CallExpr` as an argument and retrieves the 
arguments from there.  This changes it to take the argument list as a separate 
argument of type `ArrayRef`.

The main motivation that I want `VisitCXXConstructExpr` to also be able to 
invoke `handleCall`, even though `CXXConstructExpr` is not a subclass of 
`CallExpr`.  But actually adding that will wait for a future patch.

However, this also fixes a minor bug in `VisitCXXOperatorCallExpr`:

  if (const auto *MCall = dyn_cast(Call))
handleCall(MCall, MCall->getImplicitObjectArgument(), FunDecl);
  else
handleCall(Call, Call->getArg(0), FunDecl);

`Call` here is a `CXXOperatorCallExpr`, and `CXXMemberCallExpr` is a sibling 
class, so the cast will never succeed.  CXXMemberCallExprs go through a 
separate visitor, `VisitCXXMemberCallExpr`.

On the other hand, this logic may be intended to reflect the fact that C++ 
operators can be declared as either methods or free functions.  The correct way 
to differentiate these is shown at the beginning of handleCall:

  unsigned Offset = 0;
  if (isa(Call) && isa(FunD))
Offset = 1;  // first argument is 'this'

For `CXXOperatorCallExpr`s, if the decl is a `CXXMethodDecl`, the first 
argument is `this`; otherwise there is no `this`.

Going back to the other first: currently, since the `dyn_cast` always fails, it 
always passes `Call->getArg(0)` as `ObjArg` (i.e. the expression representing 
`this`), even for operators declared as free functions.  However, this is 
harmless, because `ObjArg` is only used if the function is marked as one of 
`set_typestate` or `test_typestate`, or `callable_when`, yet those attributes 
are only allowed on methods.  `Call->getArg(0)` will crash if there are zero 
arguments, but the only kind of non-method operator function allowed to have 
zero arguments is a user-defined literal, and those do not produce 
`CXXOperatorCallExpr`s.

The bug could be fixed by changing the first snippet to check for 
`CXXMethodDecl` like the second one, but this refactor makes things cleaner by 
only having to check in one place.


Repository:
  rC Clang

https://reviews.llvm.org/D67647

Files:
  lib/Analysis/Consumed.cpp

Index: lib/Analysis/Consumed.cpp
===
--- lib/Analysis/Consumed.cpp
+++ lib/Analysis/Consumed.cpp
@@ -494,8 +494,8 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArrayRef Args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +608,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArrayRef Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argument lists.
-if (Index - Offset >= FunD->getNumParams())
+if (Index >= FunD->getNumParams())
   break;
 
-const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+const ParmVarDecl *Param = FunD->getParamDecl(Index++);
 QualType ParamType = Param->getType();
 
-InfoEntry Entry = findInfo(Call->getArg(Index));
+InfoEntry Entry = findInfo(Arg);
 
 if (Entry == PropagationMap.end() || Entry->second.isTest())
   continue;
@@ -636,7 +635,7 @@
 
   if (ParamState != ExpectedState)
 Analyzer.WarningsHandler.warnParamTypestateMismatch(
-  Call->getArg(Index)->getExprLoc(),
+  Arg->getExprLoc(),
   stateToString(ExpectedState), stateToString(ParamState));
 }
 
@@ -749,7 +748,9 @@
 return;
   }
 
-  handleCall(Call, nullptr, FunDecl);
+  handleCall(Call, nullptr,
+ llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()),
+ FunDecl);
   propagateReturnType(Call, FunDecl);
 }
 
@@ -805,7 +806,8 @@
   if (!MD)
 return;
 
-  handleCall(Call, Call->getImplicitObjectArgument(), MD);
+  handleCall(Call, Call->getImplicitObjectA

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Ugh, it looks like `getArgs()` is a massive strict aliasing violation.  And 
it's used in enough different places in Clang that fixing the violation may 
require extensive refactoring... I've reported the issue as 
https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it 
:)

However, I can at least update this patch to avoid using getArgs().


Repository:
  rC Clang

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

https://reviews.llvm.org/D67647



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

[PATCH] D67740: [Consumed] Refactor and improve diagnostics

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As suggested by FIXME comments, fix commented-out diagnostic in Sema and remove 
the equivalent check within the consumed analysis.

The diagnostic in question is the warning for using `param_typestate` and 
`return_typestate` with a type that is not consumable.

There were several FIXME comments about the same issue; the most detailed was 
before the commented-out check:

  // FIXME: This check is currently being done in the analysis.  It can be
  //enabled here only after the parser propagates attributes at
  //template specialization definition, not declaration.

I was confused what this meant.  After investigating, I think it actually 
refers to the fact that attributes are parsed only once for a template, and are 
not re-parsed when the template is instantiated.  If I'm right, the issue 
actually has nothing to do with template specializations or with definitions 
versus declarations.  I could be missing something, though, so please let me 
know if there's a case I'm not thinking of.  I did add some template 
specializations as test cases (for both class and function templates).

This patch addresses the issue by moving the diagnostic to a function which is 
called from both parsing and template instantiation, similar to what's already 
done with some other attributes.

The analysis version of the check didn't always work, and only applied to 
`return_typestate` rather than `param_typestate` (even though both had 
commented-out Sema checks); therefore, the fixed code may produce warnings that 
didn't appear before.

Also, add a new diagnostic for when the `set_typestate` or `test_typestate` 
attribute is applied to a constructor or static method; previously Clang would 
ignore it or crash, respectively.


Repository:
  rC Clang

https://reviews.llvm.org/D67740

Files:
  include/clang/Analysis/Analyses/Consumed.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Analysis/Consumed.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/SemaCXX/warn-consumed-parsing.cpp

Index: test/SemaCXX/warn-consumed-parsing.cpp
===
--- test/SemaCXX/warn-consumed-parsing.cpp
+++ test/SemaCXX/warn-consumed-parsing.cpp
@@ -6,16 +6,6 @@
 #define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state)))
 #define TEST_TYPESTATE(state)   __attribute__ ((test_typestate(state)))
 
-// FIXME: This test is here because the warning is issued by the Consumed
-//analysis, not SemaDeclAttr.  The analysis won't run after an error
-//has been issued.  Once the attribute propagation for template
-//instantiation has been fixed, this can be moved somewhere else and the
-//definition can be removed.
-int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{return state set for an unconsumable type 'int'}}
-int returnTypestateForUnconsumable() {
-  return 0;
-}
-
 class AttrTester0 {
   void consumes()   __attribute__ ((set_typestate())); // expected-error {{'set_typestate' attribute takes one argument}}
   bool testUnconsumed() __attribute__ ((test_typestate())); // expected-error {{'test_typestate' attribute takes one argument}}
@@ -62,5 +52,37 @@
   Status {
 };
 
+int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+int returnTypestateForUnconsumable() {
+  return 0;
+}
+
+struct CONSUMABLE(unknown) UselessAttrs {
+  static void x() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  UselessAttrs() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+  UselessAttrs(int) CALLABLE_WHEN(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+  void operator+(UselessAttrs) SET_TYPESTATE(consumed); // OK
+  static void *operator new(unsigned long) SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  template 
+  void a([[clang::param_typestate(consumed)]] int) {} // expected-warning {{attribute 'param_typestate' invalid for parameter of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+  void b([[clang::return_typestate(consumed)]] UselessAttrs *) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'UselessAttrs *': expected a consumable class type as a value, reference, or rvalue reference}}
+  template 
+  voi

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 220771.
comex added a comment.
Herald added subscribers: llvm-commits, dexonsmith.
Herald added a project: LLVM.

Here's a new version of the patch that uses iterator ranges instead of 
`ArrayRef`, to avoid adding new uses of `CallExpr::getArgs()` in case it has to 
be removed in the future due to the strict aliasing issue.

To support this, the following additional changes are included:

- Fix `CastIterator`'s use of `iterator_adaptor_base` so that `operator[]` 
isn't broken.

- Add an `operator[]` implementation to `iterator_range`.

- Improve the existing `llvm::drop_begin` helper to assert that it doesn't go 
out of bounds.  (The implementation would be a lot less ugly if we could use 
C++17 features; oh well.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647

Files:
  clang/include/clang/AST/Stmt.h
  clang/lib/Analysis/Consumed.cpp
  llvm/include/llvm/ADT/iterator_range.h

Index: llvm/include/llvm/ADT/iterator_range.h
===
--- llvm/include/llvm/ADT/iterator_range.h
+++ llvm/include/llvm/ADT/iterator_range.h
@@ -20,9 +20,17 @@
 
 #include 
 #include 
+#include 
 
 namespace llvm {
 
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
@@ -44,6 +52,15 @@
 
   IteratorT begin() const { return begin_iterator; }
   IteratorT end() const { return end_iterator; }
+
+  template 
+  decltype(std::declval<_IteratorT>()[0]) operator[](
+  typename std::iterator_traits<_IteratorT>::difference_type N) const {
+static_assert(is_random_iterator(),
+  "Must be a random access iterator to use []");
+assert(N < end_iterator - begin_iterator && "Index out of range!");
+return begin_iterator[N];
+  }
 };
 
 /// Convenience function for iterating over sub-ranges.
@@ -58,11 +75,31 @@
   return iterator_range(std::move(p.first), std::move(p.second));
 }
 
+/// Non-random-iterator version
 template 
-iterator_range()))> drop_begin(T &&t,
-  int n) {
-  return make_range(std::next(adl_begin(t), n), adl_end(t));
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  for (int i = 0; i < n; i++) {
+assert(begin != end);
+++begin;
+  }
+  return make_range(begin, end);
 }
+
+/// Optimized version for random iterators
+template 
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  assert(end - begin >= n && "Dropping more elements than exist!");
+  return make_range(std::next(begin, n), end);
+}
+
 }
 
 #endif
Index: clang/lib/Analysis/Consumed.cpp
===
--- clang/lib/Analysis/Consumed.cpp
+++ clang/lib/Analysis/Consumed.cpp
@@ -494,8 +494,10 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+
+  using ArgRange = llvm::iterator_range;
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArgRange args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +610,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArgRange Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argument lists.
-if (Index - Offset >= FunD->getNumParams())
+if (Index >= FunD->getNumParams())
   break;
 
-const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+const ParmVarDecl *Param = FunD->getParamDecl(Index++);
 QualType ParamType = Param->getType();
 
-InfoEntry Entry = findInfo(Call->getArg(Index));
+InfoEntry Entry = fi

[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Also, fix the order of `if` statements so that an explicit `return_typestate` 
annotation takes precedence over the default behavior for rvalue refs.

Note that for by-value arguments, the object being consumed is always a 
temporary.  If you write:

`void a(X); a((X &&)x);`

...this first constructs a temporary X by moving from `x`, then calls `a` with 
a pointer to the temporary.  Before and after this change, the move copies 
`x`'s typestate to the temporary and marks `x` itself as consumed.  But before 
this change, if the temporary started out unconsumed (because `x` was 
unconsumed before the move), it would still be unconsumed when its destructor 
was run after the call.  Now it will be considered consumed at that point.

Also note that currently, only parameters explicitly marked `return_typestate` 
have their state-on-return checked on the callee side.  Parameters which are 
implicitly consuming due to being rvalue references – or, after this patch, 
by-value – are checked only on the caller side.  This discrepancy was very 
surprising to me as a user.  I wrote a fix, but in my codebase it broke some 
code that was using `std::forward`.  Therefore, I plan to hold off on 
submitting the fix until a followup patch, which will generalize the current 
`std::move` special case into an attribute that can be applied to any 'cast' 
function like `std::move` and `std::forward`.


Repository:
  rC Clang

https://reviews.llvm.org/D67743

Files:
  lib/Analysis/Consumed.cpp
  test/SemaCXX/warn-consumed-analysis.cpp


Index: test/SemaCXX/warn-consumed-analysis.cpp
===
--- test/SemaCXX/warn-consumed-analysis.cpp
+++ test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,10 +53,15 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
+  static void byVal(DestructorTester);
+  static void byValMarkUnconsumed(DestructorTester 
RETURN_TYPESTATE(unconsumed));
 };
 
 void baf0(const ConsumableClass  var);
@@ -120,6 +125,19 @@
  expected-warning {{invalid invocation of method 
'~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
 }
 
+void testDestructionByVal() {
+  {
+// both the var and the temporary are consumed:
+DestructorTester D0(nullptr);
+DestructorTester::byVal((DestructorTester &&)D0);
+  }
+  {
+// the var is consumed but the temporary isn't:
+DestructorTester D1(nullptr);
+DestructorTester::byValMarkUnconsumed((DestructorTester &&)D1); // 
expected-warning {{invalid invocation of method '~DestructorTester' on a 
temporary object while it is in the 'unconsumed' state}}
+  }
+}
+
 void testTempValue() {
   *ConsumableClass(); // expected-warning {{invalid invocation of method 
'operator*' on a temporary object while it is in the 'consumed' state}}
 }
@@ -413,10 +431,15 @@
   Param.consume();
 }
 
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass &&Param 
RETURN_TYPESTATE(unconsumed)) {
+  Param.unconsume();
+}
+
 void testParamReturnTypestateCaller() {
   ConsumableClass var;
   
   testParamReturnTypestateCallee(true, var);
+  testRvalueRefParamReturnTypestateCallee((ConsumableClass &&)var);
   
   *var;
 }
@@ -480,6 +503,9 @@
   
   baf2(&var);  
   *var;
+
+  baf3(var);
+  *var;
   
   baf4(var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on 
object 'var' while it is in the 'unknown' state}}
Index: lib/Analysis/Consumed.cpp
===
--- lib/Analysis/Consumed.cpp
+++ lib/Analysis/Consumed.cpp
@@ -645,10 +645,10 @@
   continue;
 
 // Adjust state on the caller side.
-if (isRValueRef(ParamType))
-  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-else if (ReturnTypestateAttr *RT = Param->getAttr())
+if (ReturnTypestateAttr *RT = Param->getAttr())
   setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
 else if (isPointerOrRef(ParamType) &&
  (!ParamType->getPointeeType().isConstQualified() ||
   isSetOnReadPtrType(ParamType)))


Index: test/SemaCXX/warn-consumed-analysis.cpp
===
--- test/SemaCXX/warn-consumed-analysis.cpp
+++ test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,10 +53,15 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*()

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

In D67647#1674773 , @dblaikie wrote:

> I wasn't expecting it to involve all this work - but instead to change 
> "arguments()" to return ArrayRef. Though perhaps you didn't go that route to 
> allow future fixes to the strict aliasing (by having an adapting iterator - 
> or perhaps arguments() already uses such an iterator that doesn't hit the 
> strict aliasing issue?) needing to do even more cleaunp work?


Yeah, `arguments()` returns an adapting iterator.

>> - Add an `operator[]` implementation to `iterator_range`.
> 
> I'm not sure that's appropriate - not all types with begin() and end() (even 
> those with random access iterators) would have op[], so it doesn't seem 
> appropriate to add it/depend on it?

Random access iterators are supposed to have `operator[]`, according to:
http://www.cplusplus.com/reference/iterator/RandomAccessIterator/

But the use of a template ensures that it doesn't cause an error with types 
that don't have `operator[]`, whether they are marked as random-access or not.

I checked the spec for "real" C++20 ranges... it seems that random access 
ranges don't always have `operator[]`, but "view_interface" (which is a type of 
range) does:
https://eel.is/c++draft/view.interface

Anyway, I don't think it hurts to have it on this little imitation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67740: [Consumed] Refactor and improve diagnostics

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 220784.
comex added a comment.

Fixed a mistake where the diagnostic would print the wrong parameter type.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67740

Files:
  include/clang/Analysis/Analyses/Consumed.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Analysis/Consumed.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/SemaCXX/warn-consumed-parsing.cpp

Index: test/SemaCXX/warn-consumed-parsing.cpp
===
--- test/SemaCXX/warn-consumed-parsing.cpp
+++ test/SemaCXX/warn-consumed-parsing.cpp
@@ -6,16 +6,6 @@
 #define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state)))
 #define TEST_TYPESTATE(state)   __attribute__ ((test_typestate(state)))
 
-// FIXME: This test is here because the warning is issued by the Consumed
-//analysis, not SemaDeclAttr.  The analysis won't run after an error
-//has been issued.  Once the attribute propagation for template
-//instantiation has been fixed, this can be moved somewhere else and the
-//definition can be removed.
-int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{return state set for an unconsumable type 'int'}}
-int returnTypestateForUnconsumable() {
-  return 0;
-}
-
 class AttrTester0 {
   void consumes()   __attribute__ ((set_typestate())); // expected-error {{'set_typestate' attribute takes one argument}}
   bool testUnconsumed() __attribute__ ((test_typestate())); // expected-error {{'test_typestate' attribute takes one argument}}
@@ -62,5 +52,37 @@
   Status {
 };
 
+int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+int returnTypestateForUnconsumable() {
+  return 0;
+}
+
+struct CONSUMABLE(unknown) UselessAttrs {
+  static void x() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  UselessAttrs() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+  UselessAttrs(int) CALLABLE_WHEN(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+  void operator+(UselessAttrs) SET_TYPESTATE(consumed); // OK
+  static void *operator new(unsigned long) SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  template 
+  void a([[clang::param_typestate(consumed)]] const int &) {} // expected-warning {{attribute 'param_typestate' invalid for parameter of type 'const int &': expected a consumable class type as a value, reference, or rvalue reference}}
+  void b([[clang::return_typestate(consumed)]] UselessAttrs *) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'UselessAttrs *': expected a consumable class type as a value, reference, or rvalue reference}}
+  template 
+  void c([[clang::return_typestate(consumed)]] T) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+  template <> // test a template specialization
+  void c([[clang::return_typestate(consumed)]] long); // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'long': expected a consumable class type as a value, reference, or rvalue reference}}
+  void instantiate() {
+a(42);
+c(42);
+  }
+};
+void operator-(UselessAttrs, UselessAttrs) SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' attribute only applies to functions}}
 
+template 
+struct CONSUMABLE(unknown) ClassTemplateSpecialization;
+template 
+struct ClassTemplateSpecialization {
+  [[clang::return_typestate(consumed)]] ClassTemplateSpecialization(); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'ClassTemplateSpecialization': expected a consumable class type as a value, reference, or rvalue reference}}
+  void a([[clang::return_typestate(consumed)]] T) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'long': expected a consumable class type as a value, reference, or rvalue reference}}
+};
+void testTS() { ClassTemplateSpecialization().a(1); } // expected-note {{in instantiation of template class 'ClassTemplateSpecialization' requested here}}
 
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -

[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

In D67743#1675533 , @dblaikie wrote:

> "Also, fix the order of if statements so that an explicit return_typestate 
> annotation takes precedence over the default behavior for rvalue refs."
>
> I'd probably have split that out into a separate patch - in part to discuss 
> the design implications of that. I have some doubts about supporting 
> non-consumed after passing by rvalue ref, but don't feel too strongly & the 
> alternative would be having a warning about the attribute being ignored, etc 
> - when it has a fairly clear meaning if it is there, just not sure people 
> 'should' be doing that.


Fair enough.  I agree there's not much reason to do that, but it also seems 
pretty harmless to respect the user's choice.  Silently ignoring the attribute 
as before is obviously wrong, so the alternative would be adding a diagnostic 
for that case, but it doesn't seem worth it...

> unless these are testing something noteworthy about them being static 
> functions I'd probably suggest making them non-members like the other test 
> functions below, for consistency (otherwise the inconsistency tends to raise 
> the question of "what is significant about this difference?")

Okay, I'll change them to non-members before committing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67743



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

[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372361: [Consumed] Treat by-value class arguments as 
consuming by default, like rvalue… (authored by comex, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67743?vs=220778&id=220916#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67743

Files:
  cfe/trunk/lib/Analysis/Consumed.cpp
  cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp


Index: cfe/trunk/lib/Analysis/Consumed.cpp
===
--- cfe/trunk/lib/Analysis/Consumed.cpp
+++ cfe/trunk/lib/Analysis/Consumed.cpp
@@ -644,10 +644,10 @@
   continue;
 
 // Adjust state on the caller side.
-if (isRValueRef(ParamType))
-  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-else if (ReturnTypestateAttr *RT = Param->getAttr())
+if (ReturnTypestateAttr *RT = Param->getAttr())
   setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
 else if (isPointerOrRef(ParamType) &&
  (!ParamType->getPointeeType().isConstQualified() ||
   isSetOnReadPtrType(ParamType)))
Index: cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,12 +53,18 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
 };
 
+void dtByVal(DestructorTester);
+void dtByValMarkUnconsumed(DestructorTester RETURN_TYPESTATE(unconsumed));
+
 void baf0(const ConsumableClass  var);
 void baf1(const ConsumableClass &var);
 void baf2(const ConsumableClass *var);
@@ -120,6 +126,19 @@
  expected-warning {{invalid invocation of method 
'~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
 }
 
+void testDestructionByVal() {
+  {
+// both the var and the temporary are consumed:
+DestructorTester D0(nullptr);
+dtByVal((DestructorTester &&)D0);
+  }
+  {
+// the var is consumed but the temporary isn't:
+DestructorTester D1(nullptr);
+dtByValMarkUnconsumed((DestructorTester &&)D1); // expected-warning 
{{invalid invocation of method '~DestructorTester' on a temporary object while 
it is in the 'unconsumed' state}}
+  }
+}
+
 void testTempValue() {
   *ConsumableClass(); // expected-warning {{invalid invocation of method 
'operator*' on a temporary object while it is in the 'consumed' state}}
 }
@@ -413,10 +432,15 @@
   Param.consume();
 }
 
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass &&Param 
RETURN_TYPESTATE(unconsumed)) {
+  Param.unconsume();
+}
+
 void testParamReturnTypestateCaller() {
   ConsumableClass var;
   
   testParamReturnTypestateCallee(true, var);
+  testRvalueRefParamReturnTypestateCallee((ConsumableClass &&)var);
   
   *var;
 }
@@ -480,6 +504,9 @@
   
   baf2(&var);  
   *var;
+
+  baf3(var);
+  *var;
   
   baf4(var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on 
object 'var' while it is in the 'unknown' state}}


Index: cfe/trunk/lib/Analysis/Consumed.cpp
===
--- cfe/trunk/lib/Analysis/Consumed.cpp
+++ cfe/trunk/lib/Analysis/Consumed.cpp
@@ -644,10 +644,10 @@
   continue;
 
 // Adjust state on the caller side.
-if (isRValueRef(ParamType))
-  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-else if (ReturnTypestateAttr *RT = Param->getAttr())
+if (ReturnTypestateAttr *RT = Param->getAttr())
   setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
 else if (isPointerOrRef(ParamType) &&
  (!ParamType->getPointeeType().isConstQualified() ||
   isSetOnReadPtrType(ParamType)))
Index: cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,12 +53,18 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
 };
 
+void 

[PATCH] D67740: [Consumed] Refactor and improve diagnostics

[Nicholas Allegra via Phabricator via cfe-commits]

comex marked 4 inline comments as done.
comex added a comment.

In D67740#1675564 , @dblaikie wrote:

> Looks like this might benefit from being split into independent changes - the 
> work related to templates (I haven't looked closely, but I assume that's 
> fairly indivisible) and the work related to other diagnostics seems fairly 
> separable - and maybe there's other pieces too.


Okay, I'll take the set_typestate/test_typestate part out of this and submit it 
separately.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3234
+  "consumed analysis attribute is attached to a "
+  "%select{static method|constructor}0 of class '%1'">,
+  InGroup, DefaultIgnore;

dblaikie wrote:
> "member function" would be more correct than "method" here (the diagnostics 
> in this file using the word "method" are mostly for Objective C things)
> 
> Are there other diagnostics that are similar? (like maybe function "const" - 
> which can't be on non-member, static member, or ctors - wouldn't that be the 
> same here? Where's the failure path for a non-member (free) function? Could 
> it be unified with the static member/ctor case you're adding?)
> 
> Ah, looks like in Attr.td these attributes could be flagged 
> "NonStaticCXXMethod" rather than "CXXMethod" - perhaps that narrower 
> classification wasn't available when this was originally implemented. (then, 
> I think, the attribute parsing logic will do the job of warning about the 
> misuse and ignoring the attribute entirely)
That doesn't check for constructors, but sure, I'll add a 
"NonStaticNonConstructorCXXMethod" instead (in the separate submission).




Comment at: lib/Sema/SemaDeclAttr.cpp:1237-1240
+else if (auto *CX = dyn_cast(X))
+  return CX->getThisType()->getPointeeType();
+else
+  return cast(X)->getCallResultType();

dblaikie wrote:
> Drop the else-after-return 
> https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
Done.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67740



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

[PATCH] D67740: [Consumed] Refactor and improve diagnostics

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 220918.
comex marked 2 inline comments as done.
comex edited the summary of this revision.
comex added a comment.

Addressed feedback.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67740

Files:
  include/clang/Analysis/Analyses/Consumed.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Analysis/Consumed.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/SemaCXX/warn-consumed-parsing.cpp

Index: test/SemaCXX/warn-consumed-parsing.cpp
===
--- test/SemaCXX/warn-consumed-parsing.cpp
+++ test/SemaCXX/warn-consumed-parsing.cpp
@@ -6,16 +6,6 @@
 #define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state)))
 #define TEST_TYPESTATE(state)   __attribute__ ((test_typestate(state)))
 
-// FIXME: This test is here because the warning is issued by the Consumed
-//analysis, not SemaDeclAttr.  The analysis won't run after an error
-//has been issued.  Once the attribute propagation for template
-//instantiation has been fixed, this can be moved somewhere else and the
-//definition can be removed.
-int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{return state set for an unconsumable type 'int'}}
-int returnTypestateForUnconsumable() {
-  return 0;
-}
-
 class AttrTester0 {
   void consumes()   __attribute__ ((set_typestate())); // expected-error {{'set_typestate' attribute takes one argument}}
   bool testUnconsumed() __attribute__ ((test_typestate())); // expected-error {{'test_typestate' attribute takes one argument}}
@@ -62,5 +52,33 @@
   Status {
 };
 
+int returnTypestateForUnconsumable() RETURN_TYPESTATE(consumed); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+int returnTypestateForUnconsumable() {
+  return 0;
+}
+
+struct CONSUMABLE(unknown) UselessAttrs {
+  void operator+(UselessAttrs) SET_TYPESTATE(consumed); // OK
+  template 
+  void a([[clang::param_typestate(consumed)]] const int &) {} // expected-warning {{attribute 'param_typestate' invalid for parameter of type 'const int &': expected a consumable class type as a value, reference, or rvalue reference}}
+  void b([[clang::return_typestate(consumed)]] UselessAttrs *) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'UselessAttrs *': expected a consumable class type as a value, reference, or rvalue reference}}
+  template 
+  void c([[clang::return_typestate(consumed)]] T) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'int': expected a consumable class type as a value, reference, or rvalue reference}}
+  template <> // test a template specialization
+  void c([[clang::return_typestate(consumed)]] long); // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'long': expected a consumable class type as a value, reference, or rvalue reference}}
+  void instantiate() {
+a(42);
+c(42);
+  }
+};
+void operator-(UselessAttrs, UselessAttrs) SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' attribute only applies to functions}}
 
+template 
+struct CONSUMABLE(unknown) ClassTemplateSpecialization;
+template 
+struct ClassTemplateSpecialization {
+  [[clang::return_typestate(consumed)]] ClassTemplateSpecialization(); // expected-warning {{attribute 'return_typestate' invalid for return value of type 'ClassTemplateSpecialization': expected a consumable class type as a value, reference, or rvalue reference}}
+  void a([[clang::return_typestate(consumed)]] T) {} // expected-warning {{attribute 'return_typestate' invalid for parameter of type 'long': expected a consumable class type as a value, reference, or rvalue reference}}
+};
+void testTS() { ClassTemplateSpecialization().a(1); } // expected-note {{in instantiation of template class 'ClassTemplateSpecialization' requested here}}
 
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -606,6 +606,11 @@
   continue;
 }
 
+if (isa(TmplAttr) || isa(TmplAttr)) {
+  if (!CheckParamOrReturnTypestateAttr(TmplAttr, New, Tmpl))
+continue; // don't add
+}
+
 assert(!TmplAttr->isPackExpansion());
 if (TmplAttr->isLateParsed() && LateAttrs) {
   // Late parsed attributes must be instantiated and attached after the
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -1190,19 +1190,9 @@
 return;
   }
 
-  // FIXME: This check is curr

[PATCH] D67778: [Consumed] Narrow Subject for some attributes

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is the former second part of https://reviews.llvm.org/D67740 that was 
split out (and rewritten).

Error out when the `callable_when`, `set_typestate` and `test_typestate` 
attributes are applied to a constructor or static method; previously Clang 
would ignore them or crash, depending on the attribute.


Repository:
  rC Clang

https://reviews.llvm.org/D67778

Files:
  include/clang/Basic/Attr.td
  test/SemaCXX/warn-consumed-parsing.cpp


Index: test/SemaCXX/warn-consumed-parsing.cpp
===
--- test/SemaCXX/warn-consumed-parsing.cpp
+++ test/SemaCXX/warn-consumed-parsing.cpp
@@ -12,7 +12,7 @@
   void callableWhen()   __attribute__ ((callable_when())); // expected-error 
{{'callable_when' attribute takes at least 1 argument}}
 };
 
-int var0 SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' 
attribute only applies to functions}}
+int var0 SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' 
attribute only applies to non-static non-constructor member functions}}
 int var1 TEST_TYPESTATE(consumed); // expected-warning {{'test_typestate' 
attribute only applies to}}
 int var2 CALLABLE_WHEN("consumed"); // expected-warning {{'callable_when' 
attribute only applies to}}
 int var3 CONSUMABLE(consumed); // expected-warning {{'consumable' attribute 
only applies to classes}}
@@ -58,7 +58,11 @@
 }
 
 struct CONSUMABLE(unknown) UselessAttrs {
+  static void x() SET_TYPESTATE(consumed); // expected-warning 
{{'set_typestate' attribute only applies to non-static non-constructor member 
functions}}
+  UselessAttrs() SET_TYPESTATE(consumed); // expected-warning 
{{'set_typestate' attribute only applies to non-static non-constructor member 
functions}}
+  UselessAttrs(int) CALLABLE_WHEN(consumed); // expected-warning 
{{'callable_when' attribute only applies to non-static non-constructor member 
functions}}
   void operator+(UselessAttrs) SET_TYPESTATE(consumed); // OK
+  static void *operator new(unsigned long) SET_TYPESTATE(consumed); // 
expected-warning {{'set_typestate' attribute only applies to non-static 
non-constructor member functions}}
   template 
   void a([[clang::param_typestate(consumed)]] const int &) {} // 
expected-warning {{attribute 'param_typestate' invalid for parameter of type 
'const int &': expected a consumable class type as a value, reference, or 
rvalue reference}}
   void b([[clang::return_typestate(consumed)]] UselessAttrs *) {} // 
expected-warning {{attribute 'return_typestate' invalid for parameter of type 
'UselessAttrs *': expected a consumable class type as a value, reference, or 
rvalue reference}}
@@ -71,7 +75,7 @@
 c(42);
   }
 };
-void operator-(UselessAttrs, UselessAttrs) SET_TYPESTATE(consumed); // 
expected-warning {{'set_typestate' attribute only applies to functions}}
+void operator-(UselessAttrs, UselessAttrs) SET_TYPESTATE(consumed); // 
expected-warning {{'set_typestate' attribute only applies to non-static 
non-constructor member functions}}
 
 template 
 struct CONSUMABLE(unknown) ClassTemplateSpecialization;
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -96,6 +96,11 @@
[{!S->isStatic()}],
"non-static member functions">;
 
+def NonStaticNonConstructorCXXMethod
+: SubsetSubjectisStatic() && !isa(S)}],
+"non-static non-constructor member functions">;
+
 def NonStaticNonConstCXXMethod
 : SubsetSubjectisStatic() && !S->isConst()}],
@@ -2725,7 +2730,7 @@
   // to C++ function (but doesn't require it to be a member function).
   // FIXME: should this attribute have a CPlusPlus language option?
   let Spellings = [Clang<"callable_when", 0>];
-  let Subjects = SubjectList<[CXXMethod]>;
+  let Subjects = SubjectList<[NonStaticNonConstructorCXXMethod]>;
   let Args = [VariadicEnumArgument<"CallableStates", "ConsumedState",
["unknown", "consumed", "unconsumed"],
["Unknown", "Consumed", "Unconsumed"]>];
@@ -2761,7 +2766,7 @@
   // to C++ function (but doesn't require it to be a member function).
   // FIXME: should this attribute have a CPlusPlus language option?
   let Spellings = [Clang<"set_typestate", 0>];
-  let Subjects = SubjectList<[CXXMethod]>;
+  let Subjects = SubjectList<[NonStaticNonConstructorCXXMethod]>;
   let Args = [EnumArgument<"NewState", "ConsumedState",
["unknown", "consumed", "unconsumed"],
["Unknown", "Consumed", "Unconsumed"]>];
@@ -2773,7 +2778,7 @@
   // to C++ function (but doesn't require it to be a member function).
   // FIXME: should this attribute have a CPlusPlus language

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 220926.
comex added a comment.

In D67647#1674795 , @dblaikie wrote:

> Right - I was suggesting that could be changed. Would it be OK to you to 
> change arguments() to return ArrayRef? Or would you rather avoid that to 
> avoid baking in more dependence on that alias violation?


I'd rather avoid that for the reason you said.

> Same reason I've pushed back on adding things like "empty()" or "size()", 
> etc. Which are fine (& have been added in STLExtras) as free functions.

Okay, I'll just do that then.  Added as `index(Range, N)`, matching range-v3 
(though not the actual C++20 draft).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647

Files:
  clang/include/clang/AST/Stmt.h
  clang/lib/Analysis/Consumed.cpp
  llvm/include/llvm/ADT/STLExtras.h
  llvm/include/llvm/ADT/iterator_range.h

Index: llvm/include/llvm/ADT/iterator_range.h
===
--- llvm/include/llvm/ADT/iterator_range.h
+++ llvm/include/llvm/ADT/iterator_range.h
@@ -20,9 +20,17 @@
 
 #include 
 #include 
+#include 
 
 namespace llvm {
 
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
@@ -58,11 +66,31 @@
   return iterator_range(std::move(p.first), std::move(p.second));
 }
 
+/// Non-random-iterator version
 template 
-iterator_range()))> drop_begin(T &&t,
-  int n) {
-  return make_range(std::next(adl_begin(t), n), adl_end(t));
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  for (int i = 0; i < n; i++) {
+assert(begin != end);
+++begin;
+  }
+  return make_range(begin, end);
 }
+
+/// Optimized version for random iterators
+template 
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  assert(end - begin >= n && "Dropping more elements than exist!");
+  return make_range(std::next(begin, n), end);
+}
+
 }
 
 #endif
Index: llvm/include/llvm/ADT/STLExtras.h
===
--- llvm/include/llvm/ADT/STLExtras.h
+++ llvm/include/llvm/ADT/STLExtras.h
@@ -1573,6 +1573,14 @@
 }
 template  constexpr T *to_address(T *P) { return P; }
 
+template 
+auto index(R &&TheRange,
+  typename std::iterator_traits>::difference_type N)
+  -> decltype(TheRange.begin()[N]) {
+  assert(N < TheRange.end() - TheRange.begin() && "Index out of range!");
+  return TheRange.begin()[N];
+}
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_STLEXTRAS_H
Index: clang/lib/Analysis/Consumed.cpp
===
--- clang/lib/Analysis/Consumed.cpp
+++ clang/lib/Analysis/Consumed.cpp
@@ -494,8 +494,10 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+
+  using ArgRange = llvm::iterator_range;
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArgRange args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +610,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArgRange Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argument lists.
-if (Index - Offset >= FunD->getNumParams())
+if (Index >= FunD->getNumParams())
   break;
 
-const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+const ParmVarDecl *Param = FunD->getParamDecl(Index++);
 QualType ParamType = Param->getType();
 
-InfoEntry Entry = findInfo(Call->getArg(Index));
+InfoEntry Entry = findInfo(Arg);
 
 if (Entry == P

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/ADT/iterator_range.h:27-33
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+

dblaikie wrote:
> Is this how the C++ standard is doing things these days? Or is it usually 
> done with variable templates?
"Type trait"-like things have gone through three different evolutions:

- C++11 struct templates: `std::is_integral::value`
- C++17 variable templates: `std::is_integral_v`
- C++20 concepts: `std::integral`

In fact, the C++20 draft has a `random_access_iterator` concept, though 
there is no `is_random_access_iterator` in prior versions.

However, with LLVM still built as C++11, this helper has to be a struct 
template or a constexpr function.  It seemed a bit simpler to use a constexpr 
function, since template specialization wasn't needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67740: [Consumed] Refactor and improve diagnostics

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Ping...


Repository:
  rC Clang

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

https://reviews.llvm.org/D67740



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 225557.
comex marked 2 inline comments as done.
comex added a comment.

So, I landed this patch but had to revert it as it broke the build on MSVC (and 
only MSVC).  That was almost a month ago, but I haven't gotten back around to 
this until now – sorry :|

In this updated patch, I switched is_random_iterator from a constexpr function:

  template 
  constexpr bool is_random_iterator() {
return std::is_same<
  typename std::iterator_traits::iterator_category,
  std::random_access_iterator_tag>::value;
  }

to a type alias:

  template 
  using is_random_iterator =
std::is_same::iterator_category,
 std::random_access_iterator_tag>;

and changed the uses accordingly.  For some reason, MSVC thought the two 
overloads of `drop_begin`, one with `enable_if()>` and 
one with `enable_if()>`, were duplicate definitions.  But 
with `is_random_iterator` changed to a type alias, MSVC is fine with them.  GCC 
and Clang think both versions are valid, so I think it's just an MSVC bug.

Simplified example for reference: https://gcc.godbolt.org/z/niXCy4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647

Files:
  clang/include/clang/AST/Stmt.h
  clang/lib/Analysis/Consumed.cpp
  llvm/include/llvm/ADT/STLExtras.h
  llvm/include/llvm/ADT/iterator_range.h

Index: llvm/include/llvm/ADT/iterator_range.h
===
--- llvm/include/llvm/ADT/iterator_range.h
+++ llvm/include/llvm/ADT/iterator_range.h
@@ -20,9 +20,15 @@
 
 #include 
 #include 
+#include 
 
 namespace llvm {
 
+template 
+using is_random_iterator =
+  std::is_same::iterator_category,
+   std::random_access_iterator_tag>;
+
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
@@ -59,11 +65,31 @@
   return iterator_range(std::move(p.first), std::move(p.second));
 }
 
+/// Non-random-iterator version
+template 
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if::value,
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  for (int i = 0; i < n; i++) {
+assert(begin != end);
+++begin;
+  }
+  return make_range(begin, end);
+}
+
+/// Optimized version for random iterators
 template 
-iterator_range()))> drop_begin(T &&t,
-  int n) {
-  return make_range(std::next(adl_begin(t), n), adl_end(t));
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if::value,
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  assert(end - begin >= n && "Dropping more elements than exist!");
+  return make_range(std::next(begin, n), end);
 }
+
 }
 
 #endif
Index: llvm/include/llvm/ADT/STLExtras.h
===
--- llvm/include/llvm/ADT/STLExtras.h
+++ llvm/include/llvm/ADT/STLExtras.h
@@ -1573,6 +1573,14 @@
 }
 template  constexpr T *to_address(T *P) { return P; }
 
+template 
+auto index(R &&TheRange,
+  typename std::iterator_traits>::difference_type N)
+  -> decltype(TheRange.begin()[N]) {
+  assert(N < TheRange.end() - TheRange.begin() && "Index out of range!");
+  return TheRange.begin()[N];
+}
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_STLEXTRAS_H
Index: clang/lib/Analysis/Consumed.cpp
===
--- clang/lib/Analysis/Consumed.cpp
+++ clang/lib/Analysis/Consumed.cpp
@@ -494,8 +494,10 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+
+  using ArgRange = llvm::iterator_range;
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArgRange args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +610,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArgRange Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argum

[PATCH] D66404: [CFG] Make destructor calls more accurate

[Nicholas Allegra via Phabricator via cfe-commits]

comex abandoned this revision.
comex marked 18 inline comments as done.
comex added inline comments.



Comment at: lib/Analysis/CFG.cpp:2102
 case Stmt::CompoundStmtClass:
-  return VisitCompoundStmt(cast(S));
+  return VisitCompoundStmt(cast(S), 
/*ExternallyDestructed=*/false);
 

NoQ wrote:
> This goes over the 80 characters limit :)
Thanks, fixed.



Comment at: lib/Analysis/CFG.cpp:2290-2295
+if (BuildOpts.AddTemporaryDtors) {
+  TempDtorContext Context;
+  VisitForTemporaryDtors(EC->getSubExpr(),
+ /*ExternallyDestructed=*/true, Context);
+}
+return Visit(EC->getSubExpr(), asc);

NoQ wrote:
> This looks like a copy-paste from `VisitExprWithCleanups` with the 
> `ExternallyDestructed` flag flipped. Could you double-check if the update to 
> `asc` that's present in `VisitExprWithCleanups` is relevant here as well? 
> I've actually no idea what do these do and we don't seem to have any tests 
> for them, so feel free to ignore, but it might make sense to at least 
> deduplicate the code.
Oops.  I omitted it because the function is always called with `AlwaysAdd`, but 
should have removed the second parameter to reflect that.  However, on 
reflection, I think it would be better to simply add an `ExternallyDestructed` 
parameter to `Visit` and `VisitExprWithCleanups`.  I'll do that in the updated 
patch.



Comment at: test/Analysis/more-dtors-cfg-output.cpp:3
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++14 
-DCXX2A=0 -fblocks -Wall -Wno-unused -Werror %s > %t.14 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++2a 
-DCXX2A=1 -fblocks -Wall -Wno-unused -Werror %s > %t.17 2>&1
+// RUN: FileCheck --input-file=%t.14 -check-prefixes=CHECK,CXX14 
-implicit-check-not=destructor %s

NoQ wrote:
> Did you mean `t.20`? :)
Yep, will fix.



Comment at: test/Analysis/more-dtors-cfg-output.cpp:50
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)

NoQ wrote:
> Maybe also add `CXX20-NOT` so that to make sure that the destructor *isn't* 
> there? (i think this applies to a lot of other tests)
The `-implicit-check-not=destructor` handles that: it complains if any line 
containing "destructor" isn't covered by a CHECK.



Comment at: test/Analysis/more-dtors-cfg-output.cpp:93
+
+void elided_lambda_capture_init() {
+  // The copy from get_foo() into the lambda should be elided.  Should call

NoQ wrote:
> Pls mention somehow that this language feature is called "generalized lambda 
> captures" because it's fairly hard to google for :)
Okay :)



Comment at: test/Analysis/more-dtors-cfg-output.cpp:233
+
+// FIXME: Here are some cases that are currently handled wrongly:
+

NoQ wrote:
> I'm afraid that nobody will notice this comment when more items will be added 
> to this test file. Having a `FIXME` in tests themselves is sufficient.
Okay, I'll remove it.



Comment at: test/Analysis/more-dtors-cfg-output.cpp:251-252
+void default_ctor_with_default_arg() {
+  // FIXME: The CFG records a construction of the array type but a destruction
+  // of the base type, which is inconsistent.  It really should should be
+  // emitting a loop, which should also contain the construction/destruction of

NoQ wrote:
> Is it just mismatching dump styles or is it an actual error?
Oops – it's actually just the printing code for implicit destructors that does:

```
if (const ArrayType *AT = ACtx.getAsArrayType(T))
  T = ACtx.getBaseElementType(AT);
```

I'm not sure why that's there: perhaps because `~Foo[]()` is not valid C++ 
syntax, but there's plenty of other places in the printing code that can print 
invalid syntax, and printing just `~Foo()` is misleading.
For now, until this code is overhauled to emit loops for constructors and 
destructors, I think it makes the most sense to remove those two lines.  I'll 
do that in the updated patch.



Comment at: test/Analysis/more-dtors-cfg-output.cpp:252-254
+  // of the base type, which is inconsistent.  It really should should be
+  // emitting a loop, which should also contain the construction/destruction of
+  // default arguments.

NoQ wrote:
> These should most likely be two separate loops: default arguments are 
> destroyed immediately after the constructor of `qux[2]`, but elements of 
> `qux` should not be destroyed before the end of the scope.
Yeah, that's what I meant.  I'll clarify the text.



Comment at: test/Analysis/more-dtors-cfg-output.cpp:273
+#if CXX2A
+// Boilerplate needed to test co_return:
+

NoQ wrote:
> Feel free to move this into 
> `test/Analysis/inputs/system-header-simulator-cxx.h`, we could always 

[PATCH] D66404: [CFG] Make destructor calls more accurate

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 216545.
comex marked 9 inline comments as done.
comex added a comment.

Changes since last version:

- Rebased.
- Added `ExternallyDestructed` parameter to `VisitExprWithCleanups` and 
`CFGBuilder::Visit`; removed `VisitExternallyDestructed`.
- Changed CFG printing to show when the type being destructed is an array.
- Fixed temporary filename mismatch in test.
- Improved comments to address feedback.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66404

Files:
  clang/lib/Analysis/CFG.cpp
  clang/test/Analysis/auto-obj-dtors-cfg-output.cpp
  clang/test/Analysis/cfg-rich-constructors.cpp
  clang/test/Analysis/cfg-rich-constructors.mm
  clang/test/Analysis/cfg.cpp
  clang/test/Analysis/missing-bind-temporary.cpp
  clang/test/Analysis/more-dtors-cfg-output.cpp
  clang/test/Analysis/scopes-cfg-output.cpp
  clang/test/Analysis/temporaries.cpp

Index: clang/test/Analysis/temporaries.cpp
===
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -830,12 +830,7 @@
   // On each branch the variable is constructed directly.
   if (coin) {
 clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
-#if __cplusplus < 201703L
 clang_analyzer_eval(y == 1); // expected-warning{{TRUE}}
-#else
-// FIXME: Destructor called twice in C++17?
-clang_analyzer_eval(y == 2); // expected-warning{{TRUE}}
-#endif
 clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
 clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
 
@@ -843,12 +838,7 @@
 clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
 clang_analyzer_eval(z == 1); // expected-warning{{TRUE}}
-#if __cplusplus < 201703L
 clang_analyzer_eval(w == 1); // expected-warning{{TRUE}}
-#else
-// FIXME: Destructor called twice in C++17?
-clang_analyzer_eval(w == 2); // expected-warning{{TRUE}}
-#endif
   }
 }
 } // namespace test_match_constructors_and_destructors
@@ -1055,16 +1045,11 @@
 #endif
 
   bar2(S(2));
-  // FIXME: Why are we losing information in C++17?
   clang_analyzer_eval(glob == 2);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-  // expected-warning@-3{{TRUE}}
-#else
-  // expected-warning@-5{{UNKNOWN}}
-#endif
+  // expected-warning@-2{{TRUE}}
 #else
-  // expected-warning@-8{{UNKNOWN}}
+  // expected-warning@-4{{UNKNOWN}}
 #endif
 
   C *c = new D();
Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -38,7 +38,7 @@
 // CHECK-NEXT:   3: A a[2];
 // CHECK-NEXT:   4:  (CXXConstructExpr, [B1.5], class A [0])
 // CHECK-NEXT:   5: A b[0];
-// CHECK-NEXT:   6: [B1.3].~A() (Implicit destructor)
+// CHECK-NEXT:   6: [B1.3].~A [2]() (Implicit destructor)
 // CHECK-NEXT:   7: CFGScopeEnd(a)
 // CHECK-NEXT:   Preds (1): B2
 // CHECK-NEXT:   Succs (1): B0
@@ -810,7 +810,7 @@
 // CHECK-NEXT:   1: CFGScopeEnd(__end1)
 // CHECK-NEXT:   2: CFGScopeEnd(__begin1)
 // CHECK-NEXT:   3: CFGScopeEnd(__range1)
-// CHECK-NEXT:   4: [B5.3].~A() (Implicit destructor)
+// CHECK-NEXT:   4: [B5.3].~A [10]() (Implicit destructor)
 // CHECK-NEXT:   5: CFGScopeEnd(a)
 // CHECK-NEXT:   Preds (1): B2
 // CHECK-NEXT:   Succs (1): B0
Index: clang/test/Analysis/more-dtors-cfg-output.cpp
===
--- /dev/null
+++ clang/test/Analysis/more-dtors-cfg-output.cpp
@@ -0,0 +1,317 @@
+// RUN: rm -f %t.14 %t.2a
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++14 -DCXX2A=0 -fblocks -Wall -Wno-unused -Werror %s > %t.14 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++2a -DCXX2A=1 -fblocks -Wall -Wno-unused -Werror %s > %t.2a 2>&1
+// RUN: FileCheck --input-file=%t.14 -check-prefixes=CHECK,CXX14 -implicit-check-not=destructor %s
+// RUN: FileCheck --input-file=%t.2a -check-prefixes=CHECK,CXX2A -implicit-check-not=destructor %s
+
+int puts(const char *);
+
+struct Foo {
+  Foo() = delete;
+#if CXX2A
+  // Guarantee that the elided examples are actually elided by deleting the
+  // copy constructor.
+  Foo(const Foo &) = delete;
+#else
+  // No elision support, so we need a copy constructor.
+  Foo(const Foo &);
+#endif
+  ~Foo();
+};
+
+struct TwoFoos {
+  Foo foo1, foo2;
+  ~TwoFoos();
+};
+
+Foo get_foo();
+
+struct Bar {
+  Bar();
+  Bar(const Bar &);
+  ~Bar();
+  Bar &operator=(const Bar &);
+};
+
+Bar get_bar();
+
+struct TwoBars {
+  Bar foo1, foo2;
+  ~TwoBars();
+};
+
+// Start of tests:
+
+void elided_assign() {
+  Foo x = get_foo();
+}
+// CHECK: void elided_assign()
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)
+
+void nonelided_assign() {
+  

[PATCH] D66067: Push lambda scope earlier when transforming lambda expression

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 217230.
comex marked 2 inline comments as done.
comex added a comment.

Addressed review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66067

Files:
  lib/Sema/TreeTransform.h
  test/SemaTemplate/default-arguments-cxx0x.cpp


Index: test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- test/SemaTemplate/default-arguments-cxx0x.cpp
+++ test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +115,17 @@
 S _a{};
   };
 }
+
+#if __cplusplus >= 201402L
+namespace lambda {
+  // Verify that a default argument in a lambda can refer to the type of a
+  // previous `auto` argument without crashing.
+  template 
+  void bar() {
+(void) [](auto c, int x = sizeof(decltype(c))) {};
+  }
+  void foo() {
+bar();
+  }
+} // namespace lambda
+#endif
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -11318,10 +11318,14 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
   E->getTemplateParameterList());
+  LSI->GLTemplateParameterList = TPL;
 
   // Transform the type of the original lambda's call operator.
   // The transformation MUST be done in the CurrentInstantiationScope since
@@ -11348,10 +11352,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class


Index: test/SemaTemplate/default-arguments-cxx0x.cpp
===
--- test/SemaTemplate/default-arguments-cxx0x.cpp
+++ test/SemaTemplate/default-arguments-cxx0x.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics
 
 // Test default template arguments for function templates.
@@ -114,3 +115,17 @@
 S _a{};
   };
 }
+
+#if __cplusplus >= 201402L
+namespace lambda {
+  // Verify that a default argument in a lambda can refer to the type of a
+  // previous `auto` argument without crashing.
+  template 
+  void bar() {
+(void) [](auto c, int x = sizeof(decltype(c))) {};
+  }
+  void foo() {
+bar();
+  }
+} // namespace lambda
+#endif
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -11318,10 +11318,14 @@
 }
   }
 
+  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
+  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(
   E->getTemplateParameterList());
+  LSI->GLTemplateParameterList = TPL;
 
   // Transform the type of the original lambda's call operator.
   // The transformation MUST be done in the CurrentInstantiationScope since
@@ -11348,10 +11352,6 @@
 NewCallOpType);
   }
 
-  LambdaScopeInfo *LSI = getSema().PushLambdaScope();
-  Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
-  LSI->GLTemplateParameterList = TPL;
-
   // Create the local class that will describe the lambda.
   CXXRecordDecl *OldClass = E->getLambdaClass();
   CXXRecordDecl *Class
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66067: Push lambda scope earlier when transforming lambda expression

[Nicholas Allegra via Phabricator via cfe-commits]

comex added inline comments.



Comment at: clang/test/SemaTemplate/default-arguments-cxx0x.cpp:1
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
 // expected-no-diagnostics

lebedev.ri wrote:
> Mordante wrote:
> > comex wrote:
> > > Mordante wrote:
> > > > Wouldn't it be better to keep the original C++11 test and add a second 
> > > > `RUN:` for the C++14 test?
> > > > Then guard the new test with `#if __cplusplus >= 201402L`
> > > Perhaps, but most tests aren't run multiple times with different -std 
> > > options, and I didn't see any reason this test in particular needed that. 
> > >  Maybe it should just be renamed to default-arguments-cxx14.cpp.
> > I'm rather new here so I don't know what the renaming policy is, so I leave 
> > it to the more experienced reviewers.
> The description does not call it out specifically i think - why does this 
> need `-std=c++14`?
> That being said i see absolutely no reason not to just have two run lines 
> here.
The ability to make a lambda parameter `auto`-typed is a C++14 feature.

Okay, I'll just add two run lines.



Comment at: clang/test/SemaTemplate/default-arguments-cxx0x.cpp:118
+
+namespace lambda {
+template 

lebedev.ri wrote:
> This is only simply checking that clang doesn't crash on this, right?
> Add a comment about that?
Okay.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66067



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

[PATCH] D66404: [CFG] Make destructor calls more accurate

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Heh, I guess I'll request commit access, although I'm not sure if I have enough 
of a 'track record of submitting high quality patches'.  But for now, can you 
commit this?  Thanks :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66404



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

[PATCH] D66830: Consumed checker: various improvements

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added reviewers: delesley, dblaikie, rsmith.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

- Treat arguments to constructor calls the same way as arguments to other calls 
(fixes 42856).  Previously, arguments to constructors were completely ignored – 
even if explicitly marked `param_typestate` or `return_typestate` – except for 
one special case to mark the argument to a move constructor as consumed on 
return.  Even that special case was broken, as it wouldn't apply if a move 
constructor happened to be marked with `return_typestate`.
  - Constructors are still special-cased when determining the default typestate 
for the newly constructed object, unless the constructor is marked 
`return_typestate`.

- Stop ignoring `return_typestate` on rvalue reference parameters.

- When a function takes a consumable object by value, treat that as consuming 
by default, same as with an rvalue reference.

  Note that the object being consumed is always a temporary.  If you write:

  `void a(X); a((X &&)x);`

  ...this first constructs a temporary X by moving from `x`, then calls `a` 
with a pointer to the temporary.  Before and after this change, the move copies 
`x`'s typestate to the temporary and marks `x` itself as consumed.  But before 
this change, if the temporary started out unconsumed (because `x` was 
unconsumed before the move), it would still be unconsumed when its destructor 
was run after the call.  Now it will be considered consumed at that point.

  Note that currently, only parameters explicitly marked `return_typestate` 
have their state-on-return checked on the callee side.  Parameters which are 
implicitly consuming due to being rvalue references – or, after this patch, 
by-value – are checked only on the caller side.  This discrepancy was very 
surprising to me as a user.  I wrote a fix, but in my codebase it broke some 
code that was using `std::forward`.  Therefore, I plan to hold off on 
submitting the fix until a followup patch, which will generalize the current 
`std::move` special case into an attribute that can be applied to any 'cast' 
function like `std::move` and `std::forward`.

- Diagnose if a constructor or a static method is marked `set_typestate`, 
instead of (respectively) ignoring it or crashing.

- Diagnose if a `param_typestate` or `return_typestate` attribute is attached 
to a parameter of non-consumable type.  The wording I came up with is a bit 
fuzzy; suggestions welcome.

- Fix broken check that was trying to `dyn_cast` a `CXXOperatorCallExpr` to a 
sibling class, `CXXMemberCallExpr` (that will never work).  Instead, we need to 
check whether the declaration is a `CXXMethodDecl`.
  - This was harmless in the existing code, for somewhat complicated reasons, 
but caused problems after I refactored `handleCall` to take the arguments as a 
separate parameter.

Some of these changes have the potential to 'break' code (in the sense of 
creating warnings where there were none before) because they check things that 
were previously ignored.

One notable case, which showed up in the existing test code 
(warn-consumed-analysis.cpp), is a copy constructor that takes its argument by 
non-const reference.  Previously, the argument would be ignored and thus 
implicitly treated as leaving the state alone – just like all other constructor 
arguments, except for the argument to a move constructor.  Now it's given the 
usual treatment for non-const reference parameters, which is to mark the 
original object as being in `unknown` state, unless overridden with 
`return_typestate`.  I think the new behavior is more correct, and this isn't 
removing a special case *per se*.  However, since it is a "copy" constructor, 
there might be some argument that the argument should be treated as 
non-consuming instead.

Also, if anyone wants to test this on their code, note that I have a separate 
patch pending to make the CFG more accurate with respect to temporary object 
destructors (D66404 ).


Repository:
  rC Clang

https://reviews.llvm.org/D66830

Files:
  include/clang/Analysis/Analyses/Consumed.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/Consumed.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/SemaCXX/warn-consumed-analysis.cpp
  test/SemaCXX/warn-consumed-parsing.cpp

Index: test/SemaCXX/warn-consumed-parsing.cpp
===
--- test/SemaCXX/warn-consumed-parsing.cpp
+++ test/SemaCXX/warn-consumed-parsing.cpp
@@ -62,5 +62,12 @@
   Status {
 };
 
-
+struct CONSUMABLE(unknown) UselessAttrs {
+  static void x() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+  UselessAttrs() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'Usele

[PATCH] D65694: Properly instantiate a decltype in argument's default initializer

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65694



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

[PATCH] D74684: [Sema][C++] Adopt DR2171 relaxed triviality requirements

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When defining a copy constructor or assignment operator as either explicitly 
defaulted (`= default`) or deleted (`= delete`), it's possible to give it a 
different parameter list from the normal `(const Foo &)`.

For `= default`, the only allowed change [1] is to use a non-const reference 
instead of a const one:

struct X {

  X(X &) = default;

};

For `= delete`, it's also possible to add additional parameters with default 
values, as well as variadic parameters (`...`).

(Besides the parameter list, it's also possible to change the exception 
specification and ref-qualifiers, but those never affected triviality.)

Originally, C++11 (which introduced explicit defaulting) had a rule that any 
modification of the parameter list would prevent the function from being 
considered trivial, so e.g. is_trivially_copyable_v would be false.  
However, Defect Report 2171 [2] (2016) removed that rule entirely.  Up until 
now, that change hasn't been implemented in Clang; this patch implements it.

In addition, Clang currently applies a similar rule to deleted *default* 
constructors, which AFAICT is not in compliance with any published version of 
the spec.  So this fails when it should pass:

struct X {

  X(...) = delete;

};
static_assert(__has_trivial_constructor(X));

This patch also fixes that.

This is an ABI-breaking change, since the existence of trivial constructors 
affects whether a type is "trivial for the purposes of calls" [3].  I checked 
other compilers to see whether they implement the DR:

- GCC has never treated such functions as nontrivial, even before the DR. [4]
- MSVC currently doesn't treat them as nontrivial [5]; I haven't checked old 
versions since they're not on Godbolt.

Thus the change would improve Clang's compatibility with those compilers.

Implementation-wise, this patch is simple enough: it just removes the code in 
Sema::SpecialMemberIsTrivial that checked the parameter list.  In terms of 
tests, there were already a number of tests verifying the old behavior, so the 
patch merely updates them for the new behavior.

[1] https://eel.is/c++draft/dcl.fct.def.default#2
[2] http://www.open-std.org/Jtc1/sc22/wg21/docs/cwg_defects.html#2171
[3] https://quuxplusone.github.io/blog/2018/05/02/trivial-abi-101/
[4] https://gcc.godbolt.org/z/GU_wyz (note GCC version)
[5] https://gcc.godbolt.org/z/VYiMn3


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74684

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/class/class.union/p1.cpp
  clang/test/CXX/special/class.copy/p12-0x.cpp
  clang/test/CXX/special/class.copy/p25-0x.cpp
  clang/test/CXX/special/class.ctor/p5-0x.cpp

Index: clang/test/CXX/special/class.ctor/p5-0x.cpp
===
--- clang/test/CXX/special/class.ctor/p5-0x.cpp
+++ clang/test/CXX/special/class.ctor/p5-0x.cpp
@@ -175,30 +175,27 @@
 // has a trivial default constructor.
 ASSERT_NONTRIVIAL(NonTrivialDefCtor6, , NonTrivialDefCtor1 t;)
 
-// FIXME: No core issue number yet.
-// - its parameter-declaration-clause is equivalent to that of an implicit declaration.
-struct NonTrivialDefCtor7 {
-  NonTrivialDefCtor7(...) = delete;
+// Otherwise, the default constructor is non-trivial.
+struct Trivial2 {
+  Trivial2(...) = delete;
 };
-static_assert(!__has_trivial_constructor(NonTrivialDefCtor7), "");
-struct NonTrivialDefCtor8 {
-  NonTrivialDefCtor8(int = 0) = delete;
+static_assert(__has_trivial_constructor(Trivial2), "Trivial2 is trivial");
+struct Trivial3 {
+  Trivial3(int = 0) = delete;
 };
-static_assert(!__has_trivial_constructor(NonTrivialDefCtor8), "");
-
-// Otherwise, the default constructor is non-trivial.
+static_assert(__has_trivial_constructor(Trivial3), "Trivial3 is trivial");
 
-class Trivial2 { Trivial2() = delete; };
-static_assert(__has_trivial_constructor(Trivial2), "Trivial2 is trivial");
+class Trivial4 { Trivial4() = delete; };
+static_assert(__has_trivial_constructor(Trivial4), "Trivial4 is trivial");
 
-class Trivial3 { Trivial3() = default; };
-static_assert(__has_trivial_constructor(Trivial3), "Trivial3 is trivial");
+class Trivial5 { Trivial5() = default; };
+static_assert(__has_trivial_constructor(Trivial5), "Trivial5 is trivial");
 
-template class Trivial4 { Trivial4() = default; };
-static_assert(__has_trivial_constructor(Trivial4), "Trivial4 is trivial");
+template class Trivial6 { Trivial6() = default; };
+static_assert(__has_trivial_constructor(Trivial6), "Trivial6 is trivial");
 
-template class Trivial5 { Trivial5() = delete; };
-static_assert(__has_trivial_constructor(Trivial5), "Trivial5 is trivial");
+template class Trivial7 { Trivial7() = delete; };
+static_assert(__has_trivial_constructor(Trivial7), "Trivial7 is trivial");
 
 namespace PR14558 {
   // Ensure we determine whether