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

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

I think this commit broke our standalone builds:

CMake Error at utils/hmaptool/CMakeLists.txt:1 (install):

  install PROGRAMS given no DESTINATION!

Was the LLVM_TOOLS_INSTALL_DIR in this file supposed to be changed to 
CLANG_TOOLS_INSTALL_DIR ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117977

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

[PATCH] D130160: [pseudo] Eliminate the dangling-else syntax ambiguity.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 446728.
hokein added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130160

Files:
  clang-tools-extra/pseudo/include/clang-pseudo/Language.h
  clang-tools-extra/pseudo/lib/GLR.cpp
  clang-tools-extra/pseudo/lib/cxx/CXX.cpp
  clang-tools-extra/pseudo/lib/cxx/cxx.bnf
  clang-tools-extra/pseudo/test/cxx/dangling-else.cpp
  clang-tools-extra/pseudo/unittests/GLRTest.cpp

Index: clang-tools-extra/pseudo/unittests/GLRTest.cpp
===
--- clang-tools-extra/pseudo/unittests/GLRTest.cpp
+++ clang-tools-extra/pseudo/unittests/GLRTest.cpp
@@ -634,11 +634,12 @@
 start := IDENTIFIER [guard]
   )bnf");
   TestLang.Guards.try_emplace(
-  ruleFor("start"),
-  [&](llvm::ArrayRef RHS, const TokenStream &Tokens) {
-assert(RHS.size() == 1 &&
-   RHS.front()->symbol() == tokenSymbol(clang::tok::identifier));
-return Tokens.tokens()[RHS.front()->startTokenIndex()].text() == "test";
+  ruleFor("start"), [&](const GuardParams &P) {
+assert(P.RHS.size() == 1 &&
+   P.RHS.front()->symbol() ==
+   tokenSymbol(clang::tok::identifier));
+return P.Tokens.tokens()[P.RHS.front()->startTokenIndex()]
+   .text() == "test";
   });
   clang::LangOptions LOptions;
   TestLang.Table = LRTable::buildSLR(TestLang.G);
Index: clang-tools-extra/pseudo/test/cxx/dangling-else.cpp
===
--- /dev/null
+++ clang-tools-extra/pseudo/test/cxx/dangling-else.cpp
@@ -0,0 +1,22 @@
+// RUN: clang-pseudo -grammar=cxx -source=%s --start-symbol=statement-seq --print-forest | FileCheck %s
+
+// Verify the else should belong to the nested if statement
+if (true) if (true) {} else {}
+
+// CHECK:  statement-seq~selection-statement := IF ( condition ) statement
+// CHECK-NEXT: ├─IF
+// CHECK-NEXT: ├─(
+// CHECK-NEXT: ├─condition~TRUE
+// CHECK-NEXT: ├─)
+// CHECK-NEXT: └─statement~selection-statement
+// CHECK-NEXT:   ├─IF
+// CHECK-NEXT:   ├─(
+// CHECK-NEXT:   ├─condition~TRUE
+// CHECK-NEXT:   ├─)
+// CHECK-NEXT:   ├─statement~compound-statement := { }
+// CHECK-NEXT:   │ ├─{
+// CHECK-NEXT:   │ └─}
+// CHECK-NEXT:   ├─ELSE
+// CHECK-NEXT:   └─statement~compound-statement := { }
+// CHECK-NEXT: ├─{
+// CHECK-NEXT: └─}
Index: clang-tools-extra/pseudo/lib/cxx/cxx.bnf
===
--- clang-tools-extra/pseudo/lib/cxx/cxx.bnf
+++ clang-tools-extra/pseudo/lib/cxx/cxx.bnf
@@ -290,7 +290,7 @@
 compound-statement := { statement-seq_opt [recover=Brackets] }
 statement-seq := statement
 statement-seq := statement-seq statement
-selection-statement := IF CONSTEXPR_opt ( init-statement_opt condition ) statement
+selection-statement := IF CONSTEXPR_opt ( init-statement_opt condition ) statement [guard]
 selection-statement := IF CONSTEXPR_opt ( init-statement_opt condition ) statement ELSE statement
 selection-statement := SWITCH ( init-statement_opt condition ) statement
 iteration-statement := WHILE ( condition ) statement
Index: clang-tools-extra/pseudo/lib/cxx/CXX.cpp
===
--- clang-tools-extra/pseudo/lib/cxx/CXX.cpp
+++ clang-tools-extra/pseudo/lib/cxx/CXX.cpp
@@ -156,15 +156,19 @@
   llvm_unreachable("unreachable");
 }
 
+bool guardNextTokenNotElse(const GuardParams &P) {
+  return symbolToToken(P.Lookahead) != tok::kw_else;
+}
+
 llvm::DenseMap buildGuards() {
 #define TOKEN_GUARD(kind, cond)\
-  [](llvm::ArrayRef RHS, const TokenStream &Tokens) {  \
-const Token &Tok = onlyToken(tok::kind, RHS, Tokens);  \
+  [](const GuardParams& P) {   \
+const Token &Tok = onlyToken(tok::kind, P.RHS, P.Tokens);  \
 return cond;   \
   }
 #define SYMBOL_GUARD(kind, cond)   \
-  [](llvm::ArrayRef RHS, const TokenStream &Tokens) {  \
-const ForestNode &N = onlySymbol((SymbolID)Symbol::kind, RHS, Tokens); \
+  [](const GuardParams& P) {   \
+const ForestNode &N = onlySymbol((SymbolID)Symbol::kind, P.RHS, P.Tokens); \
 return cond;   \
   }
   return {
@@ -186,6 +190,11 @@
   {(RuleID)Rule::contextual_zero_0numeric_constant,
TOKEN_GUARD(numeric_constant, Tok.text() == "0")},
 
+  {(RuleID)Rule::selection_statement_0if_1l_paren_2condition_3r_paren_4statement,
+guardNextTokenNotElse},
+  {(RuleID)Rule::selection_statement_0if_1co

[clang-tools-extra] 2a88fb2 - [pseudo] Eliminate the dangling-else syntax ambiguity.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2022-07-22T09:13:09+02:00
New Revision: 2a88fb2ecb72300bfbbc74c586fb415cc18c9f9d

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

LOG: [pseudo] Eliminate the dangling-else syntax ambiguity.

- the grammar ambiguity is eliminated by a guard;
- modify the guard function signatures, now all parameters are folded in
  to a single object, avoid a long parameter list (as we will add more
  parameters in the near future);

Reviewed By: sammccall

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

Added: 
clang-tools-extra/pseudo/test/cxx/dangling-else.cpp

Modified: 
clang-tools-extra/pseudo/include/clang-pseudo/Language.h
clang-tools-extra/pseudo/lib/GLR.cpp
clang-tools-extra/pseudo/lib/cxx/CXX.cpp
clang-tools-extra/pseudo/lib/cxx/cxx.bnf
clang-tools-extra/pseudo/unittests/GLRTest.cpp

Removed: 




diff  --git a/clang-tools-extra/pseudo/include/clang-pseudo/Language.h 
b/clang-tools-extra/pseudo/include/clang-pseudo/Language.h
index 3696543915cba..1a2b71f081da0 100644
--- a/clang-tools-extra/pseudo/include/clang-pseudo/Language.h
+++ b/clang-tools-extra/pseudo/include/clang-pseudo/Language.h
@@ -19,6 +19,12 @@ class ForestNode;
 class TokenStream;
 class LRTable;
 
+struct GuardParams {
+  llvm::ArrayRef RHS;
+  const TokenStream &Tokens;
+  // FIXME: use the index of Tokens.
+  SymbolID Lookahead;
+};
 // A guard restricts when a grammar rule can be used.
 //
 // The GLR parser will use the guard to determine whether a rule reduction will
@@ -26,8 +32,7 @@ class LRTable;
 // `virt-specifier := IDENTIFIER` only if the identifier's text is 'override`.
 //
 // Return true if the guard is satisfied.
-using RuleGuard = llvm::function_ref RHS, const TokenStream &)>;
+using RuleGuard = llvm::function_ref;
 
 // A recovery strategy determines a region of code to skip when parsing fails.
 //

diff  --git a/clang-tools-extra/pseudo/lib/GLR.cpp 
b/clang-tools-extra/pseudo/lib/GLR.cpp
index df8381d04326b..ab230accdf8f8 100644
--- a/clang-tools-extra/pseudo/lib/GLR.cpp
+++ b/clang-tools-extra/pseudo/lib/GLR.cpp
@@ -421,7 +421,7 @@ class GLRReduce {
 if (!R.Guarded)
   return true;
 if (auto Guard = Lang.Guards.lookup(RID))
-  return Guard(RHS, Params.Code);
+  return Guard({RHS, Params.Code, Lookahead});
 LLVM_DEBUG(llvm::dbgs()
<< llvm::formatv("missing guard implementation for rule {0}\n",
 Lang.G.dumpRule(RID)));

diff  --git a/clang-tools-extra/pseudo/lib/cxx/CXX.cpp 
b/clang-tools-extra/pseudo/lib/cxx/CXX.cpp
index 8fa24bfbbd0b5..7fc3a48d63189 100644
--- a/clang-tools-extra/pseudo/lib/cxx/CXX.cpp
+++ b/clang-tools-extra/pseudo/lib/cxx/CXX.cpp
@@ -156,15 +156,19 @@ bool isFunctionDeclarator(const ForestNode *Declarator) {
   llvm_unreachable("unreachable");
 }
 
+bool guardNextTokenNotElse(const GuardParams &P) {
+  return symbolToToken(P.Lookahead) != tok::kw_else;
+}
+
 llvm::DenseMap buildGuards() {
 #define TOKEN_GUARD(kind, cond)
\
-  [](llvm::ArrayRef RHS, const TokenStream &Tokens) {  
\
-const Token &Tok = onlyToken(tok::kind, RHS, Tokens);  
\
+  [](const GuardParams& P) {   
\
+const Token &Tok = onlyToken(tok::kind, P.RHS, P.Tokens);  
\
 return cond;   
\
   }
 #define SYMBOL_GUARD(kind, cond)   
\
-  [](llvm::ArrayRef RHS, const TokenStream &Tokens) {  
\
-const ForestNode &N = onlySymbol((SymbolID)Symbol::kind, RHS, Tokens); 
\
+  [](const GuardParams& P) {   
\
+const ForestNode &N = onlySymbol((SymbolID)Symbol::kind, P.RHS, P.Tokens); 
\
 return cond;   
\
   }
   return {
@@ -186,6 +190,11 @@ llvm::DenseMap buildGuards() {
   {(RuleID)Rule::contextual_zero_0numeric_constant,
TOKEN_GUARD(numeric_constant, Tok.text() == "0")},
 
+  
{(RuleID)Rule::selection_statement_0if_1l_paren_2condition_3r_paren_4statement,
+guardNextTokenNotElse},
+  
{(RuleID)Rule::selection_statement_0if_1constexpr_2l_paren_3condition_4r_paren_5statement,
+guardNextTokenNotElse},
+
   // The grammar distinguishes (only) user-defined vs plain string 
literals,
   // where the clang lexer distinguishes (only) encoding types.
   {(RuleID)Rule::user_defined_string_literal_chunk_0string_literal,

diff  --git a/clang-tools-extra/pseudo/lib/cxx/cxx.bnf 
b/clang-tools-extra/pseudo/lib/cxx/cxx.bnf
index d49fb8fb7cf42..8138d0fd481ed 100644
--- a/clang-tools-extra/pseudo/lib/

[PATCH] D130160: [pseudo] Eliminate the dangling-else syntax ambiguity.

[Haojian Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2a88fb2ecb72: [pseudo] Eliminate the dangling-else syntax 
ambiguity. (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130160

Files:
  clang-tools-extra/pseudo/include/clang-pseudo/Language.h
  clang-tools-extra/pseudo/lib/GLR.cpp
  clang-tools-extra/pseudo/lib/cxx/CXX.cpp
  clang-tools-extra/pseudo/lib/cxx/cxx.bnf
  clang-tools-extra/pseudo/test/cxx/dangling-else.cpp
  clang-tools-extra/pseudo/unittests/GLRTest.cpp

Index: clang-tools-extra/pseudo/unittests/GLRTest.cpp
===
--- clang-tools-extra/pseudo/unittests/GLRTest.cpp
+++ clang-tools-extra/pseudo/unittests/GLRTest.cpp
@@ -634,11 +634,12 @@
 start := IDENTIFIER [guard]
   )bnf");
   TestLang.Guards.try_emplace(
-  ruleFor("start"),
-  [&](llvm::ArrayRef RHS, const TokenStream &Tokens) {
-assert(RHS.size() == 1 &&
-   RHS.front()->symbol() == tokenSymbol(clang::tok::identifier));
-return Tokens.tokens()[RHS.front()->startTokenIndex()].text() == "test";
+  ruleFor("start"), [&](const GuardParams &P) {
+assert(P.RHS.size() == 1 &&
+   P.RHS.front()->symbol() ==
+   tokenSymbol(clang::tok::identifier));
+return P.Tokens.tokens()[P.RHS.front()->startTokenIndex()]
+   .text() == "test";
   });
   clang::LangOptions LOptions;
   TestLang.Table = LRTable::buildSLR(TestLang.G);
Index: clang-tools-extra/pseudo/test/cxx/dangling-else.cpp
===
--- /dev/null
+++ clang-tools-extra/pseudo/test/cxx/dangling-else.cpp
@@ -0,0 +1,22 @@
+// RUN: clang-pseudo -grammar=cxx -source=%s --start-symbol=statement-seq --print-forest | FileCheck %s
+
+// Verify the else should belong to the nested if statement
+if (true) if (true) {} else {}
+
+// CHECK:  statement-seq~selection-statement := IF ( condition ) statement
+// CHECK-NEXT: ├─IF
+// CHECK-NEXT: ├─(
+// CHECK-NEXT: ├─condition~TRUE
+// CHECK-NEXT: ├─)
+// CHECK-NEXT: └─statement~selection-statement
+// CHECK-NEXT:   ├─IF
+// CHECK-NEXT:   ├─(
+// CHECK-NEXT:   ├─condition~TRUE
+// CHECK-NEXT:   ├─)
+// CHECK-NEXT:   ├─statement~compound-statement := { }
+// CHECK-NEXT:   │ ├─{
+// CHECK-NEXT:   │ └─}
+// CHECK-NEXT:   ├─ELSE
+// CHECK-NEXT:   └─statement~compound-statement := { }
+// CHECK-NEXT: ├─{
+// CHECK-NEXT: └─}
Index: clang-tools-extra/pseudo/lib/cxx/cxx.bnf
===
--- clang-tools-extra/pseudo/lib/cxx/cxx.bnf
+++ clang-tools-extra/pseudo/lib/cxx/cxx.bnf
@@ -290,7 +290,7 @@
 compound-statement := { statement-seq_opt [recover=Brackets] }
 statement-seq := statement
 statement-seq := statement-seq statement
-selection-statement := IF CONSTEXPR_opt ( init-statement_opt condition ) statement
+selection-statement := IF CONSTEXPR_opt ( init-statement_opt condition ) statement [guard]
 selection-statement := IF CONSTEXPR_opt ( init-statement_opt condition ) statement ELSE statement
 selection-statement := SWITCH ( init-statement_opt condition ) statement
 iteration-statement := WHILE ( condition ) statement
Index: clang-tools-extra/pseudo/lib/cxx/CXX.cpp
===
--- clang-tools-extra/pseudo/lib/cxx/CXX.cpp
+++ clang-tools-extra/pseudo/lib/cxx/CXX.cpp
@@ -156,15 +156,19 @@
   llvm_unreachable("unreachable");
 }
 
+bool guardNextTokenNotElse(const GuardParams &P) {
+  return symbolToToken(P.Lookahead) != tok::kw_else;
+}
+
 llvm::DenseMap buildGuards() {
 #define TOKEN_GUARD(kind, cond)\
-  [](llvm::ArrayRef RHS, const TokenStream &Tokens) {  \
-const Token &Tok = onlyToken(tok::kind, RHS, Tokens);  \
+  [](const GuardParams& P) {   \
+const Token &Tok = onlyToken(tok::kind, P.RHS, P.Tokens);  \
 return cond;   \
   }
 #define SYMBOL_GUARD(kind, cond)   \
-  [](llvm::ArrayRef RHS, const TokenStream &Tokens) {  \
-const ForestNode &N = onlySymbol((SymbolID)Symbol::kind, RHS, Tokens); \
+  [](const GuardParams& P) {   \
+const ForestNode &N = onlySymbol((SymbolID)Symbol::kind, P.RHS, P.Tokens); \
 return cond;   \
   }
   return {
@@ -186,6 +190,11 @@
   {(RuleID)Rule::contextual_zero_0numeric_constant,
TOKEN_GUARD(numeric_constant, Tok.text() == "0")},
 
+ 

[clang] afda39a - re-land [C++20][Modules] Build module static initializers per P1874R1.

[Iain Sandoe via cfe-commits]

Author: Iain Sandoe
Date: 2022-07-22T08:38:07+01:00
New Revision: afda39a566d9b076bbcbeac1a6a1d4f6aecb3274

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

LOG: re-land [C++20][Modules] Build module static initializers per P1874R1.

The re-land fixes module map module dependencies seen on Greendragon, but
not in the clang test suite.

---

Currently we only implement this for the Itanium ABI since the correct
mangling for the initializers in other ABIs is not yet known.

Intended result:

For a module interface [which includes partition interface and implementation
units] (instead of the generic CXX initializer) we emit a module init that:

 - wraps the contained initializations in a control variable to ensure that
   the inits only happen once, even if a module is imported many times by
   imports of the main unit.

 - calls module initializers for imported modules first.  Note that the
   order of module import is not significant, and therefore neither is the
   order of imported module initializers.

 - We then call initializers for the Global Module Fragment (if present)
 - We then call initializers for the current module.
 - We then call initializers for the Private Module Fragment (if present)

For a module implementation unit, or a non-module TU that imports at least one
module we emit a regular CXX init that:

 - Calls the initializers for any imported modules first.
 - Then proceeds as normal with remaining inits.

For all module unit kinds we include a global constructor entry, this allows
for the (in most cases unusual) possibility that a module object could be
included in a final binary without a specific call to its initializer.

Implementation:

 - We provide the module pointer in the AST Context so that CodeGen can act
   on it and its sub-modules.

 - We need to account for module build lines like this:
  ` clang -cc1 -std=c++20 Foo.pcm -emit-obj -o Foo.o` or
  ` clang -cc1 -std=c++20 -xc++-module Foo.cpp -emit-obj -o Foo.o`

 - in order to do this, we add to ParseAST to set the module pointer in
   the ASTContext, once we establish that this is a module build and we
   know the module pointer. To be able to do this, we make the query for
   current module public in Sema.

 - In CodeGen, we determine if the current build requires a CXX20-style module
   init and, if so, we defer any module initializers during the "Eagerly
   Emitted" phase.

 - We then walk the module initializers at the end of the TU but before
   emitting deferred inits (which adds any hidden and static ones, fixing
   https://github.com/llvm/llvm-project/issues/51873 ).

 - We then proceed to emit the deferred inits and continue to emit the CXX
   init function.

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

Added: 
clang/test/CodeGen/module-intializer-pmf.cpp
clang/test/CodeGen/module-intializer.cpp

Modified: 
clang/include/clang/AST/ASTContext.h
clang/include/clang/Basic/Module.h
clang/include/clang/Sema/Sema.h
clang/lib/CodeGen/CGDeclCXX.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/Parse/ParseAST.cpp
clang/lib/Sema/SemaModule.cpp

Removed: 




diff  --git a/clang/include/clang/AST/ASTContext.h 
b/clang/include/clang/AST/ASTContext.h
index 85eba45e4de6e..9536b3faa02da 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -473,6 +473,9 @@ class ASTContext : public RefCountedBase {
   };
   llvm::DenseMap ModuleInitializers;
 
+  /// For module code-gen cases, this is the top-level module we are building.
+  Module *TopLevelModule = nullptr;
+
   static constexpr unsigned ConstantArrayTypesLog2InitSize = 8;
   static constexpr unsigned GeneralTypesLog2InitSize = 9;
   static constexpr unsigned FunctionProtoTypesLog2InitSize = 12;
@@ -1076,6 +1079,12 @@ class ASTContext : public RefCountedBase {
   /// Get the initializations to perform when importing a module, if any.
   ArrayRef getModuleInitializers(Module *M);
 
+  /// Set the (C++20) module we are building.
+  void setModuleForCodeGen(Module *M) { TopLevelModule = M; }
+
+  /// Get module under construction, nullptr if this is not a C++20 module.
+  Module *getModuleForCodeGen() const { return TopLevelModule; }
+
   TranslationUnitDecl *getTranslationUnitDecl() const {
 return TUDecl->getMostRecentDecl();
   }

diff  --git a/clang/include/clang/Basic/Module.h 
b/clang/include/clang/Basic/Module.h
index a1778baa04530..47d736a3b4557 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -665,6 +665,18 @@ class Module {
   Module *findSubmodule(StringRef Name) const;
   Module *findOrInferSubmodule(StringRef Name);
 
+  /// Get the Global Module Fragment (sub-module) for this 

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Iain Sandoe via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGafda39a566d9: re-land [C++20][Modules] Build module static 
initializers per P1874R1. (authored by iains).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/Module.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/test/CodeGen/module-intializer-pmf.cpp
  clang/test/CodeGen/module-intializer.cpp

Index: clang/test/CodeGen/module-intializer.cpp
===
--- /dev/null
+++ clang/test/CodeGen/module-intializer.cpp
@@ -0,0 +1,186 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 N.cpp \
+// RUN:-emit-module-interface -o N.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 N.pcm -S -emit-llvm \
+// RUN:  -o - | FileCheck %s --check-prefix=CHECK-N
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 O.cpp \
+// RUN:-emit-module-interface -o O.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 O.pcm -S -emit-llvm \
+// RUN:  -o - | FileCheck %s --check-prefix=CHECK-O
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-part.cpp \
+// RUN:-emit-module-interface -o M-part.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-part.pcm -S \
+// RUN: -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-P
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.cpp \
+// RUN: -fmodule-file=N.pcm -fmodule-file=O.pcm -fmodule-file=M-part.pcm \
+// RUN:-emit-module-interface -o M.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.pcm -S -emit-llvm \
+// RUN:  -o - | FileCheck %s --check-prefix=CHECK-M
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 useM.cpp \
+// RUN: -fmodule-file=M.pcm -S -emit-llvm  -o - \
+// RUN: | FileCheck %s --check-prefix=CHECK-USE
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-impl.cpp \
+// RUN: -fmodule-file=M.pcm -S -emit-llvm  -o - \
+// RUN: | FileCheck %s --check-prefix=CHECK-IMPL
+
+//--- N-h.h
+
+struct Oink {
+  Oink(){};
+};
+
+Oink Hog;
+
+//--- N.cpp
+
+module;
+#include "N-h.h"
+
+export module N;
+
+export struct Quack {
+  Quack(){};
+};
+
+export Quack Duck;
+
+// CHECK-N: define internal void @__cxx_global_var_init
+// CHECK-N: call {{.*}} @_ZN4OinkC1Ev
+// CHECK-N: define internal void @__cxx_global_var_init
+// CHECK-N: call {{.*}} @_ZNW1N5QuackC1Ev
+// CHECK-N: define void @_ZGIW1N
+// CHECK-N: store i8 1, ptr @_ZGIW1N__in_chrg
+// CHECK-N: call void @__cxx_global_var_init
+// CHECK-N: call void @__cxx_global_var_init
+
+//--- O-h.h
+
+struct Meow {
+  Meow(){};
+};
+
+Meow Cat;
+
+//--- O.cpp
+
+module;
+#include "O-h.h"
+
+export module O;
+
+export struct Bark {
+  Bark(){};
+};
+
+export Bark Dog;
+
+// CHECK-O: define internal void @__cxx_global_var_init
+// CHECK-O: call {{.*}} @_ZN4MeowC2Ev
+// CHECK-O: define internal void @__cxx_global_var_init
+// CHECK-O: call {{.*}} @_ZNW1O4BarkC1Ev
+// CHECK-O: define void @_ZGIW1O
+// CHECK-O: store i8 1, ptr @_ZGIW1O__in_chrg
+// CHECK-O: call void @__cxx_global_var_init
+// CHECK-O: call void @__cxx_global_var_init
+
+//--- P-h.h
+
+struct Croak {
+  Croak(){};
+};
+
+Croak Frog;
+
+//--- M-part.cpp
+
+module;
+#include "P-h.h"
+
+module M:Part;
+
+struct Squawk {
+  Squawk(){};
+};
+
+Squawk parrot;
+
+// CHECK-P: define internal void @__cxx_global_var_init
+// CHECK-P: call {{.*}} @_ZN5CroakC1Ev
+// CHECK-P: define internal void @__cxx_global_var_init
+// CHECK-P: call {{.*}} @_ZNW1M6SquawkC1Ev
+// CHECK-P: define void @_ZGIW1MWP4Part
+// CHECK-P: store i8 1, ptr @_ZGIW1MWP4Part__in_chrg
+// CHECK-P: call void @__cxx_global_var_init
+// CHECK-P: call void @__cxx_global_var_init
+
+//--- M-h.h
+
+struct Moo {
+  Moo(){};
+};
+
+Moo Cow;
+
+//--- M.cpp
+
+module;
+#include "M-h.h"
+
+export module M;
+import N;
+export import O;
+import :Part;
+
+export struct Baa {
+  int x;
+  Baa(){};
+  Baa(int x) : x(x) {}
+  int getX() { return x; }
+};
+
+export Baa Sheep(10);
+
+// CHECK-M: define internal void @__cxx_global_var_init
+// CHECK-M: call {{.*}} @_ZN3MooC1Ev
+// CHECK-M: define internal void @__cxx_global_var_init
+// CHECK-M: call {{.*}} @_ZNW1M3BaaC1Ei
+// CHECK-M: declare void @_ZGIW1O()
+// CHECK-M: declare void @_ZGIW1N()
+// CHECK-M: declare void @_ZGIW1MWP4Part()
+// CHECK-M: define void @_ZGIW1M
+// CHECK-M: store i8 1, ptr @_ZGIW1M__in_chrg
+// CHECK-M: call void @_ZGIW1O()
+// CHECK-M: call void @_ZGIW1N()
+// CHECK-M: call void @_ZGIW1MWP4Part()
+// CHECK-M: call void @__cxx_global_var_init
+// CHECK-M: call void @__cxx_global_var_init
+
+//--- useM.cpp

[PATCH] D128619: [Clang] Implement P0848 (Conditionally Trivial Special Member Functions)

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson added inline comments.



Comment at: clang/test/SemaCXX/constrained-special-member-functions.cpp:103
+// FIXME: DR1734.
+static_assert(__is_trivially_copyable(CopyAssignmentChecker<1>));
+static_assert(!__is_trivially_copyable(CopyAssignmentChecker<2>));

cor3ntin wrote:
> Have you rebased since D127593 landed?
Yes. It's related but it's different DRs. And unfortunately they seem much more 
ABI breaking to fix - GCC don't implement them, for example


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128619

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

[PATCH] D130255: [Clang][LoongArch] Add initial LoongArch target and driver support

[Lu Weining via Phabricator via cfe-commits]

SixWeining updated this revision to Diff 446736.
SixWeining added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130255

Files:
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Targets.cpp
  clang/lib/Basic/Targets/LoongArch.cpp
  clang/lib/Basic/Targets/LoongArch.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/ToolChains/Arch/LoongArch.cpp
  clang/lib/Driver/ToolChains/Arch/LoongArch.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/bin/.keep
  clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/include/.keep
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/lib/gcc/loongarch64-unknown-linux-gnu/12.1.0/base/lp64d/crtbegin.o
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/lib/gcc/loongarch64-unknown-linux-gnu/12.1.0/base/lp64f/crtbegin.o
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/lib/gcc/loongarch64-unknown-linux-gnu/12.1.0/base/lp64s/crtbegin.o
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/lib/gcc/loongarch64-unknown-linux-gnu/12.1.0/crtbegin.o
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/loongarch64-unknown-linux-gnu/bin/ld
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/loongarch64-unknown-linux-gnu/lib/.keep
  
clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/loongarch64-unknown-linux-gnu/lib64/.keep
  clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/sysroot/usr/lib/.keep
  clang/test/Driver/Inputs/multilib_loongarch_linux_sdk/sysroot/usr/lib64/.keep
  clang/test/Driver/frame-pointer.c
  clang/test/Driver/loongarch-abi-error.c
  clang/test/Driver/loongarch-abi.c
  clang/test/Driver/loongarch64-toolchain.c
  clang/test/Preprocessor/init-loongarch.c

Index: clang/test/Preprocessor/init-loongarch.c
===
--- /dev/null
+++ clang/test/Preprocessor/init-loongarch.c
@@ -0,0 +1,641 @@
+// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=loongarch32 /dev/null \
+// RUN:   | FileCheck --match-full-lines --check-prefix=LA32 %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=loongarch32-unknown-linux /dev/null \
+// RUN:   | FileCheck --match-full-lines --check-prefixes=LA32,LA32-LINUX %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=loongarch32 \
+// RUN: -fforce-enable-int128 /dev/null | FileCheck --match-full-lines \
+// RUN: --check-prefixes=LA32,LA32-INT128 %s
+
+// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=loongarch64 /dev/null \
+// RUN:   | FileCheck --match-full-lines --check-prefix=LA64 %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=loongarch64-unknown-linux /dev/null \
+// RUN:   | FileCheck --match-full-lines --check-prefixes=LA64,LA64-LINUX %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -fgnuc-version=4.2.1 -triple=loongarch64 \
+// RUN: -fforce-enable-int128 /dev/null | FileCheck --match-full-lines \
+// RUN: --check-prefixes=LA64,LA64-INT128 %s
+
+ Note that common macros are tested in init.c, such as __VERSION__. So they're not listed here.
+
+// LA32: #define _ILP32 1
+// LA32: #define __ATOMIC_ACQUIRE 2
+// LA32-NEXT: #define __ATOMIC_ACQ_REL 4
+// LA32-NEXT: #define __ATOMIC_CONSUME 1
+// LA32-NEXT: #define __ATOMIC_RELAXED 0
+// LA32-NEXT: #define __ATOMIC_RELEASE 3
+// LA32-NEXT: #define __ATOMIC_SEQ_CST 5
+// LA32: #define __BIGGEST_ALIGNMENT__ 16
+// LA32: #define __BITINT_MAXWIDTH__ 128
+// LA32: #define __BOOL_WIDTH__ 8
+// LA32: #define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
+// LA32: #define __CHAR16_TYPE__ unsigned short
+// LA32: #define __CHAR32_TYPE__ unsigned int
+// LA32: #define __CHAR_BIT__ 8
+// LA32: #define __CLANG_ATOMIC_BOOL_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_CHAR16_T_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_CHAR32_T_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_CHAR_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_INT_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_LLONG_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_LONG_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_POINTER_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_SHORT_LOCK_FREE 1
+// LA32: #define __CLANG_ATOMIC_WCHAR_T_LOCK_FREE 1
+// LA32: #define __DBL_DECIMAL_DIG__ 17
+// LA32: #define __DBL_DENORM_MIN__ 4.9406564584124654e-324
+// LA32: #define __DBL_DIG__ 15
+// LA32: #define __DBL_EPSILON__ 2.2204460492503131e-16
+// LA32: #define __DBL_HAS_DENORM__ 1
+// LA32: #define __DBL_HAS_INFINITY__ 1
+// LA32: #define __DBL_HAS_QUIET_NAN__ 1
+// LA32: #define __DBL_MANT_DIG__ 53
+// LA32: #define __DBL_MAX_10_EXP__ 308
+// LA32: #define __DBL_MAX_EXP__ 1024
+// LA32: #define __DBL_MAX__ 1.7976931348623157e+308
+// LA32: #define __DBL_MIN_10_EXP__ (-307)
+// LA32: #define __DBL_MIN_EXP__ (-10

[clang] 70257fa - Use any_of (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2022-07-22T01:05:17-07:00
New Revision: 70257fab68e8081f5a4f182ea580f284b593aa6f

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

LOG: Use any_of (NFC)

Added: 


Modified: 
clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
clang-tools-extra/clangd/index/Merge.cpp
clang/lib/Lex/ModuleMap.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaTemplate.cpp
lld/wasm/SyntheticSections.h
llvm/include/llvm/Passes/StandardInstrumentations.h
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp
polly/lib/Analysis/ScopBuilder.cpp

Removed: 




diff  --git 
a/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp 
b/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
index 245c03fe8e9c3..dbda14c97e5fc 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
@@ -49,13 +49,11 @@ void MacroRepeatedPPCallbacks::MacroExpands(const Token 
&MacroNameTok,
 
   // Bail out if the contents of the macro are containing keywords that are
   // making the macro too complex.
-  if (std::find_if(
-  MI->tokens().begin(), MI->tokens().end(), [](const Token &T) {
-return T.isOneOf(tok::kw_if, tok::kw_else, tok::kw_switch,
- tok::kw_case, tok::kw_break, tok::kw_while,
- tok::kw_do, tok::kw_for, tok::kw_continue,
- tok::kw_goto, tok::kw_return);
-  }) != MI->tokens().end())
+  if (llvm::any_of(MI->tokens(), [](const Token &T) {
+return T.isOneOf(tok::kw_if, tok::kw_else, tok::kw_switch, 
tok::kw_case,
+ tok::kw_break, tok::kw_while, tok::kw_do, tok::kw_for,
+ tok::kw_continue, tok::kw_goto, tok::kw_return);
+  }))
 return;
 
   for (unsigned ArgNo = 0U; ArgNo < MI->getNumParams(); ++ArgNo) {

diff  --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp 
b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index b5e5191876cad..67d8ccbd6cad4 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -604,9 +604,9 @@ int clangTidyMain(int argc, const char **argv) {
   std::vector Errors =
   runClangTidy(Context, OptionsParser->getCompilations(), PathList, BaseFS,
FixNotes, EnableCheckProfile, ProfilePrefix);
-  bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
-   return E.DiagLevel == ClangTidyError::Error;
- }) != Errors.end();
+  bool FoundErrors = llvm::any_of(Errors, [](const ClangTidyError &E) {
+return E.DiagLevel == ClangTidyError::Error;
+  });
 
   // --fix-errors and --fix-notes imply --fix.
   FixBehaviour Behaviour = FixNotes ? FB_FixNotes

diff  --git a/clang-tools-extra/clangd/index/Merge.cpp 
b/clang-tools-extra/clangd/index/Merge.cpp
index 997bbfb6672a1..0d15dfcb1f252 100644
--- a/clang-tools-extra/clangd/index/Merge.cpp
+++ b/clang-tools-extra/clangd/index/Merge.cpp
@@ -196,10 +196,9 @@ static bool prefer(const SymbolLocation &L, const 
SymbolLocation &R) {
 return true;
   auto HasCodeGenSuffix = [](const SymbolLocation &Loc) {
 constexpr static const char *CodegenSuffixes[] = {".proto"};
-return std::any_of(std::begin(CodegenSuffixes), std::end(CodegenSuffixes),
-   [&](llvm::StringRef Suffix) {
- return llvm::StringRef(Loc.FileURI).endswith(Suffix);
-   });
+return llvm::any_of(CodegenSuffixes, [&](llvm::StringRef Suffix) {
+  return llvm::StringRef(Loc.FileURI).endswith(Suffix);
+});
   };
   return HasCodeGenSuffix(L) && !HasCodeGenSuffix(R);
 }

diff  --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 57e344622f25a..47d6f5893e974 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -456,10 +456,8 @@ static bool violatesPrivateInclude(Module 
*RequestingModule,
 &Header.getModule()->Headers[Module::HK_Private],
 &Header.getModule()->Headers[Module::HK_PrivateTextual]};
 for (auto *Hs : HeaderList)
-  IsPrivate |=
-  std::find_if(Hs->begin(), Hs->end(), [&](const Module::Header &H) {
-return H.Entry == IncFileEnt;
-  }) != Hs->end();
+  IsPrivate |= llvm::any_of(
+  *Hs, [&](const Module::Header &H) { return H.Entry == IncFileEnt; });
 assert(IsPrivate && "inconsistent headers and roles");
   }
 #endif

diff  --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp

[PATCH] D130259: [clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

sorry for not mentioning in the bug that i was also working on a patch,  
D130260  seems to be a little bit more 
contained and focused on making the check not crash (as we're getting close to 
release cut).

WDYT about landing it now, and iterating on this patch afterwards if you still 
have cases that it doesn't handle but this patch does? (I can't really see such 
cases in hindsight)




Comment at: clang-tools-extra/clangd/AST.cpp:844
+  // outer variadic call.
+  const auto LastMatchIdx = FirstMatchIdx + Parameters.size() - 1;
+  if (LastMatchIdx < NumArgs) {

so in theory this is still a heuristic, and somewhat complicated. what about 
just checking if we have "enough parameters" to satisfy the pack expansion (as 
i did in D130260)



Comment at: clang-tools-extra/clangd/AST.cpp:914
+  static const Expr *unwrapConstructorConversion(const Expr *E) {
+if (const auto *CBTE = dyn_cast(E)) {
+  E = CBTE->getSubExpr();

this is already handled by `IgnoreImplicitAsWritten` the underlying function is 
using now (landed in https://reviews.llvm.org/D130261)



Comment at: clang-tools-extra/clangd/AST.cpp:919
+  const auto *C = CCE->getConstructor();
+  if (!C->isExplicit() && C->getNumParams() == 1) {
+E = CCE->getArg(0);

this is checking for constructor being explicit, and not the callexpr itself. 
so for example it won't fire up if call is implicit to an explicitly defined 
copy constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130259

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

[PATCH] D130145: [AArch64] Simplify BTI/PAC-RET module flags

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:4415-4430
-// Upgrade branch protection and return address signing module flags. The
-// module flag behavior for these fields were Error and now they are Min.
-if (ID->getString() == "branch-target-enforcement" ||
-ID->getString().startswith("sign-return-address")) {
-  if (auto *Behavior =
-  mdconst::dyn_extract_or_null(Op->getOperand(0))) {
-if (Behavior->getLimitedValue() == Module::Error) {

In a full LTO build the linker will complain about the mis match between the 
flags if one of the objects compiled with an older compiler that emitted 
`Module:Error` for these flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130145

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.
Herald added a subscriber: ormris.
Herald added a project: All.

Could you please add a description of this attribute to LangRef?
We need the semantics of this.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D130259: [clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D130259#3671009 , @kadircet wrote:

> sorry for not mentioning in the bug that i was also working on a patch,

Which bug/issue is this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130259

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

[PATCH] D130259: [clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D130259#3671022 , @nridge wrote:

> Which bug/issue is this?

https://github.com/llvm/llvm-project/issues/56620


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130259

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

[clang-tools-extra] d9d554a - [pseudo] Add ambiguity & unparseability metrics to -print-statistics

[Utkarsh Saxena via cfe-commits]

Author: Sam McCall
Date: 2022-07-22T10:35:06+02:00
New Revision: d9d554a3f4640e8f1ed5c0ae408740861715b897

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

LOG: [pseudo] Add ambiguity & unparseability metrics to -print-statistics

These can be used to quantify parsing improvements from a change.

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

Added: 


Modified: 
clang-tools-extra/pseudo/test/glr.cpp
clang-tools-extra/pseudo/tool/ClangPseudo.cpp

Removed: 




diff  --git a/clang-tools-extra/pseudo/test/glr.cpp 
b/clang-tools-extra/pseudo/test/glr.cpp
index 24b2ac05f6f1..221725c6f089 100644
--- a/clang-tools-extra/pseudo/test/glr.cpp
+++ b/clang-tools-extra/pseudo/test/glr.cpp
@@ -29,3 +29,6 @@ void foo() {
 // CHECK-NEXT: 1 type-name
 // CHECK-EMPTY:
 // CHECK-NEXT: 0 Opaque nodes:
+// CHECK-EMPTY:
+// CHECK-NEXT: Ambiguity: 0.40 misparses/token
+// CHECK-NEXT: Unparsed: 0.00%

diff  --git a/clang-tools-extra/pseudo/tool/ClangPseudo.cpp 
b/clang-tools-extra/pseudo/tool/ClangPseudo.cpp
index 2a2d8eda4c20..294098a3a5c1 100644
--- a/clang-tools-extra/pseudo/tool/ClangPseudo.cpp
+++ b/clang-tools-extra/pseudo/tool/ClangPseudo.cpp
@@ -24,6 +24,8 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
 
+using clang::pseudo::ForestNode;
+using clang::pseudo::Token;
 using clang::pseudo::TokenStream;
 using llvm::cl::desc;
 using llvm::cl::init;
@@ -174,9 +176,8 @@ int main(int argc, char *argv[]) {
   llvm::outs() << "GSS bytes: " << GSS.bytes()
<< " nodes: " << GSS.nodesCreated() << "\n";
 
-  for (auto &P :
-   {std::make_pair("Ambiguous", clang::pseudo::ForestNode::Ambiguous),
-std::make_pair("Opaque", clang::pseudo::ForestNode::Opaque)}) {
+  for (auto &P : {std::make_pair("Ambiguous", ForestNode::Ambiguous),
+  std::make_pair("Opaque", ForestNode::Opaque)}) {
 clang::pseudo::NodeStats Stats(
 Root, [&](const auto &N) { return N.kind() == P.second; });
 llvm::outs() << "\n" << Stats.Total << " " << P.first << " nodes:\n";
@@ -184,6 +185,39 @@ int main(int argc, char *argv[]) {
   llvm::outs() << llvm::formatv("  {0,3} {1}\n", S.second,
 Lang.G.symbolName(S.first));
   }
+
+  // Metrics for how imprecise parsing was.
+  // These are rough but aim to be:
+  // - linear: if we eliminate half the errors the metric should halve
+  // - length-independent
+  unsigned UnparsedTokens = 0; // Tokens covered by Opaque. (not unique)
+  unsigned Misparses = 0;  // Sum of alternatives-1
+  llvm::DenseSet Visited;
+  auto DFS = [&](const ForestNode &N, Token::Index End, auto &DFS) -> void 
{
+if (N.kind() == ForestNode::Opaque) {
+  UnparsedTokens += End - N.startTokenIndex();
+} else if (N.kind() == ForestNode::Ambiguous) {
+  Misparses += N.alternatives().size() - 1;
+  for (const auto *C : N.alternatives())
+if (Visited.insert(C).second)
+  DFS(*C, End, DFS);
+} else if (N.kind() == ForestNode::Sequence) {
+  for (unsigned I = 0, E = N.children().size(); I < E; ++I)
+if (Visited.insert(N.children()[I]).second)
+  DFS(*N.children()[I],
+  I + 1 == N.children().size()
+  ? End
+  : N.children()[I + 1]->startTokenIndex(),
+  DFS);
+}
+  };
+  unsigned Len = ParseableStream->tokens().size();
+  DFS(Root, Len, DFS);
+  llvm::outs() << "\n";
+  llvm::outs() << llvm::formatv("Ambiguity: {0} misparses/token\n",
+double(Misparses) / Len);
+  llvm::outs() << llvm::formatv("Unparsed: {0}%\n",
+100.0 * UnparsedTokens / Len);
 }
   }
 



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

[PATCH] D130199: [pseudo] Add ambiguity & unparseability metrics to -print-statistics

[Utkarsh Saxena via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd9d554a3f464: [pseudo] Add ambiguity & unparseability 
metrics to -print-statistics (authored by sammccall, committed by usaxena95).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130199

Files:
  clang-tools-extra/pseudo/test/glr.cpp
  clang-tools-extra/pseudo/tool/ClangPseudo.cpp


Index: clang-tools-extra/pseudo/tool/ClangPseudo.cpp
===
--- clang-tools-extra/pseudo/tool/ClangPseudo.cpp
+++ clang-tools-extra/pseudo/tool/ClangPseudo.cpp
@@ -24,6 +24,8 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
 
+using clang::pseudo::ForestNode;
+using clang::pseudo::Token;
 using clang::pseudo::TokenStream;
 using llvm::cl::desc;
 using llvm::cl::init;
@@ -174,9 +176,8 @@
   llvm::outs() << "GSS bytes: " << GSS.bytes()
<< " nodes: " << GSS.nodesCreated() << "\n";
 
-  for (auto &P :
-   {std::make_pair("Ambiguous", clang::pseudo::ForestNode::Ambiguous),
-std::make_pair("Opaque", clang::pseudo::ForestNode::Opaque)}) {
+  for (auto &P : {std::make_pair("Ambiguous", ForestNode::Ambiguous),
+  std::make_pair("Opaque", ForestNode::Opaque)}) {
 clang::pseudo::NodeStats Stats(
 Root, [&](const auto &N) { return N.kind() == P.second; });
 llvm::outs() << "\n" << Stats.Total << " " << P.first << " nodes:\n";
@@ -184,6 +185,39 @@
   llvm::outs() << llvm::formatv("  {0,3} {1}\n", S.second,
 Lang.G.symbolName(S.first));
   }
+
+  // Metrics for how imprecise parsing was.
+  // These are rough but aim to be:
+  // - linear: if we eliminate half the errors the metric should halve
+  // - length-independent
+  unsigned UnparsedTokens = 0; // Tokens covered by Opaque. (not unique)
+  unsigned Misparses = 0;  // Sum of alternatives-1
+  llvm::DenseSet Visited;
+  auto DFS = [&](const ForestNode &N, Token::Index End, auto &DFS) -> void 
{
+if (N.kind() == ForestNode::Opaque) {
+  UnparsedTokens += End - N.startTokenIndex();
+} else if (N.kind() == ForestNode::Ambiguous) {
+  Misparses += N.alternatives().size() - 1;
+  for (const auto *C : N.alternatives())
+if (Visited.insert(C).second)
+  DFS(*C, End, DFS);
+} else if (N.kind() == ForestNode::Sequence) {
+  for (unsigned I = 0, E = N.children().size(); I < E; ++I)
+if (Visited.insert(N.children()[I]).second)
+  DFS(*N.children()[I],
+  I + 1 == N.children().size()
+  ? End
+  : N.children()[I + 1]->startTokenIndex(),
+  DFS);
+}
+  };
+  unsigned Len = ParseableStream->tokens().size();
+  DFS(Root, Len, DFS);
+  llvm::outs() << "\n";
+  llvm::outs() << llvm::formatv("Ambiguity: {0} misparses/token\n",
+double(Misparses) / Len);
+  llvm::outs() << llvm::formatv("Unparsed: {0}%\n",
+100.0 * UnparsedTokens / Len);
 }
   }
 
Index: clang-tools-extra/pseudo/test/glr.cpp
===
--- clang-tools-extra/pseudo/test/glr.cpp
+++ clang-tools-extra/pseudo/test/glr.cpp
@@ -29,3 +29,6 @@
 // CHECK-NEXT: 1 type-name
 // CHECK-EMPTY:
 // CHECK-NEXT: 0 Opaque nodes:
+// CHECK-EMPTY:
+// CHECK-NEXT: Ambiguity: 0.40 misparses/token
+// CHECK-NEXT: Unparsed: 0.00%


Index: clang-tools-extra/pseudo/tool/ClangPseudo.cpp
===
--- clang-tools-extra/pseudo/tool/ClangPseudo.cpp
+++ clang-tools-extra/pseudo/tool/ClangPseudo.cpp
@@ -24,6 +24,8 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
 
+using clang::pseudo::ForestNode;
+using clang::pseudo::Token;
 using clang::pseudo::TokenStream;
 using llvm::cl::desc;
 using llvm::cl::init;
@@ -174,9 +176,8 @@
   llvm::outs() << "GSS bytes: " << GSS.bytes()
<< " nodes: " << GSS.nodesCreated() << "\n";
 
-  for (auto &P :
-   {std::make_pair("Ambiguous", clang::pseudo::ForestNode::Ambiguous),
-std::make_pair("Opaque", clang::pseudo::ForestNode::Opaque)}) {
+  for (auto &P : {std::make_pair("Ambiguous", ForestNode::Ambiguous),
+  std::make_pair("Opaque", ForestNode::Opaque)}) {
 clang::pseudo::NodeStats Stats(
 Root, [&](const auto &N) { return N.kind() == P.second; });
 llvm::outs() << "\n" << Stats.Total << " " << P.first << " nodes:\n";
@@ -184,6 +185,39 @@
   llvm::outs() << llvm::formatv("  {0,3}

[PATCH] D130199: [pseudo] Add ambiguity & unparseability metrics to -print-statistics

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

Landed the patch. Will extract this as a library to share this with other 
continuous evaluation pipeline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130199

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

[PATCH] D130091: [clang][analyzer] Added partial wide character support to CStringChecker

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

Okay, thanks for the update. LGTM!




Comment at: clang/test/Analysis/wstring.c:385
+  wchar_t a[32];
+  // FIXME: This should work with 'w_str' instead of 'w_str1'
+  const wchar_t w_str1[] = L"Hello world";

balazske wrote:
> The problem may be that the global constant is not encountered as statement 
> when a function is analyzed.
Do we have the same problem in the non-wide case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130091

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

[PATCH] D130254: [CMake][Clang] Copy folder without permissions

[Sebastian Neubauer via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf359eac5df06: [CMake][Clang] Copy folder without permissions 
(authored by sebastian-ne).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130254

Files:
  clang/cmake/modules/CMakeLists.txt


Index: clang/cmake/modules/CMakeLists.txt
===
--- clang/cmake/modules/CMakeLists.txt
+++ clang/cmake/modules/CMakeLists.txt
@@ -32,8 +32,10 @@
 
 # For compatibility with projects that include(ClangConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only
 file(COPY .
   DESTINATION ${clang_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )


Index: clang/cmake/modules/CMakeLists.txt
===
--- clang/cmake/modules/CMakeLists.txt
+++ clang/cmake/modules/CMakeLists.txt
@@ -32,8 +32,10 @@
 
 # For compatibility with projects that include(ClangConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only
 file(COPY .
   DESTINATION ${clang_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] f359eac - [CMake][Clang] Copy folder without permissions

[Sebastian Neubauer via cfe-commits]

Author: Sebastian Neubauer
Date: 2022-07-22T10:38:54+02:00
New Revision: f359eac5df06c1062019fad9aac41f4320899c5f

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

LOG: [CMake][Clang] Copy folder without permissions

Copying the folder keeps the original permissions by default. This
creates problems when the source folder is read-only, e.g. in a
packaging environment.
Then, the copied folder in the build directory is read-only as well.
Later on, with configure_file, ClangConfig.cmake is copied into that
directory (in the build tree), failing when the directory is read-only.

Fix that problem by copying the folder without keeping the original
permissions.

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

Added: 


Modified: 
clang/cmake/modules/CMakeLists.txt

Removed: 




diff  --git a/clang/cmake/modules/CMakeLists.txt 
b/clang/cmake/modules/CMakeLists.txt
index c6f6ce9fe5d69..c6afdab40d65b 100644
--- a/clang/cmake/modules/CMakeLists.txt
+++ b/clang/cmake/modules/CMakeLists.txt
@@ -32,8 +32,10 @@ set(CLANG_CONFIG_LLVM_CMAKE_DIR)
 
 # For compatibility with projects that include(ClangConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only
 file(COPY .
   DESTINATION ${clang_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )



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

[PATCH] D130273: [clang][Driver] Handle SPARC -mcpu=native etc.

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

In D130273#3670623 , @MaskRay wrote:

> I notice that in gcc, -march/-mtune/-mcpu are handled in gcc/config/* and 
> every port may have somewhat different behaviors. E.g. x86 and aarch64 are 
> different (and I suspect x86 has the weird behavior).

Right: e.g. `gcc` on SPARC doesn't support `-march` at all.  I've made no 
changes to `clang` here, i.e. the option is silently ignored.  I believe it's 
important to keep that compatibility so `clang` command lines can be passed to 
`gcc` or vice versa.

> Have you checked that whether this matches GCC? We need some 
> clang/test/Driver tests for `"-cc1"{{.*}} "-target-cpu" "..."`

Not in every detail, especially since the `gcc` driver just passes `-mcpu` etc. 
on to `cc1` which handles them itself, so it's not really easy to see what 
happens.

I'll add some tests, though.




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2226
+
+std::string TuneCPU;
+if (Name == "native")

MaskRay wrote:
> ```
> std::string TuneCPU(Name == "native" ? ... : ...)
> if (!TuneCPU.empty()) {
>   ...
> ```
I'm not sure about this: I tried that variant, but I don't really think it's 
clearer than what I have now:
```
std::string TuneCPU(Name == "native"
? std::string(llvm::sys::getHostCPUName()
: std::string(Name)));
```
My code was taken from `AddSystemZTargetArgs` directly below and it would seem 
a bit weird if they differ in style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130273

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

[PATCH] D130254: [CMake][Clang] Copy folder without permissions

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

What about similar code in LLVM 
?




Comment at: clang/cmake/modules/CMakeLists.txt:35
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only
 file(COPY .

[nit] This comment tells me "what" is happening here, but that can be deduced 
from the code (i.e. `NO_SOURCE_PERMISSIONS` --> copy without source 
permissions). Explaining "why" this being read-only is a problem would be more 
helpful, IMHO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130254

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

[clang] 6d9b847 - [C++20] [Modules] Handle reachability for partial specialization

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2022-07-22T17:03:38+08:00
New Revision: 6d9b84797c1c4bd00a2392043e9feea4ecebe482

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

LOG: [C++20] [Modules] Handle reachability for partial specialization

Previously we don't catch the reachability for partial specialization.
Handle them in this patch.

Added: 
clang/test/Modules/partial_specialization.cppm

Modified: 
clang/lib/Sema/SemaTemplate.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 171f005816b5c..95c83ebfaeab5 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4573,7 +4573,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, 
SourceLocation TemplateLoc,
   void *InsertPos = nullptr;
   if (VarTemplateSpecializationDecl *Spec = Template->findSpecialization(
   Converted, InsertPos)) {
-checkSpecializationVisibility(TemplateNameLoc, Spec);
+checkSpecializationReachability(TemplateNameLoc, Spec);
 // If we already have a variable template specialization, return it.
 return Spec;
   }
@@ -4694,7 +4694,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, 
SourceLocation TemplateLoc,
   dyn_cast(InstantiationPattern))
 Decl->setInstantiationOf(D, InstantiationArgs);
 
-  checkSpecializationVisibility(TemplateNameLoc, Decl);
+  checkSpecializationReachability(TemplateNameLoc, Decl);
 
   assert(Decl && "No variable template specialization?");
   return Decl;

diff  --git a/clang/test/Modules/partial_specialization.cppm 
b/clang/test/Modules/partial_specialization.cppm
new file mode 100644
index 0..3a01857172112
--- /dev/null
+++ b/clang/test/Modules/partial_specialization.cppm
@@ -0,0 +1,34 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/A.cppm -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp 
-fsyntax-only -verify
+//
+//--- foo.h
+template
+inline constexpr bool IsSame = false;
+
+template
+inline constexpr bool IsSame = true;
+
+template 
+class A {
+public:
+A();
+~A() noexcept(IsSame);
+};
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::A;
+
+//--- Use.cpp
+import A;
+void bool_consume(bool b);
+void use() {
+A a{};
+bool_consume(IsSame); // expected-error {{use of undeclared identifier 
'IsSame'}}
+}



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

[PATCH] D130259: [clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions

[Tobias Ribizel via Phabricator via cfe-commits]

upsj abandoned this revision.
upsj added a comment.

Superseded by https://reviews.llvm.org/D130260


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130259

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

[clang] 35b80c4 - Don't write to source directory in test

[Benjamin Kramer via cfe-commits]

Author: Benjamin Kramer
Date: 2022-07-22T11:14:26+02:00
New Revision: 35b80c448bbc056a7060201c9f62eec9115e3c31

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

LOG: Don't write to source directory in test

Added: 


Modified: 
clang/test/CodeGen/module-intializer-pmf.cpp

Removed: 




diff  --git a/clang/test/CodeGen/module-intializer-pmf.cpp 
b/clang/test/CodeGen/module-intializer-pmf.cpp
index e513b280b0a7..7ab4a2e2bd78 100644
--- a/clang/test/CodeGen/module-intializer-pmf.cpp
+++ b/clang/test/CodeGen/module-intializer-pmf.cpp
@@ -1,7 +1,7 @@
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %s \
-// RUN:-emit-module-interface -o HasPMF.pcm
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 HasPMF.pcm \
+// RUN:-emit-module-interface -o %T/HasPMF.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %T/HasPMF.pcm \
 // RUN:  -S -emit-llvm -o - | FileCheck %s
 
 module;



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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added a comment.

Your approach to handling implicit conversions is much nicer than mine, so I 
abandoned my revision. There is still one case (that might even occur in the 
code base I'm working on?) that this change would lead to incorrect hints on. 
WDYT about keeping the other changes, but using my pack detection logic?




Comment at: clang-tools-extra/clangd/AST.cpp:790
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;

This is not a sufficient check, since the other parameters need not be from an 
expanded pack.
Example:
```
void foo(int a, int b);
int baz(int x, int y);
template 
void bar(Args... args) {
  foo(baz(args, 1)...);
}

void foo() {
  bar(1, 42);
}
```
Here we shouldn't print a hint at all, but we print `x:` and `y:`
The important distinction is between `Expr(args...)` and `Expr(args)...`, which 
can be decided in the instantiated case by the check I implemented in 
https://reviews.llvm.org/D130259 for all cases, except when `args` only 
consists of a single element.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: usaxena95, kadircet.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, alextsao1999, ilya-biryukov.
Herald added a project: clang-tools-extra.

Motivating case: `foo bar;` is not a declaration of nothing with `foo` and `bar`
both types.

This is a common and critical ambiguity, clangd/AST.cpp has 20% fewer
ambiguous nodes (1674->1332) after this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130337

Files:
  clang-tools-extra/pseudo/lib/cxx/CXX.cpp
  clang-tools-extra/pseudo/lib/cxx/cxx.bnf
  clang-tools-extra/pseudo/test/cxx/decl-specfier-seq.cpp
  clang-tools-extra/pseudo/test/fuzzer.cpp

Index: clang-tools-extra/pseudo/test/fuzzer.cpp
===
--- clang-tools-extra/pseudo/test/fuzzer.cpp
+++ clang-tools-extra/pseudo/test/fuzzer.cpp
@@ -1,4 +1,4 @@
 // RUN: clang-pseudo-fuzzer -grammar=%cxx-bnf-file -print %s | FileCheck %s
 int x;
 // CHECK: translation-unit := declaration-seq
-// CHECK: simple-type-specifier := INT
+// CHECK: builtin-type := INT
Index: clang-tools-extra/pseudo/test/cxx/decl-specfier-seq.cpp
===
--- /dev/null
+++ clang-tools-extra/pseudo/test/cxx/decl-specfier-seq.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-pseudo -grammar=cxx -source=%s --print-forest | FileCheck %s
+
+// not parsed as Type{foo} Type{bar}
+foo bar;
+// CHECK-NOT: simple-declaration := decl-specifier-seq ;
+// CHECK:simple-declaration := decl-specifier-seq init-declarator-list ;
+// CHECK:├─decl-specifier-seq~simple-type-specifier
+// CHECK:├─init-declarator-list~IDENTIFIER
+// CHECK:└─;
+// CHECK-NOT: simple-declaration := decl-specifier-seq ;
+
+// not parsed as Type{std} Type{::string} Declarator{s};
+std::string s;
+// CHECK-NOT: nested-name-specifier := ::
+// CHECK: simple-declaration := decl-specifier-seq init-declarator-list ;
+// CHECK: ├─decl-specifier-seq~simple-type-specifier := 
+// CHECK: │ ├─simple-type-specifier := nested-name-specifier type-name
+// CHECK: │ │ ├─nested-name-specifier :=  #1
+// CHECK: │ │ │ ├─nested-name-specifier := type-name ::
+// CHECK: │ │ │ └─nested-name-specifier := namespace-name ::
+// CHECK: │ │ └─type-name
+// CHECK: │ └─simple-type-specifier := nested-name-specifier template-name
+// CHECK: │   ├─nested-name-specifier =#1
+// CHECK: │   └─template-name~IDENTIFIER
+// CHECK: ├─init-declarator-list~IDENTIFIER
+// CHECK: └─;
+// CHECK-NOT: nested-name-specifier := ::
Index: clang-tools-extra/pseudo/lib/cxx/cxx.bnf
===
--- clang-tools-extra/pseudo/lib/cxx/cxx.bnf
+++ clang-tools-extra/pseudo/lib/cxx/cxx.bnf
@@ -350,7 +350,7 @@
 decl-specifier := CONSTINIT
 decl-specifier := INLINE
 decl-specifier-seq := decl-specifier
-decl-specifier-seq := decl-specifier decl-specifier-seq
+decl-specifier-seq := decl-specifier decl-specifier-seq [guard]
 storage-class-specifier := STATIC
 storage-class-specifier := THREAD_LOCAL
 storage-class-specifier := EXTERN
@@ -364,31 +364,32 @@
 type-specifier := typename-specifier
 type-specifier := cv-qualifier
 type-specifier-seq := type-specifier
-type-specifier-seq := type-specifier type-specifier-seq
+type-specifier-seq := type-specifier type-specifier-seq [guard]
 defining-type-specifier := type-specifier
 defining-type-specifier := class-specifier
 defining-type-specifier := enum-specifier
 defining-type-specifier-seq := defining-type-specifier
-defining-type-specifier-seq := defining-type-specifier defining-type-specifier-seq
+defining-type-specifier-seq := defining-type-specifier defining-type-specifier-seq [guard]
 simple-type-specifier := nested-name-specifier_opt type-name
 simple-type-specifier := nested-name-specifier TEMPLATE simple-template-id
 simple-type-specifier := decltype-specifier
 simple-type-specifier := placeholder-type-specifier
 simple-type-specifier := nested-name-specifier_opt template-name
-simple-type-specifier := CHAR
-simple-type-specifier := CHAR8_T
-simple-type-specifier := CHAR16_T
-simple-type-specifier := CHAR32_T
-simple-type-specifier := WCHAR_T
-simple-type-specifier := BOOL
+simple-type-specifier := builtin-type
+builtin-type := CHAR
+builtin-type := CHAR8_T
+builtin-type := CHAR16_T
+builtin-type := CHAR32_T
+builtin-type := WCHAR_T
+builtin-type := BOOL
 simple-type-specifier := SHORT
-simple-type-specifier := INT
+builtin-type := INT
 simple-type-specifier := LONG
 simple-type-specifier := SIGNED
 simple-type-specifier := UNSIGNED
-simple-type-specifier := FLOAT
-simple-type-specifier := DOUBLE
-simple-type-specifier := VOID
+builtin-type := FLOAT
+builtin-type := DOUBLE
+builtin-type := VOID
 type-name :=

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Implementation choices here:

- we walk through the whole tree everytime we see decl-specifier-seq: this is 
dumb but we to reuse work we either need to add extra rules to the grammar 
(D130150 ) or an explicit cache. Caching is 
likely a good idea but not added in this patch.
- we handle *all* rules for the interesting node types explicitly, rather than 
`default: return false`. This allows us to assert that all cases are handled, 
so things don't "fall through the cracks" after grammar changes. Alternative 
suggestions welcome. (I have a feeling this "exhaustive pattern-matching" idea 
will come up again...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[clang] 251b5b8 - [ASTMatchers] Fix standalone build

[Nathan James via cfe-commits]

Author: Nathan James
Date: 2022-07-22T10:32:49+01:00
New Revision: 251b5b864183e868ffc86522e320f91ab3c5a771

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

LOG: [ASTMatchers] Fix standalone build

Disable the tests and remove private include introduced in 
d89f9e963e4979466193dc6a15fe091bf7ca5c47.

Added: 


Modified: 
clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Removed: 




diff  --git a/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp 
b/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
index eb9071bd9cb6d..c123f71dc5278 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
@@ -12,7 +12,6 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/Config/config.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
@@ -36,6 +35,8 @@ TEST(HasNameDeathTest, DiesOnEmptyPattern) {
 }, "");
 }
 
+// FIXME Re-enable these tests without breaking standalone builds.
+#if 0
 // FIXME: Figure out why back traces aren't being generated on clang builds on
 // windows.
 #if ENABLE_BACKTRACES && (!defined(_MSC_VER) || !defined(__clang__))
@@ -138,6 +139,7 @@ TEST(MatcherCrashDeathTest, CrashOnCallbackDump) {
 }
 #endif // ENABLE_BACKTRACES
 #endif
+#endif
 
 TEST(ConstructVariadic, MismatchedTypes_Regression) {
   EXPECT_TRUE(



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

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 446753.
sammccall added a comment.

use all_of


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

Files:
  clang-tools-extra/pseudo/lib/cxx/CXX.cpp
  clang-tools-extra/pseudo/lib/cxx/cxx.bnf
  clang-tools-extra/pseudo/test/cxx/decl-specfier-seq.cpp
  clang-tools-extra/pseudo/test/fuzzer.cpp

Index: clang-tools-extra/pseudo/test/fuzzer.cpp
===
--- clang-tools-extra/pseudo/test/fuzzer.cpp
+++ clang-tools-extra/pseudo/test/fuzzer.cpp
@@ -1,4 +1,4 @@
 // RUN: clang-pseudo-fuzzer -grammar=%cxx-bnf-file -print %s | FileCheck %s
 int x;
 // CHECK: translation-unit := declaration-seq
-// CHECK: simple-type-specifier := INT
+// CHECK: builtin-type := INT
Index: clang-tools-extra/pseudo/test/cxx/decl-specfier-seq.cpp
===
--- /dev/null
+++ clang-tools-extra/pseudo/test/cxx/decl-specfier-seq.cpp
@@ -0,0 +1,27 @@
+// RUN: clang-pseudo -grammar=cxx -source=%s --print-forest | FileCheck %s
+
+// not parsed as Type{foo} Type{bar}
+foo bar;
+// CHECK-NOT: simple-declaration := decl-specifier-seq ;
+// CHECK:simple-declaration := decl-specifier-seq init-declarator-list ;
+// CHECK:├─decl-specifier-seq~simple-type-specifier
+// CHECK:├─init-declarator-list~IDENTIFIER
+// CHECK:└─;
+// CHECK-NOT: simple-declaration := decl-specifier-seq ;
+
+// not parsed as Type{std} Type{::string} Declarator{s};
+std::string s;
+// CHECK-NOT: nested-name-specifier := ::
+// CHECK: simple-declaration := decl-specifier-seq init-declarator-list ;
+// CHECK: ├─decl-specifier-seq~simple-type-specifier := 
+// CHECK: │ ├─simple-type-specifier := nested-name-specifier type-name
+// CHECK: │ │ ├─nested-name-specifier :=  #1
+// CHECK: │ │ │ ├─nested-name-specifier := type-name ::
+// CHECK: │ │ │ └─nested-name-specifier := namespace-name ::
+// CHECK: │ │ └─type-name
+// CHECK: │ └─simple-type-specifier := nested-name-specifier template-name
+// CHECK: │   ├─nested-name-specifier =#1
+// CHECK: │   └─template-name~IDENTIFIER
+// CHECK: ├─init-declarator-list~IDENTIFIER
+// CHECK: └─;
+// CHECK-NOT: nested-name-specifier := ::
Index: clang-tools-extra/pseudo/lib/cxx/cxx.bnf
===
--- clang-tools-extra/pseudo/lib/cxx/cxx.bnf
+++ clang-tools-extra/pseudo/lib/cxx/cxx.bnf
@@ -350,7 +350,7 @@
 decl-specifier := CONSTINIT
 decl-specifier := INLINE
 decl-specifier-seq := decl-specifier
-decl-specifier-seq := decl-specifier decl-specifier-seq
+decl-specifier-seq := decl-specifier decl-specifier-seq [guard]
 storage-class-specifier := STATIC
 storage-class-specifier := THREAD_LOCAL
 storage-class-specifier := EXTERN
@@ -364,31 +364,32 @@
 type-specifier := typename-specifier
 type-specifier := cv-qualifier
 type-specifier-seq := type-specifier
-type-specifier-seq := type-specifier type-specifier-seq
+type-specifier-seq := type-specifier type-specifier-seq [guard]
 defining-type-specifier := type-specifier
 defining-type-specifier := class-specifier
 defining-type-specifier := enum-specifier
 defining-type-specifier-seq := defining-type-specifier
-defining-type-specifier-seq := defining-type-specifier defining-type-specifier-seq
+defining-type-specifier-seq := defining-type-specifier defining-type-specifier-seq [guard]
 simple-type-specifier := nested-name-specifier_opt type-name
 simple-type-specifier := nested-name-specifier TEMPLATE simple-template-id
 simple-type-specifier := decltype-specifier
 simple-type-specifier := placeholder-type-specifier
 simple-type-specifier := nested-name-specifier_opt template-name
-simple-type-specifier := CHAR
-simple-type-specifier := CHAR8_T
-simple-type-specifier := CHAR16_T
-simple-type-specifier := CHAR32_T
-simple-type-specifier := WCHAR_T
-simple-type-specifier := BOOL
+simple-type-specifier := builtin-type
+builtin-type := CHAR
+builtin-type := CHAR8_T
+builtin-type := CHAR16_T
+builtin-type := CHAR32_T
+builtin-type := WCHAR_T
+builtin-type := BOOL
 simple-type-specifier := SHORT
-simple-type-specifier := INT
+builtin-type := INT
 simple-type-specifier := LONG
 simple-type-specifier := SIGNED
 simple-type-specifier := UNSIGNED
-simple-type-specifier := FLOAT
-simple-type-specifier := DOUBLE
-simple-type-specifier := VOID
+builtin-type := FLOAT
+builtin-type := DOUBLE
+builtin-type := VOID
 type-name := class-name
 type-name := enum-name
 type-name := typedef-name
Index: clang-tools-extra/pseudo/lib/cxx/CXX.cpp
===
--- clang-tools-extra/pseudo/lib/cxx/CXX.cpp
+++ clang-tools-extra/pseudo/lib/cxx/CXX.cpp
@@ -14,7 +14,9 @@
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/TokenKinds.h"
 

[PATCH] D130338: [CMake] Copy folder without permissions

[Sebastian Neubauer via Phabricator via cfe-commits]

sebastian-ne created this revision.
sebastian-ne added reviewers: awarzynski, Ericson2314, tstellar.
Herald added subscribers: bzcheeseman, sdasgup3, wenzhicui, wrengr, cota, 
teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, 
Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, antiagainst, shauheen, 
rriddle, mehdi_amini, mgorny.
Herald added a project: All.
sebastian-ne requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, stephenneuendorffer, 
nicolasvasilache.
Herald added projects: clang, MLIR, LLVM.

Copying the folder keeps the original permissions by default. This
creates problems when the source folder is read-only, e.g. in a
packaging environment.
Then, the copied folder in the build directory is read-only as well.
Later on, other files are copied into that directory (in the build
tree), failing when the directory is read-only.

Fix that problem by copying the folder without keeping the original
permissions.

Follow-up to D130254 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130338

Files:
  clang/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/CMakeLists.txt
  mlir/cmake/modules/CMakeLists.txt


Index: mlir/cmake/modules/CMakeLists.txt
===
--- mlir/cmake/modules/CMakeLists.txt
+++ mlir/cmake/modules/CMakeLists.txt
@@ -42,9 +42,12 @@
 
 # For compatibility with projects that include(MLIRConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only,
+# but we need to write into the copied folder.
 # This should be removed in the future.
 file(COPY .
   DESTINATION ${mlir_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )
Index: llvm/cmake/modules/CMakeLists.txt
===
--- llvm/cmake/modules/CMakeLists.txt
+++ llvm/cmake/modules/CMakeLists.txt
@@ -99,9 +99,12 @@
 
 # For compatibility with projects that include(LLVMConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only,
+# but we need to write into the copied folder.
 # This should be removed in the future.
 file(COPY .
   DESTINATION ${llvm_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )
Index: clang/cmake/modules/CMakeLists.txt
===
--- clang/cmake/modules/CMakeLists.txt
+++ clang/cmake/modules/CMakeLists.txt
@@ -32,7 +32,8 @@
 
 # For compatibility with projects that include(ClangConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
-# Copy without source permissions because the source could be read-only
+# Copy without source permissions because the source could be read-only,
+# but we need to write into the copied folder.
 file(COPY .
   DESTINATION ${clang_cmake_builddir}
   NO_SOURCE_PERMISSIONS


Index: mlir/cmake/modules/CMakeLists.txt
===
--- mlir/cmake/modules/CMakeLists.txt
+++ mlir/cmake/modules/CMakeLists.txt
@@ -42,9 +42,12 @@
 
 # For compatibility with projects that include(MLIRConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only,
+# but we need to write into the copied folder.
 # This should be removed in the future.
 file(COPY .
   DESTINATION ${mlir_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )
Index: llvm/cmake/modules/CMakeLists.txt
===
--- llvm/cmake/modules/CMakeLists.txt
+++ llvm/cmake/modules/CMakeLists.txt
@@ -99,9 +99,12 @@
 
 # For compatibility with projects that include(LLVMConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only,
+# but we need to write into the copied folder.
 # This should be removed in the future.
 file(COPY .
   DESTINATION ${llvm_cmake_builddir}
+  NO_SOURCE_PERMISSIONS
   FILES_MATCHING PATTERN *.cmake
   PATTERN CMakeFiles EXCLUDE
   )
Index: clang/cmake/modules/CMakeLists.txt
===
--- clang/cmake/modules/CMakeLists.txt
+++ clang/cmake/modules/CMakeLists.txt
@@ -32,7 +32,8 @@
 
 # For compatibility with projects that include(ClangConfig)
 # via CMAKE_MODULE_PATH, place API modules next to it.
-# Copy without source permissions because the source could be read-only
+# Copy without source permissions because the source could be read-only,
+# but we need to write into the copied folder.
 file(COPY .
   DESTINATION ${clang_cmake_builddir}
   NO_SOURCE_PERMISSIONS

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

[Nathan James via Phabricator via cfe-commits]

njames93 closed this revision.
njames93 added a comment.

In D120185#3668029 , @mgorny wrote:

> This change broke standalone build of clang. Please fix.

I've pushed rG251b5b864183e868ffc86522e320f91ab3c5a771 
 which 
should fix the build, but need to look into a way to re-enable the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120185

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

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

FWIW, as-is with no caching, this is a ~2% slowdown on my machine (5.82 -> 5.72 
MB/s on SemaCodeComplete.cpp).
Whereas D130150  using the grammar is a a 7% 
speedup (5.82 -> 6.22), so roughly an 9% performance difference between the 
approaches.
My guess is we'll get some but not all of this back through caching, as hashing 
isn't free and we'll increase the size of our working set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 446759.
kadircet added a comment.

- Check to ensure function call receives all the arguments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,50 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+  R"cpp(
+void foo(int x, int y);
+int id(int a, int b);
+template 
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]])...);
+}
+void foo() {
+  bar(1, 2);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -786,6 +786,9 @@
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -823,10 +826,14 @@
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
 for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+  if (const auto *RefArg = unwrapForward(*It)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
 return std::distance(Args.begin(), It);
   }
 }
@@ -872,6 +879,13 @@
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark that in the AST as so, this might skip explicit calls
+// too.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforward) {


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,50 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 446763.
kadircet added a comment.

- Add OOB check as an asssertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,50 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+  R"cpp(
+void foo(int x, int y);
+int id(int a, int b);
+template 
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]])...);
+}
+void foo() {
+  bar(1, 2);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -786,6 +786,9 @@
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -823,10 +826,18 @@
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
 for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+  if (const auto *RefArg = unwrapForward(*It)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;
+assert(std::distance(Args.begin(), ParamEnd) <
+   std::distance(Args.begin(), Args.end()) &&
+   "Only functions with greater than or equal number of parameters "
+   "should be checked.");
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
 return std::distance(Args.begin(), It);
   }
 }
@@ -872,6 +883,13 @@
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark that in the AST as so, this might skip explicit calls
+// too.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforward) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:790
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;

upsj wrote:
> This is not a sufficient check, since the other parameters need not be from 
> an expanded pack.
> Example:
> ```
> void foo(int a, int b);
> int baz(int x, int y);
> template 
> void bar(Args... args) {
>   foo(baz(args, 1)...);
> }
> 
> void foo() {
>   bar(1, 42);
> }
> ```
> Here we shouldn't print a hint at all, but we print `x:` and `y:`
> The important distinction is between `Expr(args...)` and `Expr(args)...`, 
> which can be decided in the instantiated case by the check I implemented in 
> https://reviews.llvm.org/D130259 for all cases, except when `args` only 
> consists of a single element.
yeah that makes sense, added an extra check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Tobias Ribizel via Phabricator via cfe-commits]

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

Not sure I qualify as a suitable reviewer, but this looks really good to me, 
save for maybe a small safety measure :)




Comment at: clang-tools-extra/clangd/AST.cpp:833-837
+auto ParamEnd = It + Parameters.size() - 1;
+assert(std::distance(Args.begin(), ParamEnd) <
+   std::distance(Args.begin(), Args.end()) &&
+   "Only functions with greater than or equal number of parameters 
"
+   "should be checked.");

I think it would be safer to check this explicitly, since advancing the 
iterator past its end might be UB (depending on the implementation).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 446769.
kadircet marked an inline comment as done.
kadircet added a comment.

- Rather than asserting limit the traversal
- Have more comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,50 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+  R"cpp(
+void foo(int x, int y);
+int id(int a, int b);
+template 
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]])...);
+}
+void foo() {
+  bar(1, 2);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -786,6 +786,9 @@
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -822,11 +825,22 @@
   // in the given arguments, if it is there.
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
-for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+auto LastPosibleArg = Args.end();
+for (size_t I = 1; I < Parameters.size(); ++I) {
+  --LastPosibleArg;
+  assert(LastPosibleArg != Args.begin());
+}
+for (auto It = Args.begin(); It != LastPosibleArg; ++It) {
+  if (const auto *RefArg = unwrapForward(*It)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
+// Check that this expands all the way until the last parameter.
+// It's enough to look at the last parameter, because it isn't possible
+// to expand without expanding all of them.
+auto ParamEnd = It + Parameters.size() - 1;
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
 return std::distance(Args.begin(), It);
   }
 }
@@ -872,6 +886,13 @@
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark that in the AST as so, this might skip explicit calls
+// too.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforward) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:833
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;
+assert(std::distance(Args.begin(), ParamEnd) <

upsj wrote:
> I think it would be safer to check this explicitly, since advancing the 
> iterator past its end might be UB (depending on the implementation).
This could crash in `foo(1, args...)`, right?



Comment at: clang-tools-extra/clangd/AST.cpp:879
+// forwarded arg.
+// FIXME: Maybe mark that in the AST as so, this might skip explicit calls
+// too.

NIT: should fit in a single line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 446770.
kadircet added a comment.

- Update test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,51 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+  R"cpp(
+void foo(int x, int y);
+int id(int a, int b, int c);
+template 
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]], $param3[[args]])...);
+}
+void foo() {
+  bar(1, 2);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"c: ", "param3"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -786,6 +786,9 @@
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -822,11 +825,22 @@
   // in the given arguments, if it is there.
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
-for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+auto LastPosibleArg = Args.end();
+for (size_t I = 1; I < Parameters.size(); ++I) {
+  --LastPosibleArg;
+  assert(LastPosibleArg != Args.begin());
+}
+for (auto It = Args.begin(); It != LastPosibleArg; ++It) {
+  if (const auto *RefArg = unwrapForward(*It)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
+// Check that this expands all the way until the last parameter.
+// It's enough to look at the last parameter, because it isn't possible
+// to expand without expanding all of them.
+auto ParamEnd = It + Parameters.size() - 1;
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
 return std::distance(Args.begin(), It);
   }
 }
@@ -872,6 +886,13 @@
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark that in the AST as so, this might skip explicit calls
+// too.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforward) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely 
correct, that may still be worth landing but warrants a fixme)




Comment at: clang-tools-extra/clangd/AST.cpp:832
   continue;
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;

The essence of this test seems to be "if we can't rule it out by counting 
parameters, it must be right" which... doesn't seem very robust? Am I missing 
something?

AIUI the idea here is that
A) `foo(pack...)` is "expanded all the way" into consecutive arguments
B) `foo(pack)...` is not considered forwarding for our purposes
C) we want to treat `foo(bar(pack)...)` specially if `bar` is "just a cast" 
like move or forward

The difference between cases A and B aren't about counting arguments though, 
they're about what operations occur intercede between `pack` and the `...` 
operator. (i.e. `foo((&pack)...)` is fine for the same reason `forward` is.

This seems like something best handled by:
 - finding the `PackExpansionExpr` controlling `pack` in the non-instantiated 
template AST
 - walking down to try to find `pack`
 - continue as we walk through e.g. parens or std::forward
 - bail out if we walk through something unknown (e.g. another function call)

Does this seem sensible/feasible?
(Walking up from `pack` is nicer but the AST doesn't support walking up well...)



Comment at: clang-tools-extra/clangd/AST.cpp:833-837
+auto ParamEnd = It + Parameters.size() - 1;
+assert(std::distance(Args.begin(), ParamEnd) <
+   std::distance(Args.begin(), Args.end()) &&
+   "Only functions with greater than or equal number of parameters 
"
+   "should be checked.");

upsj wrote:
> I think it would be safer to check this explicitly, since advancing the 
> iterator past its end might be UB (depending on the implementation).
Right, this is definitely UB as written.

FWIW I find this easier to understand in classic bounds-check form:

if ((It - Args.begin()) + Parameters.size() >= Args.size())



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1468
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]])...);
+}

(Just so I understand: these are getting hinted due to the "only instantiation" 
logic, right?)



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1471
+void foo() {
+  bar(1, 2);
+}

could reasonably expect this to be `bar(a:1, 2)` or `bar(a:1, a:2)` - leave a 
comment explaining why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130268: [WIP] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Dawid Jurczak via Phabricator via cfe-commits]

yurai007 updated this revision to Diff 446773.
yurai007 edited the summary of this revision.
yurai007 added a comment.

Update constructor definition with explicit to avoid implicit conversions and 
remove ambiguity workarounds.
Adjust constructor users and add unit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

Files:
  llvm/include/llvm/ADT/SmallVector.h
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/lib/Target/Hexagon/HexagonISelDAGToDAGHVX.cpp
  llvm/lib/Target/PowerPC/PPCISelLowering.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86MCInstLower.cpp
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
  llvm/unittests/ADT/SmallVectorTest.cpp

Index: llvm/unittests/ADT/SmallVectorTest.cpp
===
--- llvm/unittests/ADT/SmallVectorTest.cpp
+++ llvm/unittests/ADT/SmallVectorTest.cpp
@@ -242,6 +242,16 @@
   this->assertValuesInOrder(this->theVector, 3u, 1, 2, 3);
 }
 
+// Constructor test.
+TYPED_TEST(SmallVectorTest, ConstructorFromArrayRefSimpleTest) {
+  SCOPED_TRACE("ConstructorTest");
+  ArrayRef Array = {Constructable(1), Constructable(2),
+   Constructable(3)};
+  this->theVector = SmallVector(Array);
+  this->assertValuesInOrder(this->theVector, 3u, 1, 2, 3);
+  ASSERT_EQ(NumBuiltinElts(TypeParam{}), NumBuiltinElts(this->theVector));
+}
+
 // New vector test.
 TYPED_TEST(SmallVectorTest, EmptyVectorTest) {
   SCOPED_TRACE("EmptyVectorTest");
@@ -1157,26 +1167,24 @@
   }
 }
 
-TEST(SmallVectorTest, ToVectorOf) {
-  struct Metadata {
-int content;
-bool operator==(const Metadata &RHS) const {
-  return content == RHS.content;
-}
-  };
+struct Metadata {
+  int content;
+  bool operator==(const Metadata &RHS) const { return content == RHS.content; }
+};
 
-  class MDOperand {
-  public:
-MDOperand() = default;
-MDOperand(Metadata *M) { MD = M; }
-operator Metadata *() const { return get(); }
-Metadata &operator*() const { return *get(); }
+class MDOperand {
+public:
+  MDOperand() = default;
+  MDOperand(Metadata *M) { MD = M; }
+  operator Metadata *() const { return get(); }
+  Metadata &operator*() const { return *get(); }
 
-  private:
-Metadata *get() const { return MD; }
-Metadata *MD = nullptr;
-  };
+private:
+  Metadata *get() const { return MD; }
+  Metadata *MD = nullptr;
+};
 
+TEST(SmallVectorTest, ToVectorOf) {
   Metadata m1{1}, m2{2}, m3{3};
   std::vector Operands = {MDOperand(&m1), MDOperand(&m2),
  MDOperand(&m3)};
@@ -1224,6 +1232,32 @@
   }
 }
 
+TEST(SmallVectorTest, ConstructFromArrayRefOfConvertibleType) {
+  Metadata m1{1}, m2{2}, m3{3};
+  std::vector Operands = {MDOperand(&m1), MDOperand(&m2),
+ MDOperand(&m3)};
+  ArrayRef ArrayOperands = Operands;
+  {
+llvm::SmallVector MetadataVector(ArrayOperands);
+
+ASSERT_EQ(ArrayOperands.size(), MetadataVector.size());
+for (size_t I = 0; I < ArrayOperands.size(); ++I) {
+  EXPECT_EQ(ArrayOperands[I], MetadataVector[I]);
+  EXPECT_EQ(*ArrayOperands[I], *MetadataVector[I]);
+}
+  }
+  {
+llvm::SmallVector MetadataVector(ArrayOperands);
+
+ASSERT_EQ(ArrayOperands.size(), MetadataVector.size());
+ASSERT_EQ(4u, NumBuiltinElts(MetadataVector));
+for (size_t I = 0; I < ArrayOperands.size(); ++I) {
+  EXPECT_EQ(ArrayOperands[I], MetadataVector[I]);
+  EXPECT_EQ(*ArrayOperands[I], *MetadataVector[I]);
+}
+  }
+}
+
 template 
 class SmallVectorReferenceInvalidationTest : public SmallVectorTestBase {
 protected:
Index: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
===
--- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -7653,7 +7653,7 @@
 
   /// Functions adds masks, merging them into  single one.
   void addMask(ArrayRef SubMask) {
-SmallVector NewMask(SubMask.begin(), SubMask.end());
+SmallVector NewMask(SubMask);
 addMask(NewMask);
   }
 
@@ -8749,7 +8749,7 @@
   return PoisonValue::get(FixedVectorType::get(
   cast(V1->getType())->getElementType(), Mask.size()));
 Value *Op = V1;
-SmallVector CombinedMask(Mask.begin(), Mask.end());
+SmallVector CombinedMask(Mask);
 PeekThroughShuffles(Op, CombinedMask);
 if (!isa(Op->getType()) ||
 !IsIdentityMask(CombinedMask, cast(Op->getType( {
Index: llvm/lib/Target/X86/X86MCInstLower.cpp
===
--- llvm/lib/Target/X86/X86MCInstLower.cpp
+++

[PATCH] D130268: [WIP] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Dawid Jurczak via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:504
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
-  CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);
+  CGM.getLLVMContext(), Packed ? PackedElems : to_vector(UnpackedElems),
+  Packed);

yurai007 wrote:
> barannikov88 wrote:
> > barannikov88 wrote:
> > > nikic wrote:
> > > > barannikov88 wrote:
> > > > > yurai007 wrote:
> > > > > > nikic wrote:
> > > > > > > yurai007 wrote:
> > > > > > > > That's because of "error: conditional expression is ambiguous; 
> > > > > > > > 'llvm::SmallVector' can be converted to 
> > > > > > > > 'ArrayRef' and vice versa". Need to check if 
> > > > > > > > there is easier workaround.
> > > > > > > Would making the ctor explicit help?
> > > > > > Nope :( Making constructor explicit disables implicit conversion so 
> > > > > > we cannot do things like: SmallVector NewMask = Mask; 
> > > > > > anymore.
> > > > > And leaving it implicit hides the fact of possible memory allocation, 
> > > > > which is not cheap. I think absense of such constructor is by-design.
> > > > > 
> > > > > Making it explicit is making it redundant, because there is already a 
> > > > > constructor which accepts begin / end, and one that accepts range 
> > > > > (note that it is explicit!). It won't save on typing, either.
> > > > > 
> > > > > I'm not in favor of this patch, but my word does not count much, so I 
> > > > > won't block it. I'd suggest you, however, to request review of core 
> > > > > maintainers.
> > > > > Nope :( Making constructor explicit disables implicit conversion so 
> > > > > we cannot do things like: SmallVector NewMask = Mask; 
> > > > > anymore.
> > > > 
> > > > I think that's fine. Similar to the existing iterator_range 
> > > > constructor, we would require `SmallVector NewMask(Mask)`, 
> > > > which seems like the idiomatic way to write it anyway?
> > > > 
> > > > > Making it explicit is making it redundant, because there is already a 
> > > > > constructor which accepts begin / end, and one that accepts range 
> > > > > (note that it is explicit!). It won't save on typing, either.
> > > > 
> > > > It is not redundant. It ensures that iterator_range and ArrayRef can be 
> > > > freely substituted. Switching iterator_range to ArrayRef currently 
> > > > requires going through a lot of SmallVector constructors and replacing 
> > > > them with less readable code. The alternative to this change is 
> > > > D129988, which looks like a significant regression in code quality.
> > > Please also consider the fact that this is API breaking change due to 
> > > this "conditional expression is amiguous" error. Many external projects 
> > > depend on LLVM/ADT, and all of them will have to adapt this change.
> > > It ensures that iterator_range and ArrayRef can be freely substituted. 
> > > Switching iterator_range to ArrayRef currently requires going through a 
> > > lot of SmallVector constructors and replacing them with less readable 
> > > code. 
> > 
> > Ok, makes sense.
> > Nope :( Making constructor explicit disables implicit conversion so we 
> > cannot do things like: SmallVector NewMask = Mask; anymore.
> > 
> > I think that's fine. Similar to the existing iterator_range constructor, we 
> > would require SmallVector NewMask(Mask), which seems like the 
> > idiomatic way to write it anyway?
> 
> You are right, explicit works when we use brackets instead assignment. I'm 
> gonna add it to constructor and adjust rest of patch.
@barannikov88: after addressing @nikic comment there is no API breaking change 
anymore. As far I can tell SmallVector API is only extended and still 
backward-compatible. Hope it looks better now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Tobias Ribizel via Phabricator via cfe-commits]

upsj added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:832
   continue;
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;

sammccall wrote:
> The essence of this test seems to be "if we can't rule it out by counting 
> parameters, it must be right" which... doesn't seem very robust? Am I missing 
> something?
> 
> AIUI the idea here is that
> A) `foo(pack...)` is "expanded all the way" into consecutive arguments
> B) `foo(pack)...` is not considered forwarding for our purposes
> C) we want to treat `foo(bar(pack)...)` specially if `bar` is "just a cast" 
> like move or forward
> 
> The difference between cases A and B aren't about counting arguments though, 
> they're about what operations occur intercede between `pack` and the `...` 
> operator. (i.e. `foo((&pack)...)` is fine for the same reason `forward` is.
> 
> This seems like something best handled by:
>  - finding the `PackExpansionExpr` controlling `pack` in the non-instantiated 
> template AST
>  - walking down to try to find `pack`
>  - continue as we walk through e.g. parens or std::forward
>  - bail out if we walk through something unknown (e.g. another function call)
> 
> Does this seem sensible/feasible?
> (Walking up from `pack` is nicer but the AST doesn't support walking up 
> well...)
This goes a bit more generally into the question how to approach parameter 
packs across clangd. For inlay hints and hover, we are already looking at 
instantiated templates (is that true in all cases? Haven't dug in yet), so 
implementing forwarding there was straightforward. For signature help and code 
completion, we'd probably need to go via the uninstantiated template (and 
probably lose a bit of type information on the way, e.g. reference vs. value, 
l-value vs. r-value). If we wanted a generic way to deal with forwarding, the 
instantiation pattern would be my first choice, but I didn't have the time to 
go down this rabbit hole yet.
Unless there is a nice way to map between template and instantiation AST, I 
don't think combining them is feasible.

On the heuristic itself: Is there a way in C++ to "partially" unpack a 
parameter pack without this showing up in the AST? I am not aware of any, which 
is why the test "do the first and last parameters appear in a matching range of 
the arguments" seems sufficient and necessary in all but single-entry parameter 
packs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130268: [NFC] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Dawid Jurczak via Phabricator via cfe-commits]

yurai007 added a comment.

Drop WIP tag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:828
 // find the argument directly referring to the first parameter
-for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+auto LastPosibleArg = Args.end();
+for (size_t I = 1; I < Parameters.size(); ++I) {

NIT: you could use `Args.end() - Parameters.size()` or `std::advance` 
equivalent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[clang-tools-extra] 934d603 - [clang-tidy][NFC] Add preposition "of" to code annotation of ElseAfterReturnCheck

[Nathan James via cfe-commits]

Author: Zhouyi Zhou
Date: 2022-07-22T12:40:08+01:00
New Revision: 934d60382673f0e70dea41406d1f900d3833e4b3

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

LOG: [clang-tidy][NFC] Add preposition "of" to code annotation of 
ElseAfterReturnCheck

Reviewed By: njames93

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

Added: 


Modified: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
index 0558b41016379..e60f5bf804d39 100644
--- a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -263,7 +263,7 @@ void ElseAfterReturnCheck::check(const 
MatchFinder::MatchResult &Result) {
 if (!WarnOnConditionVariables)
   return;
 if (IsLastInScope) {
-  // If the if statement is the last statement its enclosing statements
+  // If the if statement is the last statement of its enclosing statements
   // scope, we can pull the decl out of the if statement.
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
<< ControlFlowInterruptor
@@ -299,7 +299,7 @@ void ElseAfterReturnCheck::check(const 
MatchFinder::MatchResult &Result) {
 if (!WarnOnConditionVariables)
   return;
 if (IsLastInScope) {
-  // If the if statement is the last statement its enclosing statements
+  // If the if statement is the last statement of its enclosing statements
   // scope, we can pull the decl out of the if statement.
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
<< ControlFlowInterruptor



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

[PATCH] D129953: [PATCH] [clang-tidy] NFC: add preposition "of" to code annotation of ElseAfterReturnCheck

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG934d60382673: [clang-tidy][NFC] Add preposition 
"of" to code annotation of… (authored by zhouyizhou, committed by 
njames93).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129953

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp


Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -263,7 +263,7 @@
 if (!WarnOnConditionVariables)
   return;
 if (IsLastInScope) {
-  // If the if statement is the last statement its enclosing statements
+  // If the if statement is the last statement of its enclosing statements
   // scope, we can pull the decl out of the if statement.
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
<< ControlFlowInterruptor
@@ -299,7 +299,7 @@
 if (!WarnOnConditionVariables)
   return;
 if (IsLastInScope) {
-  // If the if statement is the last statement its enclosing statements
+  // If the if statement is the last statement of its enclosing statements
   // scope, we can pull the decl out of the if statement.
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
<< ControlFlowInterruptor


Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -263,7 +263,7 @@
 if (!WarnOnConditionVariables)
   return;
 if (IsLastInScope) {
-  // If the if statement is the last statement its enclosing statements
+  // If the if statement is the last statement of its enclosing statements
   // scope, we can pull the decl out of the if statement.
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
<< ControlFlowInterruptor
@@ -299,7 +299,7 @@
 if (!WarnOnConditionVariables)
   return;
 if (IsLastInScope) {
-  // If the if statement is the last statement its enclosing statements
+  // If the if statement is the last statement of its enclosing statements
   // scope, we can pull the decl out of the if statement.
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
<< ControlFlowInterruptor
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D130337#3671159 , @sammccall wrote:

> FWIW, as-is with no caching, this is a ~2% slowdown on my machine (5.82 -> 
> 5.72 MB/s on SemaCodeComplete.cpp).
> Whereas D130150  using the grammar is a a 
> 7% speedup (5.82 -> 6.22), so roughly an 9% performance difference between 
> the approaches.
> My guess is we'll get some but not all of this back through caching, as 
> hashing isn't free and we'll increase the size of our working set.

And indeed, I see about 6.0MB/s with a simple llvm::DenseMap, so I expect we can get ~half the performance back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:832
   continue;
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;

upsj wrote:
> sammccall wrote:
> > The essence of this test seems to be "if we can't rule it out by counting 
> > parameters, it must be right" which... doesn't seem very robust? Am I 
> > missing something?
> > 
> > AIUI the idea here is that
> > A) `foo(pack...)` is "expanded all the way" into consecutive arguments
> > B) `foo(pack)...` is not considered forwarding for our purposes
> > C) we want to treat `foo(bar(pack)...)` specially if `bar` is "just a cast" 
> > like move or forward
> > 
> > The difference between cases A and B aren't about counting arguments 
> > though, they're about what operations occur intercede between `pack` and 
> > the `...` operator. (i.e. `foo((&pack)...)` is fine for the same reason 
> > `forward` is.
> > 
> > This seems like something best handled by:
> >  - finding the `PackExpansionExpr` controlling `pack` in the 
> > non-instantiated template AST
> >  - walking down to try to find `pack`
> >  - continue as we walk through e.g. parens or std::forward
> >  - bail out if we walk through something unknown (e.g. another function 
> > call)
> > 
> > Does this seem sensible/feasible?
> > (Walking up from `pack` is nicer but the AST doesn't support walking up 
> > well...)
> This goes a bit more generally into the question how to approach parameter 
> packs across clangd. For inlay hints and hover, we are already looking at 
> instantiated templates (is that true in all cases? Haven't dug in yet), so 
> implementing forwarding there was straightforward. For signature help and 
> code completion, we'd probably need to go via the uninstantiated template 
> (and probably lose a bit of type information on the way, e.g. reference vs. 
> value, l-value vs. r-value). If we wanted a generic way to deal with 
> forwarding, the instantiation pattern would be my first choice, but I didn't 
> have the time to go down this rabbit hole yet.
> Unless there is a nice way to map between template and instantiation AST, I 
> don't think combining them is feasible.
> 
> On the heuristic itself: Is there a way in C++ to "partially" unpack a 
> parameter pack without this showing up in the AST? I am not aware of any, 
> which is why the test "do the first and last parameters appear in a matching 
> range of the arguments" seems sufficient and necessary in all but 
> single-entry parameter packs.
> Unless there is a nice way to map between template and instantiation AST, I 
> don't think combining them is feasible.

Fair enough, I don't see one.

> On the heuristic itself: Is there a way in C++ to "partially" unpack a 
> parameter pack without this showing up in the AST? 

Not AFAIK, the is either expanded at "this level" yielding sibling args, or at 
"a higher level" yielding a single arg.
So maybe the "first & last" test is actually robust, but it would need some 
justification in comments.

There's an exception I think, if the pack size is one it falls over:
```
bar(int bar0);
bar(int bar0, int bar1);
bar(int bar0i, int bar1, int bar2);

template 
void foo(Args...args) {
  bar(args...); // should yield labels when expanded
  bar(args)...; // should not yield labels when expanded
}
// but when there's one arg (foo) the two have the same AST
```
I think it's OK to just get this one wrong


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 446796.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,51 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+  R"cpp(
+void foo(int x, int y);
+int id(int a, int b, int c);
+template 
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]], $param3[[args]])...);
+}
+void foo() {
+  bar(1, 2); // FIXME: We could have `bar(a: 1, a: 2)` here.
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"c: ", "param3"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -36,6 +36,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
 #include 
 #include 
 
@@ -786,6 +787,9 @@
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -822,12 +826,21 @@
   // in the given arguments, if it is there.
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
-for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+assert(Parameters.size() <=
+   static_cast(std::distance(Args.begin(), Args.end(;
+for (auto Begin = Args.begin(), End = Args.end() - Parameters.size() + 1;
+ Begin != End; ++Begin) {
+  if (const auto *RefArg = unwrapForward(*Begin)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
-return std::distance(Args.begin(), It);
+// Check that this expands all the way until the last parameter.
+// It's enough to look at the last parameter, because it isn't possible
+// to expand without expanding all of them.
+auto ParamEnd = Begin + Parameters.size() - 1;
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
+return std::distance(Args.begin(), Begin);
   }
 }
 return llvm::None;
@@ -872,6 +885,12 @@
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark implicit calls in the AST to properly filter here.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforward) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D130260#3671290 , @sammccall wrote:

> driveby thoughts, but please don't block on them.
>
> (if this fix is a heuristic that fixes a common crash but isn't completely 
> correct, that may still be worth landing but warrants a fixme)

The fix is not an heuristic. We just make sure we never consider a function 
with less parameters than the arguments we have, that way rest of the 
assumptions hold (we might still pick a wrong function call, as we could in the 
past, but we won't crash).
In addition to that heuristics are improved a little bit to work in presence of 
copy/move constructors.




Comment at: clang-tools-extra/clangd/AST.cpp:832
   continue;
+// Check that this expands all the way until the last parameter.
+auto ParamEnd = It + Parameters.size() - 1;

sammccall wrote:
> upsj wrote:
> > sammccall wrote:
> > > The essence of this test seems to be "if we can't rule it out by counting 
> > > parameters, it must be right" which... doesn't seem very robust? Am I 
> > > missing something?
> > > 
> > > AIUI the idea here is that
> > > A) `foo(pack...)` is "expanded all the way" into consecutive arguments
> > > B) `foo(pack)...` is not considered forwarding for our purposes
> > > C) we want to treat `foo(bar(pack)...)` specially if `bar` is "just a 
> > > cast" like move or forward
> > > 
> > > The difference between cases A and B aren't about counting arguments 
> > > though, they're about what operations occur intercede between `pack` and 
> > > the `...` operator. (i.e. `foo((&pack)...)` is fine for the same reason 
> > > `forward` is.
> > > 
> > > This seems like something best handled by:
> > >  - finding the `PackExpansionExpr` controlling `pack` in the 
> > > non-instantiated template AST
> > >  - walking down to try to find `pack`
> > >  - continue as we walk through e.g. parens or std::forward
> > >  - bail out if we walk through something unknown (e.g. another function 
> > > call)
> > > 
> > > Does this seem sensible/feasible?
> > > (Walking up from `pack` is nicer but the AST doesn't support walking up 
> > > well...)
> > This goes a bit more generally into the question how to approach parameter 
> > packs across clangd. For inlay hints and hover, we are already looking at 
> > instantiated templates (is that true in all cases? Haven't dug in yet), so 
> > implementing forwarding there was straightforward. For signature help and 
> > code completion, we'd probably need to go via the uninstantiated template 
> > (and probably lose a bit of type information on the way, e.g. reference vs. 
> > value, l-value vs. r-value). If we wanted a generic way to deal with 
> > forwarding, the instantiation pattern would be my first choice, but I 
> > didn't have the time to go down this rabbit hole yet.
> > Unless there is a nice way to map between template and instantiation AST, I 
> > don't think combining them is feasible.
> > 
> > On the heuristic itself: Is there a way in C++ to "partially" unpack a 
> > parameter pack without this showing up in the AST? I am not aware of any, 
> > which is why the test "do the first and last parameters appear in a 
> > matching range of the arguments" seems sufficient and necessary in all but 
> > single-entry parameter packs.
> > Unless there is a nice way to map between template and instantiation AST, I 
> > don't think combining them is feasible.
> 
> Fair enough, I don't see one.
> 
> > On the heuristic itself: Is there a way in C++ to "partially" unpack a 
> > parameter pack without this showing up in the AST? 
> 
> Not AFAIK, the is either expanded at "this level" yielding sibling args, or 
> at "a higher level" yielding a single arg.
> So maybe the "first & last" test is actually robust, but it would need some 
> justification in comments.
> 
> There's an exception I think, if the pack size is one it falls over:
> ```
> bar(int bar0);
> bar(int bar0, int bar1);
> bar(int bar0i, int bar1, int bar2);
> 
> template 
> void foo(Args...args) {
>   bar(args...); // should yield labels when expanded
>   bar(args)...; // should not yield labels when expanded
> }
> // but when there's one arg (foo) the two have the same AST
> ```
> I think it's OK to just get this one wrong
i agree that we can do things differently here, but with this patch i am mostly 
trying to make sure we address the resiliency so that this doesn't bite us in 
production (and a couple of easy improvements to the existing cases for common 
code patterns).

i'd land this patch as-is and discuss doing these changes in a separate issue 
if possible.



Comment at: clang-tools-extra/clangd/AST.cpp:833-837
+auto ParamEnd = It + Parameters.size() - 1;
+assert(std::distance(Args.begin(), ParamEnd) <
+   std::distance(Args.begin(), Args.end()) &&
+   "Only functions with greater than or equal number of parameters 
"
+

[PATCH] D130091: [clang][analyzer] Added partial wide character support to CStringChecker

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/test/Analysis/wstring.c:385
+  wchar_t a[32];
+  // FIXME: This should work with 'w_str' instead of 'w_str1'
+  const wchar_t w_str1[] = L"Hello world";

martong wrote:
> balazske wrote:
> > The problem may be that the global constant is not encountered as statement 
> > when a function is analyzed.
> Do we have the same problem in the non-wide case?
Yes (the previous "cstring.c" file contained that test). The problem looks 
fixable if the string length is computed at first use, not at the variable 
declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130091

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

LGTM




Comment at: clang-tools-extra/clangd/AST.cpp:830
+assert(Parameters.size() <=
+   static_cast(std::distance(Args.begin(), Args.end(;
+for (auto Begin = Args.begin(), End = Args.end() - Parameters.size() + 1;

NIT: could be shorter with `llvm::size(Args)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[clang-tools-extra] 4839929 - [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2022-07-22T14:37:13+02:00
New Revision: 4839929bed6e69346ff689ffe88750a90ebc0dfa

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

LOG: [clangd] Make forwarding parameter detection logic resilient

This could crash when our heuristic picks the wrong function. Make sure
there is enough parameters in the candidate to prevent those crashes.

Also special case copy/move constructors to make the heuristic work in
presence of those.

Fixes https://github.com/llvm/llvm-project/issues/56620

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

Added: 


Modified: 
clang-tools-extra/clangd/AST.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/AST.cpp 
b/clang-tools-extra/clangd/AST.cpp
index 44c6f49f55c20..0c67c548e6c20 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -36,6 +36,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
 #include 
 #include 
 
@@ -786,6 +787,9 @@ class ForwardingCallVisitor
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -822,12 +826,20 @@ class ForwardingCallVisitor
   // in the given arguments, if it is there.
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
-for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+assert(Parameters.size() <= static_cast(llvm::size(Args)));
+for (auto Begin = Args.begin(), End = Args.end() - Parameters.size() + 1;
+ Begin != End; ++Begin) {
+  if (const auto *RefArg = unwrapForward(*Begin)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
-return std::distance(Args.begin(), It);
+// Check that this expands all the way until the last parameter.
+// It's enough to look at the last parameter, because it isn't possible
+// to expand without expanding all of them.
+auto ParamEnd = Begin + Parameters.size() - 1;
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
+return std::distance(Args.begin(), Begin);
   }
 }
 return llvm::None;
@@ -872,6 +884,12 @@ class ForwardingCallVisitor
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark implicit calls in the AST to properly filter here.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforward) {

diff  --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp 
b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index f5741184d0351..87bb0cfc01f60 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,51 @@ TEST(InlayHints, RestrictRange) {
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4839929bed6e: [clangd] Make forwarding parameter detection 
logic resilient (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D130260?vs=446796&id=446805#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1429,6 +1429,51 @@
   ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+  R"cpp(
+struct Foo{ Foo(); Foo(int x); };
+void foo(Foo a, int b);
+template 
+void bar(Args... args) {
+  foo(args...);
+}
+template 
+void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+template 
+void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+void foo() {
+  bar($param4[[Foo{}]], $param5[[42]]);
+  bar($param6[[42]], $param7[[42]]);
+  baz($param8[[42]]);
+  bax($param9[[42]]);
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+  ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+  ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+  ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+  R"cpp(
+void foo(int x, int y);
+int id(int a, int b, int c);
+template 
+void bar(Args... args) {
+  foo(id($param1[[args]], $param2[[1]], $param3[[args]])...);
+}
+void foo() {
+  bar(1, 2); // FIXME: We could have `bar(a: 1, a: 2)` here.
+}
+  )cpp",
+  ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+  ExpectedHint{"c: ", "param3"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -36,6 +36,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
 #include 
 #include 
 
@@ -786,6 +787,9 @@
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+// Skip functions with less parameters, they can't be the target.
+if (Callee->parameters().size() < Parameters.size())
+  return;
 if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
   return dyn_cast(E) != nullptr;
 })) {
@@ -822,12 +826,20 @@
   // in the given arguments, if it is there.
   llvm::Optional findPack(typename CallExpr::arg_range Args) {
 // find the argument directly referring to the first parameter
-for (auto It = Args.begin(); It != Args.end(); ++It) {
-  const Expr *Arg = *It;
-  if (const auto *RefArg = unwrapForward(Arg)) {
+assert(Parameters.size() <= static_cast(llvm::size(Args)));
+for (auto Begin = Args.begin(), End = Args.end() - Parameters.size() + 1;
+ Begin != End; ++Begin) {
+  if (const auto *RefArg = unwrapForward(*Begin)) {
 if (Parameters.front() != RefArg->getDecl())
   continue;
-return std::distance(Args.begin(), It);
+// Check that this expands all the way until the last parameter.
+// It's enough to look at the last parameter, because it isn't possible
+// to expand without expanding all of them.
+auto ParamEnd = Begin + Parameters.size() - 1;
+RefArg = unwrapForward(*ParamEnd);
+if (!RefArg || Parameters.back() != RefArg->getDecl())
+  continue;
+return std::distance(Args.begin(), Begin);
   }
 }
 return llvm::None;
@@ -872,6 +884,12 @@
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
 E = E->IgnoreImplicitAsWritten();
+// There might be an implicit copy/move constructor call on top of the
+// forwarded arg.
+// FIXME: Maybe mark implicit calls in the AST to properly filter here.
+if (const auto *Const = dyn_cast(E))
+  if (Const->getConstructor()->isCopyOrMoveConstructor())
+E = Const->getArg(0)->IgnoreImplicitAsWritten();
 if (const auto *Call = dyn_cast(E)) {
   const auto Callee = Call->getBuiltinCallee();
   if (Callee == Builtin::BIforw

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, the change looks good in general.

> we handle *all* rules for the interesting node types explicitly, rather than 
> default: return false. This allows us to assert that all cases are handled, 
> so things don't "fall through the cracks" after grammar changes. Alternative 
> suggestions welcome. (I have a feeling this "exhaustive pattern-matching" 
> idea will come up again...)

The dangerous bit is that now we will crash at the runtime if the parsing code 
triggers a missing case.

Yeah, I hit this problem in the previous function-declarator patch. One 
approach will be to define an enum for each nonterminal `enum Nonterminal { 
rhs0_rhs1 }`, rather than put all rules into a single enum. It is easier to 
enumerate all values for a single nonterminal (I think this is the common case)

  namespace cxx {
  namespace rule {
  
  enum simple_type_specifier {
builtin_type0,
nested_name_specifier0_type_name1,
...
  }
  
  }
  }



In D130337#3671159 , @sammccall wrote:

> FWIW, as-is with no caching, this is a ~2% slowdown on my machine (5.82 -> 
> 5.72 MB/s on SemaCodeComplete.cpp).

Actually, the number looks quite good to me (I expected that there will be a 
significant slowdown without caching). I think we can check in the current 
version, and add the caching stuff afterwards.

> Whereas D130150  using the grammar is a a 
> 7% speedup (5.82 -> 6.22), so roughly an 9% performance difference between 
> the approaches.

Yeah, because the grammar-based approach reduces the number of ambiguous node 
in the forest.




Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:284
+   
defining_type_specifier_seq_0defining_type_specifier_1defining_type_specifier_seq,
+   GUARD(!hasExclusiveType(P.RHS.front()) ||
+ !hasExclusiveType(P.RHS.back()))},

nit: I would suggest using the index explicitly `P.RHS[0]`, `P.RHS[1]`, it 
increases the readability (the rul name encoding the index, easier to spot the 
corresponding element).



Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.bnf:353
 decl-specifier-seq := decl-specifier
-decl-specifier-seq := decl-specifier decl-specifier-seq
+decl-specifier-seq := decl-specifier decl-specifier-seq [guard]
 storage-class-specifier := STATIC

offtopic comment: The sad bit of the `RuleID` approach (vs `guard=SingleType`) 
is that we don't really know what kind of the guard is by reading the grammar 
file only.

I think this is critical information, and worth to keep in the grammar file. 
(ideas: add comments, or bring the `guard=SingleType` in the grammar again, but 
we ignore the `guard` value in the grammar parsing).



Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.bnf:385
+builtin-type := BOOL
 simple-type-specifier := SHORT
+builtin-type := INT

I think the reason to leave SHORT/LONG/SIGNED UNSIGNED as-is is that they can 
combined with other type (e.g. short int).

Can we group them together, and add a comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D130260#3671494 , @kadircet wrote:

> In D130260#3671290 , @sammccall 
> wrote:
>
>> driveby thoughts, but please don't block on them.
>>
>> (if this fix is a heuristic that fixes a common crash but isn't completely 
>> correct, that may still be worth landing but warrants a fixme)
>
> The fix is not an heuristic. We just make sure we never consider a function 
> with less parameters than the arguments we have, that way rest of the 
> assumptions hold (we might still pick a wrong function call, as we could in 
> the past, but we won't crash).
> In addition to that heuristics are improved a little bit to work in presence 
> of copy/move constructors.

OK, but it's missing a comment that says "this is a heuristic and may not be 
correct, at least it won't crash"!

The original code was intended to be *correct*, not a heuristic. If we're not 
going to fix it properly (which is reasonable in the circumstances), we need to 
document the limitations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260

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

[PATCH] D129048: Rewording the "static_assert" to static assertion

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D129048#3670212 , @ldionne wrote:

> The libc++ CI runs pre-commit only, and it runs on all commits that touch 
> something under `libcxx/`, `libcxxabi/`, `runtimes/` and `libunwind/`. In 
> particular, it won't trigger if you make changes to `clang/` only -- that 
> would create too much traffic and concretely it wouldn't help much, as we're 
> rarely broken by Clang changes (this is an exception). Once the tests under 
> `libcxx/` were touched, the libc++ CI triggered, I found this build for 
> instance: https://buildkite.com/llvm-project/libcxx-ci/builds/12210. I do 
> think it boils down to familiarity -- while the Clang pre-commit CI is 
> sometimes overlooked because it fails spuriously, the libc++ CI usually 
> doesn't, so when it fails, there's usually a good reason. I do understand why 
> someone mostly familiar with the Clang workflow could think they needed to be 
> overlooked, though.

Thanks for the explanation!

FWIW, the specific reason why I overloooked it was because of running against 
old versions of Clang -- I saw the diagnostic was correct in the test and 
presumed that the old clang was a bot configuration issue. It definitely 
doesn't help that Clang's precommit CI tends to be spuriously broken quite 
often; it's trained some of us reviewers to be more dismissive of failing 
precommit CI than we should.

> Concretely, libc++ supports the two latest releases of Clang, and so the CI 
> tests against those. In fact, we run most of the tests using pre-installed 
> Clangs since it allows us to bypass building Clang itself, which is a 
> necessary optimization in the current setup. Regarding the tests themselves, 
> I am able to find the following kind of output in the CI job that failed:

Yeah, which makes a lot of sense for libc++!

>   FAIL: llvm-libc++-shared.cfg.in :: 
> std/numerics/numbers/illformed.verify.cpp (4388 of 7532)
>   [...]
>   error: 'error' diagnostics expected but not seen:
> File 
> /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/numbers
>  Line * (directive at 
> /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/numbers/illformed.verify.cpp:15):
>  static assertion failed{{.*}}A program that instantiates a primary template 
> of a mathematical constant variable template is ill-formed.
>   error: 'error' diagnostics seen but not expected:
> File 
> /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/numbers
>  Line 83: static_assert failed due to requirement '__false' "A program 
> that instantiates a primary template of a mathematical constant variable 
> template is ill-formed."
>   2 errors generated.
>
>
> IMO this is reasonable output, in fact it's pretty much as good as we could 
> have had. So I'm not taking any action item regarding improving the output of 
> our CI, but I'm entirely open to suggestions if somebody has some.

I don't have a concrete suggestion for the CI output itself -- the failure was 
clear in retrospect, this was more a matter of not being familiar with the 
testing practices used.

> In D129048#3669568 , @aaron.ballman 
> wrote:
>
>> At a high-level, I think it boils down to familiarity. If we can get the 
>> libc++ CI to run as part of precommit CI (assuming precommit CI can be made 
>> to run with reliable results, which is a bit of a question mark),
>
> It is pretty reliable. However, I don't think it'll ever become reasonable to 
> run the full libc++ CI on every Clang patch due to resource limitations.

The resource limitations are certainly a pragmatic thing for us to be concerned 
with, but I hope we're able to find some happy middle ground.

>> It would have also helped to catch the cause for the initial revert where 
>> everyone thought the patch was ready to land.
>
> 100% agreed, however this is blocked on the resource limitations mentioned 
> above. I guess one thing we could do here is trigger a simple bootstrapping 
> build of Clang and run the libc++ tests in a single configuration even on 
> changes to `clang/`. That might catch most issues and perhaps that would be 
> small enough to be scalable. I'll experiment with that after the release.

Thanks, that would be really beneficial. It's not just diagnostic wording 
changes that cause Clang developers problems with libc++ reverts; I've also 
seen C++20 work reverted because it broke libc++ without anyone in Clang 
knowing about it until after it landed (most recently, it's with the deferred 
concept checking work done by @erichkeane).

>> Another thing that would help would be to get the libc++ test bots into the 
>> LLVM lab (https://lab.llvm.org/buildbot/#/builders)
>
> Libc++ CI is pre-commit, the lab is post-commit only AFAICT.

Correct, but right no

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.bnf:353
 decl-specifier-seq := decl-specifier
-decl-specifier-seq := decl-specifier decl-specifier-seq
+decl-specifier-seq := decl-specifier decl-specifier-seq [guard]
 storage-class-specifier := STATIC

hokein wrote:
> offtopic comment: The sad bit of the `RuleID` approach (vs 
> `guard=SingleType`) is that we don't really know what kind of the guard is by 
> reading the grammar file only.
> 
> I think this is critical information, and worth to keep in the grammar file. 
> (ideas: add comments, or bring the `guard=SingleType` in the grammar again, 
> but we ignore the `guard` value in the grammar parsing).
Yeah, the current balance doesn't feel obviously right.

I'd like to leave this for the time being, because of the various options 
(remove the annotations, replace them with comments, bring back values), i'm 
not sure there's a clear winner.

I have a suspicion that while it's appealing now to at least reference here all 
the restrictions that may apply, when we add "soft" disambiguation based on 
scoring it may not be so appealing as we won't be documenting the things that 
affect the parse in practice.



Comment at: clang-tools-extra/pseudo/lib/cxx/cxx.bnf:385
+builtin-type := BOOL
 simple-type-specifier := SHORT
+builtin-type := INT

hokein wrote:
> I think the reason to leave SHORT/LONG/SIGNED UNSIGNED as-is is that they can 
> combined with other type (e.g. short int).
> 
> Can we group them together, and add a comment?
Grouped them.
I don't think the idea that SHORT is a specifier but not actually a type needs 
to be spelled out, but added a comment about `builtin-type` (which is 
nonstandard) which hints at this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D130331: [C++20] [Modules] Disable preferred_name when writing a C++20 Module interface

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So this is perhaps the 'most acceptable' version of this workaround or me.  
What do you think @aaron.ballman and @tahonermann ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130331

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

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

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

In D130337#3671559 , @hokein wrote:

> Thanks, the change looks good in general.
>
>> we handle *all* rules for the interesting node types explicitly, rather than 
>> default: return false. This allows us to assert that all cases are handled, 
>> so things don't "fall through the cracks" after grammar changes. Alternative 
>> suggestions welcome. (I have a feeling this "exhaustive pattern-matching" 
>> idea will come up again...)
>
> The dangerous bit is that now we will crash at the runtime if the parsing 
> code triggers a missing case.

Yeah, grammar changes make this brittle.
Still, hopefully we canary our releases...

> Yeah, I hit this problem in the previous function-declarator patch. One 
> approach will be to define an enum for each nonterminal `enum Nonterminal { 
> rhs0_rhs1 }`, rather than put all rules into a single enum. It is easier to 
> enumerate all values for a single nonterminal (I think this is the common 
> case)

Well, I don't think it's the most common (vs just targeting a rule or two) but 
certainly we never enumerate *all* the rules!
Interesting idea.

I'm not sure it'd be worth adding control-flow here to split up the switches by 
rule type though, switches are pretty heavyweight syntactically.

> In D130337#3671159 , @sammccall 
> wrote:
>
>> FWIW, as-is with no caching, this is a ~2% slowdown on my machine (5.82 -> 
>> 5.72 MB/s on SemaCodeComplete.cpp).
>
> Actually, the number looks quite good to me (I expected that there will be a 
> significant slowdown without caching). I think we can check in the current 
> version, and add the caching stuff afterwards.

I'm not so happy...

The relevant baseline here IMO is the +7% version, as we're clearly currently 
paying ~8% cost to build all the silly incorrect parses, and that cost is 
artificial.
So 9% overall slowdown now, and still 5% with the cache, feels significant. But 
we can change this later.

>> Whereas D130150  using the grammar is a a 
>> 7% speedup (5.82 -> 6.22), so roughly an 9% performance difference between 
>> the approaches.
>
> Yeah, because the grammar-based approach reduces the number of ambiguous node 
> in the forest.

*Both* approaches do that, which shows how slow the guard version is :-(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

LMK if anything else blocking here. I want to take a stab at changing the enums 
(cool idea), but I don't think there's much point blocking this patch on it.
Better to use it as a testbed for the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:18886
+  InitVal.getActiveBits() ? InitVal.getActiveBits() : 1;
+  NumPositiveBits = std::max(NumPositiveBits, ActiveBits);
+} else

What about:

`std::max({NumPositiveBits, ActiveBits, 1})` (which uses the init-list version 
of std::max).

Also, not completely following this: BUT we don't need a positive bit IF we 
have a negative bit, right?  So this is only necessary if BOTH NumPositiveBits 
and NumNegativeBits would have been 0?



Comment at: clang/lib/Sema/SemaDecl.cpp:1
+} else
   NumNegativeBits = std::max(NumNegativeBits,
  (unsigned)InitVal.getMinSignedBits());

By coding standard, we need curleys around the 'else' as well if we have it on 
the 'if'.



Comment at: compiler-rt/test/ubsan/TestCases/Misc/enum.cpp:10
+enum class EBool : bool { a = 1 } e3;
+enum EEmpty {};
 

can you do a test on:

`enum ENeg { a = -1 }`
?

That should ALSO have only a single bit.


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

https://reviews.llvm.org/D130301

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

[PATCH] D130066: [pseudo] Key guards by RuleID, add guards to literals (and 0).

[Amy Kwan via Phabricator via cfe-commits]

amyk added a comment.

In D130066#3670875 , @sammccall wrote:

> In D130066#3670745 , @amyk wrote:
>
>> Hi!
>>
>> It appears that this patch is causing a build failure on a couple PPC bots 
>> that build with shared libraries:
>> https://lab.llvm.org/buildbot/#/builders/57/builds/20179
>> https://lab.llvm.org/buildbot/#/builders/121/builds/21678
>>
>> The specific error that occurs looks like this:
>>
>>   2.485 [936/22/19] Linking CXX shared library lib/libclangPseudoCXX.so.15git
>>   FAILED: lib/libclangPseudoCXX.so.15git 
>>   : && /home/buildbots/clang.11.0.0/bin/clang++ 
>> --gcc-toolchain=/opt/rh/devtoolset-7/root/usr -fPIC -fPIC 
>> -fvisibility-inlines-hidden -Werror -Werror=date-time 
>> -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
>> -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
>> -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
>> -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
>> -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion 
>> -Wmisleading-indentation -fdiagnostics-color -ffunction-sections 
>> -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 
>> -DNDEBUG  -Wl,-z,defs -Wl,-z,nodelete   
>> -Wl,-rpath-link,/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/./lib
>>   -Wl,--gc-sections -shared -Wl,-soname,libclangPseudoCXX.so.15git -o 
>> lib/libclangPseudoCXX.so.15git 
>> tools/clang/tools/extra/pseudo/lib/cxx/CMakeFiles/obj.clangPseudoCXX.dir/CXX.cpp.o
>>   -Wl,-rpath,"\$ORIGIN/../lib"  lib/libclangPseudo.so.15git  
>> lib/libclangPseudoGrammar.so.15git  lib/libLLVMSupport.so.15git  
>> -Wl,-rpath-link,/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/lib
>>  && :
>>   
>> tools/clang/tools/extra/pseudo/lib/cxx/CMakeFiles/obj.clangPseudoCXX.dir/CXX.cpp.o:(.toc+0x10):
>>  undefined reference to `clang::charinfo::InfoTable'
>>   clang++: error: linker command failed with exit code 1 (use -v to see 
>> invocation)
>>
>> Would you be able to take a look at this issue (or revert the patch if this 
>> requires more time to resolve)? Thank you in advance.
>
> Sorry about that. d26ee284ded30aff46 
>  links 
> in clangBasic for this dep.
> (I think it wasn't showing up in the non-shared builds because these 
> functions were always inlined)

Thank you for fixing the build failures, @sammccall, I appreciate it! The bots 
are back to green now. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130066

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

[PATCH] D129654: [Clang][Driver] Fix include paths for `--sysroot /` on OpenBSD/FreeBSD

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan added a comment.

Thanks @brad @3405691582 @MaskRay for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129654

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

[clang] 1d0cc51 - [Clang][Driver] Fix include paths for `--sysroot /` on OpenBSD/FreeBSD

[Egor Zhdan via cfe-commits]

Author: Egor Zhdan
Date: 2022-07-22T14:30:32+01:00
New Revision: 1d0cc510516d50c459f78896a0375fadb13a2b45

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

LOG: [Clang][Driver] Fix include paths for `--sysroot /` on OpenBSD/FreeBSD

This is the same change as https://reviews.llvm.org/D126289, but applied for 
OpenBSD & FreeBSD.

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

Added: 
clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/bin/.keep
clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/include/c++/v1/.keep
clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/lib/.keep
clang/test/Driver/Inputs/basic_openbsd_libcxx_tree/usr/bin/.keep
clang/test/Driver/Inputs/basic_openbsd_libcxx_tree/usr/include/c++/v1/.keep
clang/test/Driver/Inputs/basic_openbsd_libcxx_tree/usr/lib/.keep

Modified: 
clang/lib/Driver/ToolChains/FreeBSD.cpp
clang/lib/Driver/ToolChains/OpenBSD.cpp
clang/test/Driver/freebsd.cpp
clang/test/Driver/openbsd.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/FreeBSD.cpp 
b/clang/lib/Driver/ToolChains/FreeBSD.cpp
index fec80f85e2787..e5451c20a00c8 100644
--- a/clang/lib/Driver/ToolChains/FreeBSD.cpp
+++ b/clang/lib/Driver/ToolChains/FreeBSD.cpp
@@ -389,10 +389,10 @@ FreeBSD::FreeBSD(const Driver &D, const llvm::Triple 
&Triple,
   // back to '/usr/lib' if it doesn't exist.
   if ((Triple.getArch() == llvm::Triple::x86 || Triple.isMIPS32() ||
Triple.isPPC32()) &&
-  D.getVFS().exists(getDriver().SysRoot + "/usr/lib32/crt1.o"))
-getFilePaths().push_back(getDriver().SysRoot + "/usr/lib32");
+  D.getVFS().exists(concat(getDriver().SysRoot, "/usr/lib32/crt1.o")))
+getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib32"));
   else
-getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
+getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib"));
 }
 
 ToolChain::CXXStdlibType FreeBSD::GetDefaultCXXStdlibType() const {
@@ -411,14 +411,14 @@ unsigned FreeBSD::GetDefaultDwarfVersion() const {
 void FreeBSD::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
 llvm::opt::ArgStringList &CC1Args) const {
   addSystemInclude(DriverArgs, CC1Args,
-   getDriver().SysRoot + "/usr/include/c++/v1");
+   concat(getDriver().SysRoot, "/usr/include/c++/v1"));
 }
 
 void FreeBSD::addLibStdCxxIncludePaths(
 const llvm::opt::ArgList &DriverArgs,
 llvm::opt::ArgStringList &CC1Args) const {
-  addLibStdCXXIncludePaths(getDriver().SysRoot + "/usr/include/c++/4.2", "", 
"",
-   DriverArgs, CC1Args);
+  addLibStdCXXIncludePaths(concat(getDriver().SysRoot, "/usr/include/c++/4.2"),
+   "", "", DriverArgs, CC1Args);
 }
 
 void FreeBSD::AddCXXStdlibLibArgs(const ArgList &Args,

diff  --git a/clang/lib/Driver/ToolChains/OpenBSD.cpp 
b/clang/lib/Driver/ToolChains/OpenBSD.cpp
index 44cc9dee67f4d..8b3a40606ff32 100644
--- a/clang/lib/Driver/ToolChains/OpenBSD.cpp
+++ b/clang/lib/Driver/ToolChains/OpenBSD.cpp
@@ -284,7 +284,7 @@ SanitizerMask OpenBSD::getSupportedSanitizers() const {
 OpenBSD::OpenBSD(const Driver &D, const llvm::Triple &Triple,
  const ArgList &Args)
 : Generic_ELF(D, Triple, Args) {
-  getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
+  getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib"));
 }
 
 void OpenBSD::AddClangSystemIncludeArgs(
@@ -317,13 +317,14 @@ void OpenBSD::AddClangSystemIncludeArgs(
 return;
   }
 
-  addExternCSystemInclude(DriverArgs, CC1Args, D.SysRoot + "/usr/include");
+  addExternCSystemInclude(DriverArgs, CC1Args,
+  concat(D.SysRoot, "/usr/include"));
 }
 
 void OpenBSD::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
 llvm::opt::ArgStringList &CC1Args) const {
   addSystemInclude(DriverArgs, CC1Args,
-   getDriver().SysRoot + "/usr/include/c++/v1");
+   concat(getDriver().SysRoot, "/usr/include/c++/v1"));
 }
 
 void OpenBSD::AddCXXStdlibLibArgs(const ArgList &Args,

diff  --git a/clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/bin/.keep 
b/clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/bin/.keep
new file mode 100644
index 0..e69de29bb2d1d

diff  --git 
a/clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/include/c++/v1/.keep 
b/clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/include/c++/v1/.keep
new file mode 100644
index 0..e69de29bb2d1d

diff  --git a/clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/lib/.keep 
b/clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/lib/.keep
new file mode 100644
index 0

[PATCH] D129654: [Clang][Driver] Fix include paths for `--sysroot /` on OpenBSD/FreeBSD

[Egor Zhdan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1d0cc510516d: [Clang][Driver] Fix include paths for 
`--sysroot /` on OpenBSD/FreeBSD (authored by egorzhdan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129654

Files:
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.cpp
  clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/bin/.keep
  clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/include/c++/v1/.keep
  clang/test/Driver/Inputs/basic_freebsd_libcxx_tree/usr/lib/.keep
  clang/test/Driver/Inputs/basic_openbsd_libcxx_tree/usr/bin/.keep
  clang/test/Driver/Inputs/basic_openbsd_libcxx_tree/usr/include/c++/v1/.keep
  clang/test/Driver/Inputs/basic_openbsd_libcxx_tree/usr/lib/.keep
  clang/test/Driver/freebsd.cpp
  clang/test/Driver/openbsd.cpp

Index: clang/test/Driver/openbsd.cpp
===
--- clang/test/Driver/openbsd.cpp
+++ clang/test/Driver/openbsd.cpp
@@ -19,3 +19,23 @@
 // RUN: %clangxx %s -### -pg -o %t.o -target arm-unknown-openbsd 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-CXX %s
 // CHECK-PG-CXX: "-lc++_p" "-lc++abi_p" "-lpthread_p" "-lm_p"
+
+// Test include paths with a sysroot.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN: --target=amd64-pc-openbsd \
+// RUN: --sysroot=%S/Inputs/basic_openbsd_libcxx_tree \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT %s
+// CHECK-LIBCXX-SYSROOT: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SAME: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-LIBCXX-SYSROOT-SAME: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+
+// Test include paths when the sysroot path ends with `/`.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN: --target=amd64-pc-openbsd \
+// RUN: --sysroot=%S/Inputs/basic_openbsd_libcxx_tree/ \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT-SLASH %s
+// CHECK-LIBCXX-SYSROOT-SLASH: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-isysroot" "[[SYSROOT:[^"]+/]]"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/c++/v1"
Index: clang/test/Driver/freebsd.cpp
===
--- clang/test/Driver/freebsd.cpp
+++ clang/test/Driver/freebsd.cpp
@@ -20,3 +20,23 @@
 // CHECK-PG-FOURTEEN: "-lc++" "-lm"
 // CHECK-PG-TEN: "-lc++_p" "-lm_p"
 // CHECK-PG-NINE: "-lstdc++_p" "-lm_p"
+
+// Test include paths with a sysroot.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN: --target=amd64-unknown-freebsd \
+// RUN: --sysroot=%S/Inputs/basic_openbsd_libcxx_tree \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT %s
+// CHECK-LIBCXX-SYSROOT: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SAME: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-LIBCXX-SYSROOT-SAME: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+
+// Test include paths when the sysroot path ends with `/`.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN: --target=amd64-unknown-freebsd \
+// RUN: --sysroot=%S/Inputs/basic_openbsd_libcxx_tree/ \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT-SLASH %s
+// CHECK-LIBCXX-SYSROOT-SLASH: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-isysroot" "[[SYSROOT:[^"]+/]]"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/c++/v1"
Index: clang/lib/Driver/ToolChains/OpenBSD.cpp
===
--- clang/lib/Driver/ToolChains/OpenBSD.cpp
+++ clang/lib/Driver/ToolChains/OpenBSD.cpp
@@ -284,7 +284,7 @@
 OpenBSD::OpenBSD(const Driver &D, const llvm::Triple &Triple,
  const ArgList &Args)
 : Generic_ELF(D, Triple, Args) {
-  getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
+  getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib"));
 }
 
 void OpenBSD::AddClangSystemIncludeArgs(
@@ -317,13 +317,14 @@
 return;
   }
 
-  addExternCSystemInclude(DriverArgs, CC1Args, D.SysRoot + "/usr/include");
+  addExternCSystemInclude(DriverArgs, CC1Args,
+  concat(D.SysRoot, "/usr/include"));
 }
 
 void OpenBSD::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
 llvm::opt::ArgStringList &CC1Args) const {
   addSystemInclude(DriverArgs, CC1Args,
-   getDriver().SysRoot + "/usr/include/c++/v1");
+   concat(getDriver().SysRoot, "/usr/include/c++/v1"));
 }
 
 void OpenBSD::AddCXXStdlibLibArgs(const ArgList &Args,
Index: clang/lib/Driver/ToolChains/FreeBSD.cpp
===
--- clang/lib/Driver/ToolChains/FreeBSD.cpp
+++ clang/lib/Dr

[PATCH] D130337: [pseudo] Eliminate multiple-specified-types ambiguities using guards

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The change looks good to me.

In D130337#3671575 , @sammccall wrote:

> LMK if anything else blocking here.

I don't want to block you, but I'd suggest postponing it a little bit until we 
collect some metrics in our internal pipeline (I think usaxena95@ is working on 
it, hopefully we will get it next week).

> I want to take a stab at changing the enums (cool idea), but I don't think 
> there's much point blocking this patch on it.

Agree.

> Well, I don't think it's the most common (vs just targeting a rule or two) 
> but certainly we never enumerate *all* the rules!
> Interesting idea.

If we look at the existing guard implementations, we have a few of these usages:

- in `isFunctionDeclarator`, we enumerate all rules of `noptr_declarator`, 
`ptr_declarator`, `declarator` ;
- in `hasExclusiveType`, we enumerate all rules of `decl_specifier`, 
`simple_type_specifier`, `type_specifier`, `type_specifier_seq` etc;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130337

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

[PATCH] D128927: [libc++] Always build c++experimental.a

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.



>> In https://github.com/llvm/llvm-zorg/pull/28 you mentioned that "for a 
>> couple of years now, Clang and libc++ are entirely separate projects w.r.t. 
>> how they are shipped on Apple platforms, and it doesn't make sense to build 
>> libc++ at the same time as Clang anymore".
>> Does that mean the libc++ headers have moved to the SDK now, and if so from 
>> what version? If it's in the SDK version we use (I'd have to check), perhaps 
>> that would allow us to stop building like this.
>
> Yes, the libc++ headers are in the SDK. They have been since Xcode 12 if I'm 
> not mistaken. They are *also* still shipped in the Clang toolchain, but that 
> will change eventually -- there are a few things blocking us from doing that, 
> but they will go away eventually.

We've removed libc++ from our toolchain build process 
(https://chromium-review.googlesource.com/c/chromium/src/+/3779521) so 
hopefully will see fewer problems of this nature in the future. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128927

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

[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Sam, this is a great start! I'm really excited to see that you have a core 
working so quickly.




Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:208
+
+  // TODO: Currently this only works if the callee is never a method and the
+  // same callee is never analyzed from multiple separate callsites. To





Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220
+  assert(Body != nullptr);
+  initGlobalVars(*Body, Env);
+

I wonder how this will work between caller and callee. Do we need separate 
global var state in the frame? If so, maybe mention that as well in the FIXME 
above.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:237
+const Expr *Arg = *ArgIt;
+// TODO: Confirm that this is the correct `SkipPast` to use here.
+auto *ArgLoc = Env.getStorageLocation(*Arg, SkipPast::None);

I'm pretty sure we want `SkipPast::Reference`. That will ensure that the 
parameter and argument share the same underlying location. Otherwise, in the 
case of references, the parameter will point to the reference location object 
rather than just directly to the location.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:517
+
+  // TODO: Cache these CFGs.
+  auto CFCtx = ControlFlowContext::build(F, F->getBody(), &ASTCtx);

here and below: s/TODO/FIXME.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:524
+  auto CalleeEnv = Env.pushCall(S);
+  auto Analysis = NoopAnalysis(ASTCtx, /*ApplyBuiltinTransfer=*/true);
+

This seems worth a FIXME or, at least, an explanation.  It implies that with 
the current design, we can't support general-purpose analyses, which we should 
probably fix. Given our goal of supporting models that don't involve 
specialized lattices, I think this is a good compromise for the short term, but 
not a stable solution for the framework (hence  FIXME sounds right).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306

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

[PATCH] D129048: Rewording the "static_assert" to static assertion

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D129048#3671568 , @aaron.ballman 
wrote:

> FWIW, I've convinced myself that I agree with you here that the burden 
> probably should have been on libc++ maintainers in this case. libc++ almost 
> feels more like a downstream consumer of Clang in terms of testing needs, so 
> I think when tests break because Clang diagnostic wording or behavior changes 
> in a standards conforming way, libc++ folks should address it, but if Clang 
> changes in a way that's not standards conforming, then Clang folks should 
> address it. (Roughly speaking.)

Yes, exactly. It's always possible to write tests that depend on implementation 
details of another component in subtle ways, and the fact that such brittle 
tests exist doesn't create a responsibility on that component to avoid breaking 
those downstream users. Here, it's about Clang making a valid change and libc++ 
containing brittle tests, but it happens quite often where libc++ changes 
something valid and a downstream consumer is broken in some way. The LLVM 
revert culture has some benefits to keep things working in a post-commit CI 
world, however I've noticed that it also creates a lot of friction between 
projects and sometimes makes important work much more difficult to land than it 
should. For example, we're currently in the middle of D128146 
 where LLDB reverted an important patch for 
LLVM 15 because one of their tests broke. This is not something that comes up 
super often, but it's extremely disruptive and frustrating when it does, and I 
think it would be worth trying to address. Anyway, I'm digressing now, but I'll 
try to talk to a few people at the LLVM Dev Meeting to see if this is a shared 
concern and to think about potential solutions to address this.

> That said, I absolutely think we all need to continue to collaborate closely 
> with one another as best we can when issues arise, and I really appreciate 
> the discussion on how we can do that!

Agreed.

> For this particular issue, I'd like @Codesbyusman to continue to try to fix 
> the libc++ testing issues (it's good experience), but if that takes 
> significantly longer (say, more than 8 hours of his effort), perhaps @ldionne 
> or someone else from libc++ will have a moment to step in to help?

Replacing `static_assert` by `(static_assert|static assertion)` should do the 
trick. See the patch attached to this comment, I think it should satisfy the CI 
@Codesbyusman.
F23872372: static_assert.diff 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129048

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

[PATCH] D129048: Rewording the "static_assert" to static assertion

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D129048#3671657 , @ldionne wrote:

> In D129048#3671568 , @aaron.ballman 
> wrote:
>
>> FWIW, I've convinced myself that I agree with you here that the burden 
>> probably should have been on libc++ maintainers in this case. libc++ almost 
>> feels more like a downstream consumer of Clang in terms of testing needs, so 
>> I think when tests break because Clang diagnostic wording or behavior 
>> changes in a standards conforming way, libc++ folks should address it, but 
>> if Clang changes in a way that's not standards conforming, then Clang folks 
>> should address it. (Roughly speaking.)
>
> Yes, exactly. It's always possible to write tests that depend on 
> implementation details of another component in subtle ways, and the fact that 
> such brittle tests exist doesn't create a responsibility on that component to 
> avoid breaking those downstream users. Here, it's about Clang making a valid 
> change and libc++ containing brittle tests, but it happens quite often where 
> libc++ changes something valid and a downstream consumer is broken in some 
> way. The LLVM revert culture has some benefits to keep things working in a 
> post-commit CI world, however I've noticed that it also creates a lot of 
> friction between projects and sometimes makes important work much more 
> difficult to land than it should. For example, we're currently in the middle 
> of D128146  where LLDB reverted an 
> important patch for LLVM 15 because one of their tests broke. This is not 
> something that comes up super often, but it's extremely disruptive and 
> frustrating when it does, and I think it would be worth trying to address. 
> Anyway, I'm digressing now, but I'll try to talk to a few people at the LLVM 
> Dev Meeting to see if this is a shared concern and to think about potential 
> solutions to address this.
>
>> That said, I absolutely think we all need to continue to collaborate closely 
>> with one another as best we can when issues arise, and I really appreciate 
>> the discussion on how we can do that!
>
> Agreed.
>
>> For this particular issue, I'd like @Codesbyusman to continue to try to fix 
>> the libc++ testing issues (it's good experience), but if that takes 
>> significantly longer (say, more than 8 hours of his effort), perhaps 
>> @ldionne or someone else from libc++ will have a moment to step in to help?
>
> Replacing `static_assert` by `(static_assert|static assertion)` should do the 
> trick. See the patch attached to this comment, I think it should satisfy the 
> CI @Codesbyusman.
> F23872372: static_assert.diff 

I just want to say: I really appreciate the insight you gave into the libc++ 
testing in this thread.  From the CFE's perspective, our view of precommit is 
"it is basically always misconfigured or broken in some way", so knowing that 
you guys keep yours running well is nice to hear!  We'll be more cognizant in 
the future.

I DO think there IS a sizable difference between patches like this one, and 
patches like my deferred-concepts (which, btw, the libc++ tests have been great 
for coming up with new tests... though it was a pain to figure out how to run 
them in the first place!). My patches DID deserve the reverts it received so 
far, so don't see Aaron's mention as an issue with that.

I appreciate your suggestion/patch file, that IS a much better regex than we'd 
come up with, and looks like it should work.  So thank you!  We'll commit that 
once @Codesbyusman can get it integrated into his patch (and we'll make sure CI 
passes too!).

Thanks again Louis!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129048

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

[PATCH] D127284: [WIP] [clang-repl] Support statements on global scope in incremental mode.

[Vassil Vassilev via Phabricator via cfe-commits]

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

Address comments.


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

https://reviews.llvm.org/D127284

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Parse/Parser.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Interpreter/IncrementalParser.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Interpreter/execute-stmts.cpp
  clang/unittests/Interpreter/InterpreterTest.cpp

Index: clang/unittests/Interpreter/InterpreterTest.cpp
===
--- clang/unittests/Interpreter/InterpreterTest.cpp
+++ clang/unittests/Interpreter/InterpreterTest.cpp
@@ -120,12 +120,7 @@
 
   // FIXME: Add support for wrapping and running statements.
   auto R2 = Interp->Parse("var1++; printf(\"var1 value %d\\n\", var1);");
-  EXPECT_FALSE(!!R2);
-  using ::testing::HasSubstr;
-  EXPECT_THAT(DiagnosticsOS.str(),
-  HasSubstr("error: unknown type name 'var1'"));
-  auto Err = R2.takeError();
-  EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err)));
+  EXPECT_TRUE(!!R2);
 }
 
 TEST(InterpreterTest, UndoCommand) {
Index: clang/test/Interpreter/execute-stmts.cpp
===
--- /dev/null
+++ clang/test/Interpreter/execute-stmts.cpp
@@ -0,0 +1,20 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+// RUN: cat %s | clang-repl | FileCheck %s
+
+int i = 12;
+++i;
+extern "C" int printf(const char*,...);
+printf("i = %d\n", i);
+// CHECK: i = 13
+
+namespace Ns { void f(){ i++; } }
+Ns::f();
+
+void g() { ++i; }
+g();
+
+printf("i = %d\n", i);
+// CHECK: i = 15
+
+%quit
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -582,13 +582,14 @@
 ///
 /// Note that in C, it is an error if there is no first declaration.
 bool Parser::ParseFirstTopLevelDecl(DeclGroupPtrTy &Result,
-Sema::ModuleImportState &ImportState) {
+Sema::ModuleImportState &ImportState,
+StmtVector *Stmts) {
   Actions.ActOnStartOfTranslationUnit();
 
   // For C++20 modules, a module decl must be the first in the TU.  We also
   // need to track module imports.
   ImportState = Sema::ModuleImportState::FirstDecl;
-  bool NoTopLevelDecls = ParseTopLevelDecl(Result, ImportState);
+  bool NoTopLevelDecls = ParseTopLevelDecl(Result, ImportState, Stmts);
 
   // C11 6.9p1 says translation units must have at least one top-level
   // declaration. C++ doesn't have this restriction. We also don't want to
@@ -609,7 +610,8 @@
 ///   declaration
 /// [C++20]   module-import-declaration
 bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
-   Sema::ModuleImportState &ImportState) {
+   Sema::ModuleImportState &ImportState,
+   StmtVector *Stmts /*=nullptr*/) {
   DestroyTemplateIdAnnotationsRAIIObj CleanupRAII(*this);
 
   // Skip over the EOF token, flagging end of previous input for incremental
@@ -724,6 +726,23 @@
   ParsedAttributes attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
 
+  // FIXME: Remove the incremental processing pre-condition and verify clang
+  // still can pass its test suite, which will harden `isDeclarationStatement`.
+  // It is known to have several weaknesses, for example in
+  // isConstructorDeclarator, infinite loop in c-index-test, etc..
+  // Parse a block of top-level-stmts.
+  while (PP.isIncrementalProcessingEnabled() && Stmts &&
+ !isDeclarationStatement(/*IsParsingDecl=*/false)) {
+//isStmtExpr ? ParsedStmtContext::InStmtExpr : ParsedStmtContext()
+ParsedStmtContext SubStmtCtx = ParsedStmtContext();
+auto R = ParseStatementOrDeclaration(*Stmts, SubStmtCtx);
+if (!R.isUsable())
+  return true;
+Stmts->push_back(R.get());
+if (Tok.is(tok::eof))
+  return false;
+  }
+
   Result = ParseExternalDeclaration(attrs);
   // An empty Result might mean a line with ';' or some parsing error, ignore
   // it.
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -22,6 +22,7 @@
 ///
 /// declaration-statement:
 ///   block-declaration
+///   template-declaration
 ///
 /// block-declaration:
 ///   simple-declaration
@@ -46,12 +47,24 @@
 ///   'using' 'namespace' '::'[opt] nested-name-specifier[opt]
 /// namespace-name ';'
 ///
-bool Parser::isCXXDeclarationStatement() {
+/// template-declarat

[PATCH] D127284: [WIP] [clang-repl] Support statements on global scope in incremental mode.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev updated this revision to Diff 446817.
v.g.vassilev added a comment.

clang-format


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

https://reviews.llvm.org/D127284

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Parse/Parser.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Interpreter/IncrementalParser.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/Interpreter/execute-stmts.cpp
  clang/unittests/Interpreter/InterpreterTest.cpp

Index: clang/unittests/Interpreter/InterpreterTest.cpp
===
--- clang/unittests/Interpreter/InterpreterTest.cpp
+++ clang/unittests/Interpreter/InterpreterTest.cpp
@@ -120,12 +120,7 @@
 
   // FIXME: Add support for wrapping and running statements.
   auto R2 = Interp->Parse("var1++; printf(\"var1 value %d\\n\", var1);");
-  EXPECT_FALSE(!!R2);
-  using ::testing::HasSubstr;
-  EXPECT_THAT(DiagnosticsOS.str(),
-  HasSubstr("error: unknown type name 'var1'"));
-  auto Err = R2.takeError();
-  EXPECT_EQ("Parsing failed.", llvm::toString(std::move(Err)));
+  EXPECT_TRUE(!!R2);
 }
 
 TEST(InterpreterTest, UndoCommand) {
Index: clang/test/Interpreter/execute-stmts.cpp
===
--- /dev/null
+++ clang/test/Interpreter/execute-stmts.cpp
@@ -0,0 +1,22 @@
+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+// RUN: cat %s | clang-repl | FileCheck %s
+
+int i = 12;
+++i;
+extern "C" int printf(const char *, ...);
+printf("i = %d\n", i);
+// CHECK: i = 13
+
+namespace Ns {
+void f() { i++; }
+} // namespace Ns
+Ns::f();
+
+void g() { ++i; }
+g();
+
+printf("i = %d\n", i);
+// CHECK: i = 15
+
+% quit
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -582,13 +582,14 @@
 ///
 /// Note that in C, it is an error if there is no first declaration.
 bool Parser::ParseFirstTopLevelDecl(DeclGroupPtrTy &Result,
-Sema::ModuleImportState &ImportState) {
+Sema::ModuleImportState &ImportState,
+StmtVector *Stmts) {
   Actions.ActOnStartOfTranslationUnit();
 
   // For C++20 modules, a module decl must be the first in the TU.  We also
   // need to track module imports.
   ImportState = Sema::ModuleImportState::FirstDecl;
-  bool NoTopLevelDecls = ParseTopLevelDecl(Result, ImportState);
+  bool NoTopLevelDecls = ParseTopLevelDecl(Result, ImportState, Stmts);
 
   // C11 6.9p1 says translation units must have at least one top-level
   // declaration. C++ doesn't have this restriction. We also don't want to
@@ -609,7 +610,8 @@
 ///   declaration
 /// [C++20]   module-import-declaration
 bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
-   Sema::ModuleImportState &ImportState) {
+   Sema::ModuleImportState &ImportState,
+   StmtVector *Stmts /*=nullptr*/) {
   DestroyTemplateIdAnnotationsRAIIObj CleanupRAII(*this);
 
   // Skip over the EOF token, flagging end of previous input for incremental
@@ -724,6 +726,23 @@
   ParsedAttributes attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
 
+  // FIXME: Remove the incremental processing pre-condition and verify clang
+  // still can pass its test suite, which will harden `isDeclarationStatement`.
+  // It is known to have several weaknesses, for example in
+  // isConstructorDeclarator, infinite loop in c-index-test, etc..
+  // Parse a block of top-level-stmts.
+  while (PP.isIncrementalProcessingEnabled() && Stmts &&
+ !isDeclarationStatement(/*IsParsingDecl=*/false)) {
+// isStmtExpr ? ParsedStmtContext::InStmtExpr : ParsedStmtContext()
+ParsedStmtContext SubStmtCtx = ParsedStmtContext();
+auto R = ParseStatementOrDeclaration(*Stmts, SubStmtCtx);
+if (!R.isUsable())
+  return true;
+Stmts->push_back(R.get());
+if (Tok.is(tok::eof))
+  return false;
+  }
+
   Result = ParseExternalDeclaration(attrs);
   // An empty Result might mean a line with ';' or some parsing error, ignore
   // it.
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -22,6 +22,7 @@
 ///
 /// declaration-statement:
 ///   block-declaration
+///   template-declaration
 ///
 /// block-declaration:
 ///   simple-declaration
@@ -46,12 +47,25 @@
 ///   'using' 'namespace' '::'[opt] nested-name-specifier[opt]
 /// namespace-name ';'
 ///
-bool Parser::isCXXDeclarationStatement() {
+/// template-declaration:
+///   'template

[PATCH] D129872: [clang][OpenMP] Fix runtime crash in the call to __kmp_alloc.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

Ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129872

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D112374#3666531 , @kimgr wrote:

> From a purely personal perspective, I'd prefer if this landed after the 
> branch for llvm-15.
>
> We try to co-release IWYU shortly after LLVM/Clang are released, to get a 
> public API-compatible release out there. So it would be really nice if we 
> didn't have to spend time working around AST changes while the clock is 
> ticking to get a release out the door.
>
> That said, I know we're just an out-of-tree project and the LLVM project as 
> such doesn't make any promises (and shouldn't have to!). I just thought I'd 
> raise the concern to see if anybody shares it.

So I checked out and played a bit with IWYU sources yesterday. I saw that my 
previous suggestions only fixes one of the six test breakages.

Yeah its a bit messy, I totally understand now why you seem a bit desperate 
about this haha.

Since I am not one to met around in the torture business, I concur to hold.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D125946: Handles failing driver tests of clang

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev accepted this revision.
v.g.vassilev added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D125946

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

[PATCH] D130270: [clang][dataflow] Use a dedicated bool to encode which branch was taken

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: 
clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp:439
   Context.addFlowConditionConstraint(FC2, Y);
-  // JoinedFC = (FC1 || FC2) && Z = (X || Y) && Z
-  auto &JoinedFC = Context.joinFlowConditions(FC1, FC2);
+  // JoinedFC = ((!B && FC1) || (B && FC2)) && Z = ((!B && X) || (B && Y)) && Z
+  auto &B = Context.createAtomicBoolValue();

I'm a bit concerned by the need to update this (essentially) unrelated test. It 
implies the test is depending on implementation details that it shouldn't be 
concerned with.

Not for this patch, but if you have thoughts on how to simplify this test to do 
what it needs w/o being tied to the details of how joins work, that would be 
appreciated. At least, it seems worth a FIXME.

thanks



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1202
+
+EXPECT_FALSE(Env.flowConditionImplies(FooVal));
+  });

should we also test the inverse?
```
EXPECT_FALSE(Env.flowConditionImplies(Env.makeNot(FooVal)));
```
That is, I would think we want to show that neither is provable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130270

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

[clang] 32dcb75 - [clang][dataflow] Move NoopAnalysis from unittests to include

[Sam Estep via cfe-commits]

Author: Sam Estep
Date: 2022-07-22T14:11:32Z
New Revision: 32dcb759c3005d8395b411a9aaa3d90815661d1c

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

LOG: [clang][dataflow] Move NoopAnalysis from unittests to include

This patch moves `Analysis/FlowSensitive/NoopAnalysis.h` from 
`clang/unittests/` to `clang/include/clang/`, so that we can use it for doing 
context-sensitive analysis.

Reviewed By: ymandel, gribozavr2, sgatev

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

Added: 
clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h

Modified: 
clang/docs/tools/clang-formatted-files.txt
clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
clang/unittests/Analysis/FlowSensitive/TestingSupportTest.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Removed: 
clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h



diff  --git a/clang/docs/tools/clang-formatted-files.txt 
b/clang/docs/tools/clang-formatted-files.txt
index 107d4a7f3a02d..81d0e9522e604 100644
--- a/clang/docs/tools/clang-formatted-files.txt
+++ b/clang/docs/tools/clang-formatted-files.txt
@@ -132,6 +132,7 @@ 
clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h
 clang/include/clang/Analysis/FlowSensitive/DebugSupport.h
 clang/include/clang/Analysis/FlowSensitive/MapLattice.h
 clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h
+clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
 clang/include/clang/Analysis/FlowSensitive/NoopLattice.h
 clang/include/clang/Analysis/FlowSensitive/Solver.h
 clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -634,7 +635,6 @@ 
clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
 clang/unittests/Analysis/FlowSensitive/MapLatticeTest.cpp
 clang/unittests/Analysis/FlowSensitive/MatchSwitchTest.cpp
 clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp
-clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
 clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
 clang/unittests/Analysis/FlowSensitive/SolverTest.cpp
 clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp

diff  --git a/clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h 
b/clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
similarity index 83%
rename from clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
rename to clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
index 45ed414406372..15b72211fa18c 100644
--- a/clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
@@ -6,13 +6,12 @@
 //
 
//===--===//
 //
-//  This file defines a NoopAnalysis class that is used by dataflow analysis
-//  tests.
+//  This file defines a NoopAnalysis class that just uses the builtin transfer.
 //
 
//===--===//
 
-#ifndef LLVM_CLANG_UNITTESTS_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
-#define LLVM_CLANG_UNITTESTS_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Stmt.h"
@@ -41,4 +40,4 @@ class NoopAnalysis : public DataflowAnalysis {
 } // namespace dataflow
 } // namespace clang
 
-#endif // LLVM_CLANG_UNITTESTS_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
+#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H

diff  --git a/clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp 
b/clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
index 2be1405465838..103e424755e3c 100644
--- a/clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
@@ -8,10 +8,10 @@
 // FIXME: Move this to clang/unittests/Analysis/FlowSensitive/Models.
 
 #include "clang/Analysis/FlowSensitive/Models/ChromiumCheckModel.h"
-#include "NoopAnalysis.h"
 #include "TestingSupport.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringExtras.h"

diff  --git 
a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp 
b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 434bc6b8042ee..ae70131a60a57 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensiti

[PATCH] D130304: [clang][dataflow] Move NoopAnalysis from unittests to include

[Sam Estep via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG32dcb759c300: [clang][dataflow] Move NoopAnalysis from 
unittests to include (authored by samestep).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130304

Files:
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
  clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
  clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
  clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h
  clang/unittests/Analysis/FlowSensitive/TestingSupportTest.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
  clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -6,7 +6,6 @@
 //
 //===--===//
 
-#include "NoopAnalysis.h"
 #include "TestingSupport.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/ExprCXX.h"
@@ -18,6 +17,7 @@
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
 #include "clang/Tooling/Tooling.h"
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -6,13 +6,13 @@
 //
 //===--===//
 
-#include "NoopAnalysis.h"
 #include "TestingSupport.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/LangStandard.h"
Index: clang/unittests/Analysis/FlowSensitive/TestingSupportTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupportTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TestingSupportTest.cpp
@@ -1,8 +1,8 @@
 #include "TestingSupport.h"
-#include "NoopAnalysis.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
Index: clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -7,9 +7,9 @@
 //===--===//
 
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
-#include "NoopAnalysis.h"
 #include "TestingSupport.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
 #include "gmock/gmock.h"
Index: clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/ChromiumCheckModelTest.cpp
@@ -8,10 +8,10 @@
 // FIXME: Move this to clang/unittests/Analysis/FlowSensitive/Models.
 
 #include "clang/Analysis/FlowSensitive/Models/ChromiumCheckModel.h"
-#include "NoopAnalysis.h"
 #include "TestingSupport.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringExtras.h"
Index: clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
+++ clan

[PATCH] D130270: [clang][dataflow] Use a dedicated bool to encode which branch was taken

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.



Comment at: 
clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp:439
   Context.addFlowConditionConstraint(FC2, Y);
-  // JoinedFC = (FC1 || FC2) && Z = (X || Y) && Z
-  auto &JoinedFC = Context.joinFlowConditions(FC1, FC2);
+  // JoinedFC = ((!B && FC1) || (B && FC2)) && Z = ((!B && X) || (B && Y)) && Z
+  auto &B = Context.createAtomicBoolValue();

ymandel wrote:
> I'm a bit concerned by the need to update this (essentially) unrelated test. 
> It implies the test is depending on implementation details that it shouldn't 
> be concerned with.
> 
> Not for this patch, but if you have thoughts on how to simplify this test to 
> do what it needs w/o being tied to the details of how joins work, that would 
> be appreciated. At least, it seems worth a FIXME.
> 
> thanks
Agreed, this test seems a bit awkward; in this patch I was just focusing on 
updating it instead of rethinking it.



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1202
+
+EXPECT_FALSE(Env.flowConditionImplies(FooVal));
+  });

ymandel wrote:
> should we also test the inverse?
> ```
> EXPECT_FALSE(Env.flowConditionImplies(Env.makeNot(FooVal)));
> ```
> That is, I would think we want to show that neither is provable.
Sure, that makes sense; if I remember correctly, the inverse of `FooVal` is 
indeed not provable already. You're right that it would be good to test both 
here, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130270

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

[PATCH] D130305: [clang][dataflow] Refactor ApplyBuiltinTransfer field out into DataflowAnalysisOptions struct

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:66
   explicit DataflowAnalysis(ASTContext &Context) : Context(Context) {}
   explicit DataflowAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
   : TypeErasedDataflowAnalysis(ApplyBuiltinTransfer), Context(Context) {}

gribozavr2 wrote:
> Will you remove this bool overload in a later patch?
I didn't have any plans to, but I'm fine with removing it in a later patch if 
people want to.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:38
+  // analysis).
+  bool ApplyBuiltinTransfer;
+};

gribozavr2 wrote:
> xazax.hun wrote:
> > I think it might be better to have a default value.
> +1, uninitialized data is tricky to handle -- especially if we add more 
> options in future.
Makes sense, will do!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130305

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

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

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

https://lab.llvm.org/buildbot#builders/74/builds/12262 here is a failure with 
only me in the blamelist, but I cannot see how those failures have anything to 
do with this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117977

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

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

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

@tstellar Thank you! That is an error I do understand, and yes your example is 
exactly right.

Sorry, I thought I had checked every rebase that no new usages cropped up, but 
I guess I missed one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117977

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

[PATCH] D130305: [clang][dataflow] Refactor ApplyBuiltinTransfer field out into DataflowAnalysisOptions struct

[Sam Estep via Phabricator via cfe-commits]

samestep updated this revision to Diff 446829.
samestep added a comment.

Give a default value to ApplyBuiltinTransfer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130305

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h


Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -30,6 +30,14 @@
 namespace clang {
 namespace dataflow {
 
+struct DataflowAnalysisOptions {
+  /// Determines whether to apply the built-in transfer functions.
+  // FIXME: Remove this option once the framework supports composing analyses
+  // (at which point the built-in transfer functions can be simply a standalone
+  // analysis).
+  bool ApplyBuiltinTransfer = true;
+};
+
 /// Type-erased lattice element container.
 ///
 /// Requirements:
@@ -42,16 +50,14 @@
 
 /// Type-erased base class for dataflow analyses built on a single lattice 
type.
 class TypeErasedDataflowAnalysis : public Environment::ValueModel {
-  /// Determines whether to apply the built-in transfer functions.
-  // FIXME: Remove this option once the framework supports composing analyses
-  // (at which point the built-in transfer functions can be simply a standalone
-  // analysis).
-  bool ApplyBuiltinTransfer;
+  DataflowAnalysisOptions Options;
 
 public:
-  TypeErasedDataflowAnalysis() : ApplyBuiltinTransfer(true) {}
+  TypeErasedDataflowAnalysis() : Options({}) {}
   TypeErasedDataflowAnalysis(bool ApplyBuiltinTransfer)
-  : ApplyBuiltinTransfer(ApplyBuiltinTransfer) {}
+  : Options({ApplyBuiltinTransfer}) {}
+  TypeErasedDataflowAnalysis(DataflowAnalysisOptions Options)
+  : Options(Options) {}
 
   virtual ~TypeErasedDataflowAnalysis() {}
 
@@ -80,7 +86,7 @@
 
   /// Determines whether to apply the built-in transfer functions, which model
   /// the heap and stack in the `Environment`.
-  bool applyBuiltinTransfer() const { return ApplyBuiltinTransfer; }
+  bool applyBuiltinTransfer() const { return Options.ApplyBuiltinTransfer; }
 };
 
 /// Type-erased model of the program at a given program point.
Index: clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
@@ -32,6 +32,9 @@
   : DataflowAnalysis(Context,
 ApplyBuiltinTransfer) {}
 
+  NoopAnalysis(ASTContext &Context, DataflowAnalysisOptions Options)
+  : DataflowAnalysis(Context, Options) {}
+
   static NoopLattice initialElement() { return {}; }
 
   void transfer(const Stmt *S, NoopLattice &E, Environment &Env) {}
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -65,6 +65,9 @@
   explicit DataflowAnalysis(ASTContext &Context) : Context(Context) {}
   explicit DataflowAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
   : TypeErasedDataflowAnalysis(ApplyBuiltinTransfer), Context(Context) {}
+  explicit DataflowAnalysis(ASTContext &Context,
+DataflowAnalysisOptions Options)
+  : TypeErasedDataflowAnalysis(Options), Context(Context) {}
 
   ASTContext &getASTContext() final { return Context; }
 


Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -30,6 +30,14 @@
 namespace clang {
 namespace dataflow {
 
+struct DataflowAnalysisOptions {
+  /// Determines whether to apply the built-in transfer functions.
+  // FIXME: Remove this option once the framework supports composing analyses
+  // (at which point the built-in transfer functions can be simply a standalone
+  // analysis).
+  bool ApplyBuiltinTransfer = true;
+};
+
 /// Type-erased lattice element container.
 ///
 /// Requirements:
@@ -42,16 +50,14 @@
 
 /// Type-erased base class for dataflow analyses built on a single lattice type.
 class TypeErasedDataflowAnalysis : public Environment::ValueModel {
-  /// Determines whether to apply the built-in transfer functions.
-  // FIXME: Remove this option once the framework supports composing analyses
-  // (at which point the built-in transfer func

[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

[Sam Estep via Phabricator via cfe-commits]

samestep updated this revision to Diff 446830.
samestep added a comment.

Update parent patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/include/clang/Analysis/FlowSensitive/Transfer.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -39,14 +39,14 @@
 
 template 
 void runDataflow(llvm::StringRef Code, Matcher Match,
-  LangStandard::Kind Std = LangStandard::lang_cxx17,
-  bool ApplyBuiltinTransfer = true,
-  llvm::StringRef TargetFun = "target") {
+ DataflowAnalysisOptions Options,
+ LangStandard::Kind Std = LangStandard::lang_cxx17,
+ llvm::StringRef TargetFun = "target") {
   ASSERT_THAT_ERROR(
   test::checkDataflow(
   Code, TargetFun,
-  [ApplyBuiltinTransfer](ASTContext &C, Environment &) {
-return NoopAnalysis(C, ApplyBuiltinTransfer);
+  [Options](ASTContext &C, Environment &) {
+return NoopAnalysis(C, Options);
   },
   [&Match](
   llvm::ArrayRef<
@@ -54,12 +54,19 @@
   Results,
   ASTContext &ASTCtx) { Match(Results, ASTCtx); },
   {"-fsyntax-only", "-fno-delayed-template-parsing",
-"-std=" +
-std::string(
-LangStandard::getLangStandardForKind(Std).getName())}),
+   "-std=" + std::string(
+ LangStandard::getLangStandardForKind(Std).getName())}),
   llvm::Succeeded());
 }
 
+template 
+void runDataflow(llvm::StringRef Code, Matcher Match,
+ LangStandard::Kind Std = LangStandard::lang_cxx17,
+ bool ApplyBuiltinTransfer = true,
+ llvm::StringRef TargetFun = "target") {
+  runDataflow(Code, Match, {ApplyBuiltinTransfer}, Std, TargetFun);
+}
+
 TEST(TransferTest, IntVarDeclNotTrackedWhenTransferDisabled) {
   std::string Code = R"(
 void target() {
@@ -3862,4 +3869,94 @@
   });
 }
 
+TEST(TransferTest, ContextSensitiveOptionDisabled) {
+  std::string Code = R"(
+bool GiveBool();
+void SetBool(bool &Var) { Var = true; }
+
+void target() {
+  bool Foo = GiveBool();
+  SetBool(Foo);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext &ASTCtx) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment &Env = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+auto &FooVal =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_FALSE(Env.flowConditionImplies(FooVal));
+  },
+  {/*.ApplyBuiltinTransfer=*/true,
+   /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/false}});
+}
+
+TEST(TransferTest, ContextSensitiveSetTrue) {
+  std::string Code = R"(
+bool GiveBool();
+void SetBool(bool &Var) { Var = true; }
+
+void target() {
+  bool Foo = GiveBool();
+  SetBool(Foo);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext &ASTCtx) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment &Env = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+auto &FooVal =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  },
+  {/*.ApplyBuiltinTransfer=*/true,
+   /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}});
+}
+
+TEST(TransferTest, ContextSensitiveSetFalse) {
+  std::string Code = R"(
+bool GiveBool();
+void SetBool(bool &Var) { Var = false; }
+
+void target() {
+  bool Foo = GiveBool();
+  SetBool(Foo);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+  

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

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

Will make patch forit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117977

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

[PATCH] D130362: Fix one stray `{LLVM -> CLANG}_TOOLS_INSTALL_DIR`

[John Ericson via Phabricator via cfe-commits]

Ericson2314 created this revision.
Ericson2314 added a reviewer: tstellar.
Herald added a subscriber: mgorny.
Herald added a project: All.
Ericson2314 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Follow up to D117977 , where I missed this 
new usage after one rebase.

Thanks @tsteller in https://reviews.llvm.org/D117977#3670919 for
noticing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130362

Files:
  clang/utils/hmaptool/CMakeLists.txt


Index: clang/utils/hmaptool/CMakeLists.txt
===
--- clang/utils/hmaptool/CMakeLists.txt
+++ clang/utils/hmaptool/CMakeLists.txt
@@ -1,4 +1,4 @@
-install(PROGRAMS hmaptool DESTINATION "${LLVM_TOOLS_INSTALL_DIR}" COMPONENT 
hmaptool)
+install(PROGRAMS hmaptool DESTINATION "${CLANG_TOOLS_INSTALL_DIR}" COMPONENT 
hmaptool)
 add_custom_target(hmaptool ALL DEPENDS "hmaptool")
 set_target_properties(hmaptool PROPERTIES FOLDER "Utils")
 


Index: clang/utils/hmaptool/CMakeLists.txt
===
--- clang/utils/hmaptool/CMakeLists.txt
+++ clang/utils/hmaptool/CMakeLists.txt
@@ -1,4 +1,4 @@
-install(PROGRAMS hmaptool DESTINATION "${LLVM_TOOLS_INSTALL_DIR}" COMPONENT hmaptool)
+install(PROGRAMS hmaptool DESTINATION "${CLANG_TOOLS_INSTALL_DIR}" COMPONENT hmaptool)
 add_custom_target(hmaptool ALL DEPENDS "hmaptool")
 set_target_properties(hmaptool PROPERTIES FOLDER "Utils")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130305: [clang][dataflow] Refactor ApplyBuiltinTransfer field out into DataflowAnalysisOptions struct

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:66
   explicit DataflowAnalysis(ASTContext &Context) : Context(Context) {}
   explicit DataflowAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
   : TypeErasedDataflowAnalysis(ApplyBuiltinTransfer), Context(Context) {}

samestep wrote:
> gribozavr2 wrote:
> > Will you remove this bool overload in a later patch?
> I didn't have any plans to, but I'm fine with removing it in a later patch if 
> people want to.
I think it makes sense to remove it. Can you already document that it's 
deprecated in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130305

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

[PATCH] D130305: [clang][dataflow] Refactor ApplyBuiltinTransfer field out into DataflowAnalysisOptions struct

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:66
   explicit DataflowAnalysis(ASTContext &Context) : Context(Context) {}
   explicit DataflowAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
   : TypeErasedDataflowAnalysis(ApplyBuiltinTransfer), Context(Context) {}

sgatev wrote:
> samestep wrote:
> > gribozavr2 wrote:
> > > Will you remove this bool overload in a later patch?
> > I didn't have any plans to, but I'm fine with removing it in a later patch 
> > if people want to.
> I think it makes sense to remove it. Can you already document that it's 
> deprecated in this patch?
Sure, I can document that; what's the best way to do so? Is there a special way 
to say something is deprecated in a `///` comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130305

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

[PATCH] D130168: [CUDA] remove duplicate condition

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D130168#3667283 , @VitalyR wrote:

> @yaxunl Hi! Could you commit this on my behalf? I read the documentation and 
> it seems the appropriate way to commit changes for a newcomer not having 
> commit access like me. My name is "VitalyR" and my email address is 
> "vitalya...@gmail.com". Thanks!

sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130168

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

[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/AST/ASTImporter.cpp:3237
+  ParentMapContext &ParentC = DC->getParentASTContext().getParentMapContext();
+  DynTypedNodeList P = ParentC.getParents(*S);
+  while (!P.empty()) {

martong wrote:
> martong wrote:
> > 
> The first call of `getParents` will create the parent map, via a full-blown 
> AST visitation. I am concerned a bit about the additional performance 
> overhead. Could you please run some measurements? (E.g. a CTU run on 
> `protobuf` and `bitcoin` with our internal CI infra)
> The first call of `getParents` will create the parent map, via a full-blown 
> AST visitation. I am concerned a bit about the additional performance 
> overhead. Could you please run some measurements? (E.g. a CTU run on 
> `protobuf` and `bitcoin` with our internal CI infra)

I've seen the measurement results, they look good.
{F23872820}

We have errors in the CTU analysis of 'qtbase' and 'contour' even with the base 
line. The duration for the remaining 3 (xerces, protobuf, bincoin) looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129640

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

[PATCH] D130363: [clang] Give priority to Class context while parsing declarations

[Furkan via Phabricator via cfe-commits]

furkanusta created this revision.
Herald added subscribers: usaxena95, kadircet.
Herald added a project: All.
furkanusta requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

Fixes: https://github.com/clangd/clangd/issues/290


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130363

Files:
  clang/lib/Parse/ParseDecl.cpp


Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -3266,13 +3266,13 @@
 return;
   }
 
-  if (getCurScope()->getFnParent() || getCurScope()->getBlockParent())
-CCC = Sema::PCC_LocalDeclarationSpecifiers;
-  else if (TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate)
+  if (TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate)
 CCC = DSContext == DeclSpecContext::DSC_class ? 
Sema::PCC_MemberTemplate
   : Sema::PCC_Template;
   else if (DSContext == DeclSpecContext::DSC_class)
 CCC = Sema::PCC_Class;
+  else if (getCurScope()->getFnParent() || getCurScope()->getBlockParent())
+CCC = Sema::PCC_LocalDeclarationSpecifiers;
   else if (CurParsedObjCImpl)
 CCC = Sema::PCC_ObjCImplementation;
 


Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -3266,13 +3266,13 @@
 return;
   }
 
-  if (getCurScope()->getFnParent() || getCurScope()->getBlockParent())
-CCC = Sema::PCC_LocalDeclarationSpecifiers;
-  else if (TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate)
+  if (TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate)
 CCC = DSContext == DeclSpecContext::DSC_class ? Sema::PCC_MemberTemplate
   : Sema::PCC_Template;
   else if (DSContext == DeclSpecContext::DSC_class)
 CCC = Sema::PCC_Class;
+  else if (getCurScope()->getFnParent() || getCurScope()->getBlockParent())
+CCC = Sema::PCC_LocalDeclarationSpecifiers;
   else if (CurParsedObjCImpl)
 CCC = Sema::PCC_ObjCImplementation;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129872: [clang][OpenMP] Fix runtime crash in the call to __kmp_alloc.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Why does it get truncated if the type must be integer? Probably, something 
incorrect in sema.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129872

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

[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

[Sam Estep via Phabricator via cfe-commits]

samestep added a comment.

In D130306#3670259 , @xazax.hun wrote:

> There are many ways to introduce context sensitivity into the framework, this 
> patch seems to take the "inline substitution" approach, the same approach the 
> Clang Static Analyzer is taking. While this approach is relatively easy to 
> implement and has great precision, it also has some scalability problems. Did 
> you also consider a summary-based approach? In general, I believe the inline 
> substitution approach results in an easier to use interface for the users of 
> the framework, but I am a bit concerned about the scalability problems.

Good point, thanks! Yes, we considered a summary-based approach, but we decided 
not to use it because (as you mentioned) it would be much more difficult to 
implement, especially for callees with nontrivial CFGs, which would result in a 
nontrivial flow condition instead of just `Value`s in the `Environment`.

Could you elaborate on what specific scalability problems you are concerned 
about? The main one that comes to mind for me is unpredictable cost due to the 
potential for arbitrary callee bodies to be present in the translation unit. 
While this particular patch doesn't address that concern, we definitely have 
plans to do so: I'm guessing that will take the form of providing the analysis 
an allowlist of symbols of which it is allowed to analyze the bodies, so it 
would treat any symbols not in that list as if their bodies are not available 
in the TU regardless.

To clarify, the main reason we want context-sensitive analysis is to allow us 
to simplify our definitions of some models, such as the `optional` checker 
model. The goal is to provide the analysis a mock implementation of an 
`optional` type, and then use context-sensitive analysis (probably just one or 
two layers deep) to model the constructors and methods.

> Some other related questions:
>
> - Why call noop analysis? As far as I understand, this would only update the 
> environment but not the lattice of the current analysis, i.e., if the 
> analysis is computing some information like liveness, that information would 
> not be context sensitive. Do I miss something?

The alternative in this case would be to use the same analysis for the callee 
that is already being used for the caller? I agree that would be nicer to serve 
a broader category of use cases. I didn't do that in this patch for a couple 
reasons:

1. In the short term, that would require threading more information through to 
the builtin transfer function, while this patch was meant to just be a minimum 
viable product.
2. In the longer term, we probably don't need that for our specific goals (just 
modeling simple fields of mock classes) mentioned above.

However, if you have a suggestion for a way to construct an instance of the 
outer analysis here, that would definitely be useful.

> - Why limit the call depth to 1? The patch mentions recursive functions. In 
> case of the Clang Static Analyzer, the call depth is 4. I think if we go with 
> the inline substitution approach, we want this parameter to be tunable, 
> because different analyses might have different sweet spots for the call 
> stack depth.

There's no particular reason for this. We plan to support more call stack depth 
soon. This would probably make sense as a field in the `TransferOptions` struct.

> - The CSA also has other tunables, e.g., small functions are always inlined 
> and large functions are never inlined.

See my response earlier about an allowlist for symbols to inline.

> - Currently, it looks like the framework assumes functions that cannot be 
> inlined are not doing anything. This is an unsound assumption, and I wonder 
> if we should change that before we try to settle on the best values for the 
> tunable parameters.

Agreed, this assumption is unsound. However, the framework already makes many 
other unsound assumptions in a similar spirit, so this one doesn't immediately 
strike me as one that needs to change. I'll defer to people with more context.

> - The current code might do a bit too much work. E.g. consider:
>
>   while (...) {
> inlinableCall();
>   }
>
> As far as I understand, the current approach would start the analysis of 
> `inlinableCall` from scratch in each iteration. I wonder if we actually want 
> to preserve the state between the iterations, so we do not always reevaluate 
> the call from scratch. Currently, it might not be a big deal as the 
> fixed-point iteration part is disabled. But this could be a perf problem in 
> the future, unless I miss something.

Agreed, this is somewhat wasteful. Note that not everything is thrown away, 
because the same `DataflowAnalysisContext` is reused when analyzing the callee. 
Still, we would like to handle this in a smarter way in the future, as you 
mention. For now, though, while just building up functionality behind a feature 
flag, we don't plan to worry as much abo

[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:208
+
+  // TODO: Currently this only works if the callee is never a method and the
+  // same callee is never analyzed from multiple separate callsites. To

ymandel wrote:
> 
OK, I'll change this; would you like for me to replace all the other `TODO`s 
with `FIXME`s, as well?



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220
+  assert(Body != nullptr);
+  initGlobalVars(*Body, Env);
+

ymandel wrote:
> I wonder how this will work between caller and callee. Do we need separate 
> global var state in the frame? If so, maybe mention that as well in the FIXME 
> above.
Could you clarify what you mean? Perhaps I just don't understand exactly what 
is meant by "global vars" here.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:237
+const Expr *Arg = *ArgIt;
+// TODO: Confirm that this is the correct `SkipPast` to use here.
+auto *ArgLoc = Env.getStorageLocation(*Arg, SkipPast::None);

ymandel wrote:
> I'm pretty sure we want `SkipPast::Reference`. That will ensure that the 
> parameter and argument share the same underlying location. Otherwise, in the 
> case of references, the parameter will point to the reference location object 
> rather than just directly to the location.
OK, thank you! I'll make that change.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:517
+
+  // TODO: Cache these CFGs.
+  auto CFCtx = ControlFlowContext::build(F, F->getBody(), &ASTCtx);

ymandel wrote:
> here and below: s/TODO/FIXME.
Will do.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:524
+  auto CalleeEnv = Env.pushCall(S);
+  auto Analysis = NoopAnalysis(ASTCtx, /*ApplyBuiltinTransfer=*/true);
+

ymandel wrote:
> This seems worth a FIXME or, at least, an explanation.  It implies that with 
> the current design, we can't support general-purpose analyses, which we 
> should probably fix. Given our goal of supporting models that don't involve 
> specialized lattices, I think this is a good compromise for the short term, 
> but not a stable solution for the framework (hence  FIXME sounds right).
Good point, and @xazax.hun pointed this out as well. I'll add a `FIXME` here, 
at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306

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

[PATCH] D129864: [Flang] Generate documentation for compiler flags

[Dylan Fleming via Phabricator via cfe-commits]

DylanFleming-arm updated this revision to Diff 446840.
DylanFleming-arm added a comment.

After pushing to main, this patch cause a buildbot failure as 
CLANG_TABLEGEN_EXE could not be found.

I've updated flang/docs/CMakeLists.txt to set the parameter before making a 
call to clang_tablegen


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129864

Files:
  clang/utils/TableGen/ClangOptionDocEmitter.cpp
  flang/docs/CMakeLists.txt
  flang/docs/index.md
  flang/include/flang/FlangOptionsDocs.td

Index: flang/include/flang/FlangOptionsDocs.td
===
--- /dev/null
+++ flang/include/flang/FlangOptionsDocs.td
@@ -0,0 +1,35 @@
+//==--- FlangOptionDocs.td - Option documentation -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+def GlobalDocumentation {
+  code Intro =[{..
+  ---
+  NOTE: This file is automatically generated by running clang-tblgen
+  -gen-opt-docs. Do not edit this file by hand!!
+  ---
+
+=
+Flang command line argument reference
+=
+.. contents::
+   :local:
+
+Introduction
+
+
+}];
+
+  string Program = "flang";
+
+  list ExcludedFlags = [];
+  list IncludedFlags = ["FlangOption"];
+
+}
+
+
+include "../../../clang/include/clang/Driver/Options.td"
Index: flang/docs/index.md
===
--- flang/docs/index.md
+++ flang/docs/index.md
@@ -45,6 +45,7 @@
DoConcurrent
Extensions
FIRLangRef
+   FlangCommandLineReference
FlangDriver
FortranIR
FortranLLVMTestSuite
Index: flang/docs/CMakeLists.txt
===
--- flang/docs/CMakeLists.txt
+++ flang/docs/CMakeLists.txt
@@ -91,6 +91,16 @@
 endif()
 endif()
 
+function (gen_rst_file_from_td output_file td_option source docs_target)
+  if (NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${source}")
+message(FATAL_ERROR "Cannot find source file: ${source} in ${CMAKE_CURRENT_SOURCE_DIR}")
+  endif()
+  get_filename_component(TABLEGEN_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/${source}" DIRECTORY)
+  list(APPEND LLVM_TABLEGEN_FLAGS "-I${TABLEGEN_INCLUDE_DIR}")
+  clang_tablegen(Source/${output_file} ${td_option} SOURCE ${source} TARGET "gen-${output_file}")
+  add_dependencies(${docs_target} "gen-${output_file}")
+endfunction()
+
 if (LLVM_ENABLE_SPHINX)
   include(AddSphinxTarget)
   if (SPHINX_FOUND)
@@ -108,12 +118,15 @@
 "${CMAKE_CURRENT_BINARY_DIR}/Source"
 DEPENDS flang-doc)
 
-# Runs a python script prior to HTML generation to prepend a header to FIRLangRef,
-# Without the header, the page is incorrectly formatted, as it assumes the first entry is the page title.
-add_custom_command(TARGET copy-flang-src-docs
-  COMMAND "${Python3_EXECUTABLE}"
-  ARGS ${CMAKE_CURRENT_BINARY_DIR}/Source/FIR/CreateFIRLangRef.py)
+  # Runs a python script prior to HTML generation to prepend a header to FIRLangRef,
+  # Without the header, the page is incorrectly formatted, as it assumes the first entry is the page title.
+  add_custom_command(TARGET copy-flang-src-docs
+COMMAND "${Python3_EXECUTABLE}"
+ARGS ${CMAKE_CURRENT_BINARY_DIR}/Source/FIR/CreateFIRLangRef.py)
+
 
+  set(CLANG_TABLEGEN_EXE clang-tblgen)
+  gen_rst_file_from_td(FlangCommandLineReference.rst -gen-opt-docs ../include/flang/FlangOptionsDocs.td docs-flang-html)
 endif()
 if (${SPHINX_OUTPUT_MAN})
   add_sphinx_target(man flang)
Index: clang/utils/TableGen/ClangOptionDocEmitter.cpp
===
--- clang/utils/TableGen/ClangOptionDocEmitter.cpp
+++ clang/utils/TableGen/ClangOptionDocEmitter.cpp
@@ -168,6 +168,29 @@
   return false;
 }
 
+bool isIncluded(const Record *OptionOrGroup, const Record *DocInfo) {
+  assert(DocInfo->getValue("IncludedFlags") && "Missing includeFlags");
+  for (StringRef Inclusion : DocInfo->getValueAsListOfStrings("IncludedFlags"))
+if (hasFlag(OptionOrGroup, Inclusion))
+  return true;
+  return false;
+}
+
+bool isGroupIncluded(const DocumentedGroup &Group, const Record *DocInfo) {
+  if (isIncluded(Group.Group, DocInfo))
+return true;
+  for (auto &O : Group.Options)
+if (isIncluded(O.Option, DocInfo))
+  return true;
+  for (auto &G : Group.Groups) {
+if (isIncluded(G.Group, DocInfo))
+  return true;
+if (isGroupIncluded(G, DocInfo))
+   

[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:208
+
+  // TODO: Currently this only works if the callee is never a method and the
+  // same callee is never analyzed from multiple separate callsites. To

samestep wrote:
> ymandel wrote:
> > 
> OK, I'll change this; would you like for me to replace all the other `TODO`s 
> with `FIXME`s, as well?
Just those in this patch.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220
+  assert(Body != nullptr);
+  initGlobalVars(*Body, Env);
+

samestep wrote:
> ymandel wrote:
> > I wonder how this will work between caller and callee. Do we need separate 
> > global var state in the frame? If so, maybe mention that as well in the 
> > FIXME above.
> Could you clarify what you mean? Perhaps I just don't understand exactly what 
> is meant by "global vars" here.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp#L131-L135

```
/// Initializes global storage values that are declared or referenced from
/// sub-statements of `S`.
// FIXME: Add support for resetting globals after function calls to enable
// the implementation of sound analyses.
```
Since this already mentions a need to reset after function calls, seemed 
relevant here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306

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

[PATCH] D130305: [clang][dataflow] Refactor ApplyBuiltinTransfer field out into DataflowAnalysisOptions struct

[Sam Estep via Phabricator via cfe-commits]

samestep updated this revision to Diff 446841.
samestep added a comment.

Mark bool constructors as deprecated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130305

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h


Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -30,6 +30,14 @@
 namespace clang {
 namespace dataflow {
 
+struct DataflowAnalysisOptions {
+  /// Determines whether to apply the built-in transfer functions.
+  // FIXME: Remove this option once the framework supports composing analyses
+  // (at which point the built-in transfer functions can be simply a standalone
+  // analysis).
+  bool ApplyBuiltinTransfer = true;
+};
+
 /// Type-erased lattice element container.
 ///
 /// Requirements:
@@ -42,16 +50,17 @@
 
 /// Type-erased base class for dataflow analyses built on a single lattice 
type.
 class TypeErasedDataflowAnalysis : public Environment::ValueModel {
-  /// Determines whether to apply the built-in transfer functions.
-  // FIXME: Remove this option once the framework supports composing analyses
-  // (at which point the built-in transfer functions can be simply a standalone
-  // analysis).
-  bool ApplyBuiltinTransfer;
+  DataflowAnalysisOptions Options;
 
 public:
-  TypeErasedDataflowAnalysis() : ApplyBuiltinTransfer(true) {}
+  TypeErasedDataflowAnalysis() : Options({}) {}
+
+  /// Deprecated. Use the `DataflowAnalysisOptions` constructor instead.
   TypeErasedDataflowAnalysis(bool ApplyBuiltinTransfer)
-  : ApplyBuiltinTransfer(ApplyBuiltinTransfer) {}
+  : Options({ApplyBuiltinTransfer}) {}
+
+  TypeErasedDataflowAnalysis(DataflowAnalysisOptions Options)
+  : Options(Options) {}
 
   virtual ~TypeErasedDataflowAnalysis() {}
 
@@ -80,7 +89,7 @@
 
   /// Determines whether to apply the built-in transfer functions, which model
   /// the heap and stack in the `Environment`.
-  bool applyBuiltinTransfer() const { return ApplyBuiltinTransfer; }
+  bool applyBuiltinTransfer() const { return Options.ApplyBuiltinTransfer; }
 };
 
 /// Type-erased model of the program at a given program point.
Index: clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/NoopAnalysis.h
@@ -24,13 +24,17 @@
 
 class NoopAnalysis : public DataflowAnalysis {
 public:
+  /// Deprecated. Use the `DataflowAnalysisOptions` constructor instead.
+  NoopAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
+  : DataflowAnalysis(Context,
+ApplyBuiltinTransfer) {}
+
   /// `ApplyBuiltinTransfer` controls whether to run the built-in transfer
   /// functions that model memory during the analysis. Their results are not
   /// used by `NoopAnalysis`, but tests that need to inspect the environment
   /// should enable them.
-  NoopAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
-  : DataflowAnalysis(Context,
-ApplyBuiltinTransfer) {}
+  NoopAnalysis(ASTContext &Context, DataflowAnalysisOptions Options)
+  : DataflowAnalysis(Context, Options) {}
 
   static NoopLattice initialElement() { return {}; }
 
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -63,9 +63,15 @@
   using Lattice = LatticeT;
 
   explicit DataflowAnalysis(ASTContext &Context) : Context(Context) {}
+
+  /// Deprecated. Use the `DataflowAnalysisOptions` constructor instead.
   explicit DataflowAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
   : TypeErasedDataflowAnalysis(ApplyBuiltinTransfer), Context(Context) {}
 
+  explicit DataflowAnalysis(ASTContext &Context,
+DataflowAnalysisOptions Options)
+  : TypeErasedDataflowAnalysis(Options), Context(Context) {}
+
   ASTContext &getASTContext() final { return Context; }
 
   TypeErasedLattice typeErasedInitialElement() final {


Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -30,6 +30,14 @@