[PATCH] D43159: [libc++] Replace several uses of 0 by nullptr

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43159

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

[PATCH] D90944: [clang-tidy] implement concurrency-mt-unsafe

[Roman Lebedev via Phabricator via cfe-commits]

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

Please upload all patches with full context (`-U`)
I'm guessing you'll need help committing this, in which case please specify 
`Author ` to be used for `git commit --author="<>"`

Thanks, looks good to me then.
Does anyone else have any further thoughts here?

In D90944#2418864 , @segoon wrote:

> In D90944#2418845 , @lebedev.ri 
> wrote:
>
>> In D90944#2418792 , @segoon wrote:
>>
>>> - mark mt-unsafe decls and check for marks in exprs
>>
>> Eeeh.
>> I was thinking of either some smart matcher "match any function declaration
>> with name from these lists, and then match every call to said decl".
>
> I tried to utilize bind()+equalsBoundNode(), but it seems it's impossible to 
> mark and use the mark in a single matcher.
>
>> But the current implementation, i'm not sure this approach is even legal for 
>> checks.
>
> The trick is stolen from abseil/UpgradeDurationConversionsCheck.h. If it's 
> invalid here, then abseil should be fixed too.

The problem with that approach isn't so much that it stores state in check (i 
guess that is fine in general - see preprocessor handling callbacks),
but that

1. is it guaranteed that we'll match all the decl before first callinst to that 
decl?
2. and what's worse, this approach wouldn't actually solve the problem, because 
it still matches all the callinsts, it just caches "is bad callee" check 
somewhat..


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

https://reviews.llvm.org/D90944

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

[PATCH] D91656: [clang-tidy] add concurrency module

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D91656#2418790 , @segoon wrote:

> - fix sort order

Please always upload all patches with full context (`-U`)


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

https://reviews.llvm.org/D91656

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

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

> What I'm asking specifically is: this feature has a cost, how important is 
> supporting it? Are there codebases where these attributes are widely used, 
> and enforcement at development time is particularly valuable?

FWIW, one of the codebases I work on uses a clang plugin to report a number of 
additional diagnostics, and I think it would be quite useful to have those show 
up as you're editing (comparable to clang-tidy diagnostics, except even more 
relevant as they're domain-specific).

I think making plugin support opt-in (including by requiring the clangd flags 
to mention the plugin, as suggested) would be fine. I think it's also fine to 
make clangd's support for plugins best-effort, in the sense that if a plugin 
doesn't play nicely in one of the mentioned ways (e.g. it uses static storage, 
or does not expect SkipFunctionBodies), the onus is on the plugin to fix those 
behaviours.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added a comment.

> clangd (and other clang tools) get deployed in environments where all access 
> to the filesystem goes through a llvm::vfs::Filesystem, and all filenames 
> referred to in the compile command are within that virtual filesystem rather 
> than the real one.
> (Again, this isn't an issue if we require these paths to be specified on the 
> *clangd* startup command line, as opposed to the clang flags that form the 
> compile command)

Yes I know how clang manager files through vfs, it just that loading libraries 
does not involve vfs at all, the path of a lib is passed directly to the system 
level API (eg, dlopen on Linux, System::Load on Windows), so I still can't 
understand what you are concerning, maybe you could point out a more specific 
example? Or, could you help to point out what's the difference between passing 
a plugin path through *clangd* startup command line and through clang flags?




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3905
+std::string Error;
+if (llvm::sys::DynamicLibrary::LoadLibraryPermanently(Path.c_str(), 
))
+  Diags.Report(diag::err_fe_unable_to_load_plugin) << Path << Error;

sammccall wrote:
> psionic12 wrote:
> > sammccall wrote:
> > > is this threadsafe on all platforms?
> > While I can't tell, clang driver use this for a while and seems no problem, 
> > could you help to point out what's the worst case your concerned about?
> clang-driver isn't multithreaded, so wouldn't show up problems here.
> 
> I'm concerned that if thread A loads plugin X, thread B loads plugin X, and 
> thread C loads plugin Y, all concurrently and using this code path, whether 
> they will all load correctly.
> 
> In clangd these threads could correspond to two open files and a 
> background-index worker.
> 
> (I don't know much about dynamic loading, this may be totally fine)
In this case I can make sure multiple thread works just fine with 
`LoadLibraryPermanently`, I checked all the dynamic loading API on most 
platforms and all of the are thread-safe(it's rare to see system APIs which are 
not thread safe, to me).



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3914
+if (P->getActionType() == PluginASTAction::ReplaceAction) {
+  Res.getFrontendOpts().ProgramAction = clang::frontend::PluginAction;
+  Res.getFrontendOpts().ActionName = Plugin.getName().str();

sammccall wrote:
> psionic12 wrote:
> > sammccall wrote:
> > > we can't support replacement of the main action in clangd. What's the 
> > > plan there - ignore the plugin?
> > Could you help to explain why action replacements are not supported? 
> > 
> > As far as I know, replacement of actions is related with Actions, which 
> > does not have directly relationship with clangd,
> Clangd uses some custom FrontendActions (different ones for different ways 
> we're parsing the file) to implement its features (e.g. track which parts of 
> the AST are part of the main-file without deserializing the preamble, and to 
> do indexing).
> If you replace these actions, normal features will not work.
> 
> e.g.:
> - 
> https://github.com/llvm/llvm-project/blob/4df8efce80e373dd1e05bd4910c796a0c91383e7/clang-tools-extra/clangd/ParsedAST.cpp#L97
> - 
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/index/IndexAction.cpp#L206
> 
> If we replace these with the plugin's action, clangd won't work 
I think this is the part I didn't expected, seems a standalone action loading 
logic needs to exist.

I'll try to come up a patch which has standalone plugin loading and guard it 
with experimental checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGabfcb606c2f8: [clangd] Add support for within-file rename of 
complicated fields (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  R"cpp(
+template
+struct Foo {
+  T [[Vari^able]] = 42;
+};
+
+void foo() {
+  Foo f;
+  f.[[Varia^ble]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T Variable;
+  void bar() { ++Variable; }
+};
+
+template<>
+struct Foo {
+  unsigned [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,28 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(D)) {
+// This is a hacky way to do something like
+// CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
+// Clang AST does not store relevant information about the field that is
+// instantiated.
+const auto *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] abfcb60 - [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2020-11-27T03:59:28+01:00
New Revision: abfcb606c2f86da6dbf25bc260e4d716bc87eaf0

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

LOG: [clangd] Add support for within-file rename of complicated fields

This was originally a part of D71880 but is separated for simplicity and ease
of reviewing.

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

Reviewed By: hokein

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

Added: 


Modified: 
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 78aaa9930cd4..946daaf6d158 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,28 @@ const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(D)) {
+// This is a hacky way to do something like
+// CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
+// Clang AST does not store relevant information about the field that is
+// instantiated.
+const auto *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 

diff  --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp 
b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index c67339ff2be4..2382dba19659 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -540,6 +540,94 @@ TEST(RenameTest, WithinFileRename) {
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  R"cpp(
+template
+struct Foo {
+  T [[Vari^able]] = 42;
+};
+
+void foo() {
+  Foo f;
+  f.[[Varia^ble]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T Variable;
+  void bar() { ++Variable; }
+};
+
+template<>
+struct Foo {
+  unsigned [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 



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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn updated this revision to Diff 307947.
xndcn added a comment.

Thanks. Update commit to fix the last nit.


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

https://reviews.llvm.org/D92041

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2019,6 +2019,61 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+  namespace ns {
+// comment
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+HI.Documentation = "comment";
+  }},
+  {
+  R"cpp(// this expr for template class
+  namespace ns {
+template 
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "template  class Foo {}";
+HI.TemplateParameters = {
+{std::string("typename"), std::string("T"), llvm::None}};
+  }},
+  {
+  R"cpp(// this expr for specialization struct
+  namespace ns {
+template  struct Foo {};
+template <>
+struct Foo {
+  Foo* bar() {
+return [[thi^s]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Struct;
+HI.NamespaceScope = "ns::";
+HI.Definition = "template <> struct Foo {}";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -628,15 +628,21 @@
   return llvm::StringLiteral("expression");
 }
 
-// Generates hover info for evaluatable expressions.
+// Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional getHoverContents(const Expr *E, ParsedAST ) {
+llvm::Optional getHoverContents(const Expr *E, ParsedAST ,
+   const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
 return llvm::None;
 
   HoverInfo HI;
+  // For `this` expr we currently generate hover with class declaration.
+  if (const CXXThisExpr *CTE = dyn_cast(E)) {
+const NamedDecl *D = CTE->getType()->getPointeeType()->getAsTagDecl();
+return getHoverContents(D, Index);
+  }
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
@@ -861,7 +867,7 @@
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
   } else if (const Expr *E = N->ASTNode.get()) {
-HI = getHoverContents(E, AST);
+HI = getHoverContents(E, AST, Index);
   }
   // FIXME: support hovers for other nodes?
   //  - built-in types
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92202: [clangd] Add symbol origin for remote index

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kbobyrev requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Makes it easier to diagnose remote index issues with --debug-origins flag.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92202

Files:
  clang-tools-extra/clangd/index/SymbolOrigin.cpp
  clang-tools-extra/clangd/index/SymbolOrigin.h
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp


Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
@@ -16,6 +16,7 @@
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "index/SymbolLocation.h"
+#include "index/SymbolOrigin.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/SmallString.h"
@@ -73,7 +74,7 @@
   Sym.CanonicalDeclaration = Location;
 
   Sym.References = 9000;
-  Sym.Origin = clangd::SymbolOrigin::Static;
+  Sym.Origin = clangd::SymbolOrigin::Static | clangd::SymbolOrigin::Remote;
   Sym.Signature = Strings.save("(int X, char Y, Type T)");
   Sym.TemplateSpecializationArgs = Strings.save("");
   Sym.CompletionSnippetSuffix =
Index: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
===
--- clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
+++ clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
@@ -161,7 +161,8 @@
 return Declaration.takeError();
   Result.CanonicalDeclaration = *Declaration;
   Result.References = Message.references();
-  Result.Origin = static_cast(Message.origin());
+  Result.Origin = static_cast(Message.origin()) |
+  clangd::SymbolOrigin::Remote;
   Result.Signature = Message.signature();
   Result.TemplateSpecializationArgs = Message.template_specialization_args();
   Result.CompletionSnippetSuffix = Message.completion_snippet_suffix();
Index: clang-tools-extra/clangd/index/SymbolOrigin.h
===
--- clang-tools-extra/clangd/index/SymbolOrigin.h
+++ clang-tools-extra/clangd/index/SymbolOrigin.h
@@ -25,6 +25,7 @@
   Static = 1 << 2, // From the static, externally-built index.
   Merge = 1 << 3,  // A non-trivial index merge was performed.
   Identifier = 1 << 4, // Raw identifiers in file.
+  Remote = 1 << 5, // Remote index.
   // Remaining bits reserved for index implementations.
 };
 
Index: clang-tools-extra/clangd/index/SymbolOrigin.cpp
===
--- clang-tools-extra/clangd/index/SymbolOrigin.cpp
+++ clang-tools-extra/clangd/index/SymbolOrigin.cpp
@@ -14,7 +14,7 @@
 llvm::raw_ostream <<(llvm::raw_ostream , SymbolOrigin O) {
   if (O == SymbolOrigin::Unknown)
 return OS << "unknown";
-  constexpr static char Sigils[] = "ADSMI567";
+  constexpr static char Sigils[] = "ADSMIR67";
   for (unsigned I = 0; I < sizeof(Sigils); ++I)
 if (static_cast(O) & 1u << I)
   OS << Sigils[I];


Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
@@ -16,6 +16,7 @@
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "index/SymbolLocation.h"
+#include "index/SymbolOrigin.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/SmallString.h"
@@ -73,7 +74,7 @@
   Sym.CanonicalDeclaration = Location;
 
   Sym.References = 9000;
-  Sym.Origin = clangd::SymbolOrigin::Static;
+  Sym.Origin = clangd::SymbolOrigin::Static | clangd::SymbolOrigin::Remote;
   Sym.Signature = Strings.save("(int X, char Y, Type T)");
   Sym.TemplateSpecializationArgs = Strings.save("");
   Sym.CompletionSnippetSuffix =
Index: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
===
--- clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
+++ clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
@@ -161,7 +161,8 @@
 return Declaration.takeError();
   Result.CanonicalDeclaration = *Declaration;
   Result.References = Message.references();
-  Result.Origin = static_cast(Message.origin());
+  Result.Origin = static_cast(Message.origin()) |
+  clangd::SymbolOrigin::Remote;
   Result.Signature = Message.signature();
   Result.TemplateSpecializationArgs = 

[PATCH] D92201: [clangd] Make sure project-aware index is up-to-date for estimateMemoryUsage()

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kbobyrev requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92201

Files:
  clang-tools-extra/clangd/index/ProjectAware.cpp


Index: clang-tools-extra/clangd/index/ProjectAware.cpp
===
--- clang-tools-extra/clangd/index/ProjectAware.cpp
+++ clang-tools-extra/clangd/index/ProjectAware.cpp
@@ -71,6 +71,8 @@
 };
 
 size_t ProjectAwareIndex::estimateMemoryUsage() const {
+  // Ensure IndexForSpec is up-to-date.
+  getIndex(); // Unused.
   size_t Total = 0;
   std::lock_guard Lock(Mu);
   for (auto  : IndexForSpec)


Index: clang-tools-extra/clangd/index/ProjectAware.cpp
===
--- clang-tools-extra/clangd/index/ProjectAware.cpp
+++ clang-tools-extra/clangd/index/ProjectAware.cpp
@@ -71,6 +71,8 @@
 };
 
 size_t ProjectAwareIndex::estimateMemoryUsage() const {
+  // Ensure IndexForSpec is up-to-date.
+  getIndex(); // Unused.
   size_t Total = 0;
   std::lock_guard Lock(Mu);
   for (auto  : IndexForSpec)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92006: Refactoring the attrubute plugin example to fit the new API

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92006

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

[PATCH] D92198: [clangd] Implement remote index handshake

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307943.
kbobyrev added a comment.

Remove unused header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92198

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/Client.h
  clang-tools-extra/clangd/index/remote/Index.proto
  clang-tools-extra/clangd/index/remote/Service.proto
  clang-tools-extra/clangd/index/remote/server/Server.cpp

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -96,6 +97,19 @@
 private:
   using stopwatch = std::chrono::steady_clock;
 
+  grpc::Status Handshake(grpc::ServerContext *Context,
+ const remote::HandshakeRequest *Request,
+ HandshakeReply *Reply) override {
+WithContextValue WithRequestContext(CurrentRequest, Context);
+logRequest(*Request);
+trace::Span Tracer("LookupRequest");
+Reply->set_checksum(Request->checksum());
+logResponse(*Reply);
+log("[public] request {0} => OK", HandshakeRequest::descriptor()->name());
+SPAN_ATTACH(Tracer, "Checksum", Reply->checksum());
+return grpc::Status::OK;
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
Index: clang-tools-extra/clangd/index/remote/Service.proto
===
--- clang-tools-extra/clangd/index/remote/Service.proto
+++ clang-tools-extra/clangd/index/remote/Service.proto
@@ -15,6 +15,8 @@
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
+  rpc Handshake(HandshakeRequest) returns (HandshakeReply) {}
+
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
Index: clang-tools-extra/clangd/index/remote/Index.proto
===
--- clang-tools-extra/clangd/index/remote/Index.proto
+++ clang-tools-extra/clangd/index/remote/Index.proto
@@ -10,6 +10,14 @@
 
 package clang.clangd.remote;
 
+message HandshakeRequest {
+  required uint32 checksum = 1;
+}
+
+message HandshakeReply {
+  required uint32 checksum = 1;
+}
+
 // Common final result for streaming requests.
 message FinalResult { optional bool has_more = 1; }
 
Index: clang-tools-extra/clangd/index/remote/Client.h
===
--- clang-tools-extra/clangd/index/remote/Client.h
+++ clang-tools-extra/clangd/index/remote/Client.h
@@ -22,10 +22,13 @@
 /// described by the remote index. Paths returned by the index will be treated
 /// as relative to this directory.
 ///
-/// This method attempts to resolve the address and establish the connection.
+/// This method attempts to resolve the address and establish the connection by
+/// performing the handshake (sending the data and trying to receive it back).
 ///
-/// \returns nullptr if the address is not resolved during the function call or
-/// if the project was compiled without Remote Index support.
+/// \returns nullptr if one of the following conditions holds:
+/// * Address is not resolved during the function call.
+/// * Handshake was not successful or received data is corrupted.
+/// * Project was compiled without Remote Index support.
 std::unique_ptr getClient(llvm::StringRef Address,
llvm::StringRef IndexRoot);
 
Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -19,6 +19,8 @@
 #include "llvm/Support/Error.h"
 
 #include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -82,6 +84,35 @@
 assert(!ProjectRoot.empty());
   }
 
+  // Returns true if received checksum matches given one.
+  bool handshake(uint32_t Checksum) const {
+trace::Span Tracer(HandshakeRequest::descriptor()->name());
+HandshakeRequest RPCRequest;
+RPCRequest.set_checksum(Checksum);
+SPAN_ATTACH(Tracer, "Request", RPCRequest.DebugString());
+grpc::ClientContext Context;
+Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
+const std::chrono::system_clock::time_point StartTime =
+std::chrono::system_clock::now();
+const auto Deadline = StartTime + DeadlineWaitingTime;
+Context.set_deadline(Deadline);
+vlog("Sending RPC Request {0}: {1}", 

[PATCH] D92198: [clangd] Implement remote index handshake

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307942.
kbobyrev added a comment.

This is working now. However, the handshake is actually deferred for
ProjectAwareIndex until the first request. I tried to invoke it via something
like `Opts.StaticIdx->estimateMemoryUsage()` in `ClangdMain.cpp` but the
problem is `getIndex()` will return `nullptr` until the `Config::current` is
ready. I'm not entirely sure when this happens so I can't promote handshake to
some sensible time point yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92198

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/Client.h
  clang-tools-extra/clangd/index/remote/Index.proto
  clang-tools-extra/clangd/index/remote/Service.proto
  clang-tools-extra/clangd/index/remote/server/Server.cpp

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -96,6 +97,19 @@
 private:
   using stopwatch = std::chrono::steady_clock;
 
+  grpc::Status Handshake(grpc::ServerContext *Context,
+ const remote::HandshakeRequest *Request,
+ HandshakeReply *Reply) override {
+WithContextValue WithRequestContext(CurrentRequest, Context);
+logRequest(*Request);
+trace::Span Tracer("LookupRequest");
+Reply->set_checksum(Request->checksum());
+logResponse(*Reply);
+log("[public] request {0} => OK", HandshakeRequest::descriptor()->name());
+SPAN_ATTACH(Tracer, "Checksum", Reply->checksum());
+return grpc::Status::OK;
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
Index: clang-tools-extra/clangd/index/remote/Service.proto
===
--- clang-tools-extra/clangd/index/remote/Service.proto
+++ clang-tools-extra/clangd/index/remote/Service.proto
@@ -15,6 +15,8 @@
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
+  rpc Handshake(HandshakeRequest) returns (HandshakeReply) {}
+
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
Index: clang-tools-extra/clangd/index/remote/Index.proto
===
--- clang-tools-extra/clangd/index/remote/Index.proto
+++ clang-tools-extra/clangd/index/remote/Index.proto
@@ -10,6 +10,14 @@
 
 package clang.clangd.remote;
 
+message HandshakeRequest {
+  required uint32 checksum = 1;
+}
+
+message HandshakeReply {
+  required uint32 checksum = 1;
+}
+
 // Common final result for streaming requests.
 message FinalResult { optional bool has_more = 1; }
 
Index: clang-tools-extra/clangd/index/remote/Client.h
===
--- clang-tools-extra/clangd/index/remote/Client.h
+++ clang-tools-extra/clangd/index/remote/Client.h
@@ -22,10 +22,13 @@
 /// described by the remote index. Paths returned by the index will be treated
 /// as relative to this directory.
 ///
-/// This method attempts to resolve the address and establish the connection.
+/// This method attempts to resolve the address and establish the connection by
+/// performing the handshake (sending the data and trying to receive it back).
 ///
-/// \returns nullptr if the address is not resolved during the function call or
-/// if the project was compiled without Remote Index support.
+/// \returns nullptr if one of the following conditions holds:
+/// * Address is not resolved during the function call.
+/// * Handshake was not successful or received data is corrupted.
+/// * Project was compiled without Remote Index support.
 std::unique_ptr getClient(llvm::StringRef Address,
llvm::StringRef IndexRoot);
 
Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -14,11 +14,14 @@
 #include "marshalling/Marshalling.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 
 #include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -82,6 +85,35 @@
 assert(!ProjectRoot.empty());
   }
 
+  // Returns true if received checksum matches given one.
+  bool 

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+OptionStack.push_back(std::move(*Config));
+if (!OptionStack.back().InheritParentConfig)
+  break;

njames93 wrote:
> sammccall wrote:
> > njames93 wrote:
> > > This will result in incorrect behaviour if a config specifies 
> > > `InheritParentConfig` as `false`
> > Whoops, thanks!
> > (Why is this optional rather than bool?)
> I'm honestly not sure. Personally I don't think it should belong in 
> ClangTidyOptions. Its only used when reading configuration from files(or 
> command line). Maybe I'll rustle something up for that.
What do you think about this approach? D92199


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133

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

[PATCH] D92198: [clangd] Implement remote index handshake

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307936.
kbobyrev added a comment.

Remove some logs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92198

Files:
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/Client.h
  clang-tools-extra/clangd/index/remote/Index.proto
  clang-tools-extra/clangd/index/remote/Service.proto
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -815,6 +815,10 @@
   Opts.CodeComplete.RankingModel = RankingModel;
   Opts.CodeComplete.DecisionForestBase = DecisionForestBase;
 
+#if CLANGD_ENABLE_REMOTE
+  // clang::clangd::remote::handshake(Opts.StaticIndex);
+#endif
+
   RealThreadsafeFS TFS;
   std::vector> ProviderStack;
   std::unique_ptr Config;
Index: clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
===
--- clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
+++ clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
@@ -20,6 +20,11 @@
   return nullptr;
 }
 
+bool handshake(const clangd::SymbolIndex *Index) {
+  elog("Can't perform a handshake without Remote Index support.");
+  return false;
+}
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -96,6 +97,18 @@
 private:
   using stopwatch = std::chrono::steady_clock;
 
+  grpc::Status Handshake(grpc::ServerContext *Context,
+ const remote::HandshakeRequest *Request,
+ HandshakeReply *Reply) override {
+WithContextValue WithRequestContext(CurrentRequest, Context);
+logRequest(*Request);
+trace::Span Tracer("LookupRequest");
+Reply->set_checksum(Request->checksum());
+logResponse(*Reply);
+SPAN_ATTACH(Tracer, "Checksum", Reply->checksum());
+return grpc::Status::OK;
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
Index: clang-tools-extra/clangd/index/remote/Service.proto
===
--- clang-tools-extra/clangd/index/remote/Service.proto
+++ clang-tools-extra/clangd/index/remote/Service.proto
@@ -15,6 +15,8 @@
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
+  rpc Handshake(HandshakeRequest) returns (HandshakeReply) {}
+
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
Index: clang-tools-extra/clangd/index/remote/Index.proto
===
--- clang-tools-extra/clangd/index/remote/Index.proto
+++ clang-tools-extra/clangd/index/remote/Index.proto
@@ -10,6 +10,14 @@
 
 package clang.clangd.remote;
 
+message HandshakeRequest {
+  required uint32 checksum = 1;
+}
+
+message HandshakeReply {
+  required uint32 checksum = 1;
+}
+
 // Common final result for streaming requests.
 message FinalResult { optional bool has_more = 1; }
 
Index: clang-tools-extra/clangd/index/remote/Client.h
===
--- clang-tools-extra/clangd/index/remote/Client.h
+++ clang-tools-extra/clangd/index/remote/Client.h
@@ -29,6 +29,12 @@
 std::unique_ptr getClient(llvm::StringRef Address,
llvm::StringRef IndexRoot);
 
+/// Sends data to the server and compares it against response.
+///
+/// \returns true if Index is an instance of IndexClient and response matches
+/// sent data.
+bool handshake(const clangd::SymbolIndex *Index);
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -14,11 +14,14 @@
 #include "marshalling/Marshalling.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
+#include "clang/Basic/LLVM.h"

[PATCH] D92198: [clangd] Implement remote index handshake

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

This is not working properly yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92198

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

[PATCH] D92198: [clangd] Implement remote index handshake

[Kirill Bobyrev via Phabricator via cfe-commits]

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

WIP.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92198

Files:
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/Client.h
  clang-tools-extra/clangd/index/remote/Index.proto
  clang-tools-extra/clangd/index/remote/Service.proto
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -815,6 +815,10 @@
   Opts.CodeComplete.RankingModel = RankingModel;
   Opts.CodeComplete.DecisionForestBase = DecisionForestBase;
 
+#if CLANGD_ENABLE_REMOTE
+  clang::clangd::remote::handshake(Opts.StaticIndex);
+#endif
+
   RealThreadsafeFS TFS;
   std::vector> ProviderStack;
   std::unique_ptr Config;
Index: clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
===
--- clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
+++ clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp
@@ -20,6 +20,11 @@
   return nullptr;
 }
 
+bool handshake(const clangd::SymbolIndex *Index) {
+  elog("Can't perform a handshake without Remote Index support.");
+  return false;
+}
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -96,6 +97,18 @@
 private:
   using stopwatch = std::chrono::steady_clock;
 
+  grpc::Status Handshake(grpc::ServerContext *Context,
+ const remote::HandshakeRequest *Request,
+ HandshakeReply *Reply) override {
+WithContextValue WithRequestContext(CurrentRequest, Context);
+logRequest(*Request);
+trace::Span Tracer("LookupRequest");
+Reply->set_checksum(Request->checksum());
+logResponse(*Reply);
+SPAN_ATTACH(Tracer, "Checksum", Reply->checksum());
+return grpc::Status::OK;
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
Index: clang-tools-extra/clangd/index/remote/Service.proto
===
--- clang-tools-extra/clangd/index/remote/Service.proto
+++ clang-tools-extra/clangd/index/remote/Service.proto
@@ -15,6 +15,8 @@
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
+  rpc Handshake(HandshakeRequest) returns (HandshakeReply) {}
+
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
Index: clang-tools-extra/clangd/index/remote/Index.proto
===
--- clang-tools-extra/clangd/index/remote/Index.proto
+++ clang-tools-extra/clangd/index/remote/Index.proto
@@ -10,6 +10,14 @@
 
 package clang.clangd.remote;
 
+message HandshakeRequest {
+  required uint32 checksum = 1;
+}
+
+message HandshakeReply {
+  required uint32 checksum = 1;
+}
+
 // Common final result for streaming requests.
 message FinalResult { optional bool has_more = 1; }
 
Index: clang-tools-extra/clangd/index/remote/Client.h
===
--- clang-tools-extra/clangd/index/remote/Client.h
+++ clang-tools-extra/clangd/index/remote/Client.h
@@ -29,6 +29,12 @@
 std::unique_ptr getClient(llvm::StringRef Address,
llvm::StringRef IndexRoot);
 
+/// Sends data to the server and compares it against response.
+///
+/// \returns true if Index is an instance of IndexClient and response matches
+/// sent data.
+bool handshake(const clangd::SymbolIndex *Index);
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -14,11 +14,14 @@
 #include "marshalling/Marshalling.h"
 #include 

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34
+  llvm::Optional
+  get(const ThreadsafeFS ,
+  std::chrono::steady_clock::time_point FreshTime) const {

sammccall wrote:
> njames93 wrote:
> > To save a copy, could this not return a const pointer to whats stored in 
> > `Value`, nullptr if `Value` is empty.
> > In `DotClangTidyTree`, `OptionStack` could then store pointers instead of 
> > values.
> > Considering the size of ClangTidyOptions (312 bytes in x64 with libstdc++) 
> > this is definitely a worthwhile saving.
> > 
> > One slight issue is I'm not sure how nicely this approach would play with 
> > the mutex.
> You're right, and 312 bytes understates it - it contains strings and stuff 
> that likely allocate on the heap when copied.
> 
> The issue with a pointer is that the underlying value may not live long 
> enough (or to put it another way, the pointer will point to the cache slot 
> which may be concurrently modified).
> 
> I think the fix for that is to store and return a `shared_ptr ClangTidyOptions>`. I'll give that a try.
That sounds like a good resolve. I'm guessing in the common case where the 
config file doesn't change, this would never mutate. 



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-87
+  // Avoid weird non-substring cases like phantom "." components.
+  // In practice, Component is a substring for all "normal" ancestors.
+  if (I->end() < Parent.begin() && I->end() > Parent.end())
+continue;

sammccall wrote:
> njames93 wrote:
> > How does this work with `..` components. Or does clangd ensure all absolute 
> > paths have those removed?
> Right, paths are always canonical.
If that's the case, aren't these checks redundant. Maybe put some asserts in so 
we don't pay for them in release builds



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+OptionStack.push_back(std::move(*Config));
+if (!OptionStack.back().InheritParentConfig)
+  break;

sammccall wrote:
> njames93 wrote:
> > This will result in incorrect behaviour if a config specifies 
> > `InheritParentConfig` as `false`
> Whoops, thanks!
> (Why is this optional rather than bool?)
I'm honestly not sure. Personally I don't think it should belong in 
ClangTidyOptions. Its only used when reading configuration from files(or 
command line). Maybe I'll rustle something up for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D52050#2415723 , @glaubitz wrote:

> Yeah, @hvdijk has made multiple other improvements which should finally allow 
> the backend to be usable.
>
> We still disagree on the search paths for libraries and headers though if I 
> remember correctly.

No disagreement. I hadn't done anything in this area only because my non-Debian 
system doesn't need it, not because I'm in any way opposed to this change. My 
knowledge of the Debian file system layout is a bit limited, but this change 
looks to me like it makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34
+  llvm::Optional
+  get(const ThreadsafeFS ,
+  std::chrono::steady_clock::time_point FreshTime) const {

njames93 wrote:
> To save a copy, could this not return a const pointer to whats stored in 
> `Value`, nullptr if `Value` is empty.
> In `DotClangTidyTree`, `OptionStack` could then store pointers instead of 
> values.
> Considering the size of ClangTidyOptions (312 bytes in x64 with libstdc++) 
> this is definitely a worthwhile saving.
> 
> One slight issue is I'm not sure how nicely this approach would play with the 
> mutex.
You're right, and 312 bytes understates it - it contains strings and stuff that 
likely allocate on the heap when copied.

The issue with a pointer is that the underlying value may not live long enough 
(or to put it another way, the pointer will point to the cache slot which may 
be concurrently modified).

I think the fix for that is to store and return a `shared_ptr`. I'll give that a try.



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+  const ThreadsafeFS 
+  std::string RelPath;
+  std::chrono::steady_clock::duration MaxStaleness;
 

njames93 wrote:
> Should these both be const?
We've generally avoided making members const in clangd when they can be, 
probably mostly to avoid problems with move semantics, but in the end because 
you've got to pick a style and stick with it.

(The move-semantics argument seems weak because we do use reference members 
sometimes, including in this class, and convert them to pointers when it 
becomes a problem. Oh well...)



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-87
+  // Avoid weird non-substring cases like phantom "." components.
+  // In practice, Component is a substring for all "normal" ancestors.
+  if (I->end() < Parent.begin() && I->end() > Parent.end())
+continue;

njames93 wrote:
> How does this work with `..` components. Or does clangd ensure all absolute 
> paths have those removed?
Right, paths are always canonical.



Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+OptionStack.push_back(std::move(*Config));
+if (!OptionStack.back().InheritParentConfig)
+  break;

njames93 wrote:
> This will result in incorrect behaviour if a config specifies 
> `InheritParentConfig` as `false`
Whoops, thanks!
(Why is this optional rather than bool?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133

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

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 307924.
sammccall marked 3 inline comments as done.
sammccall added a comment.

Return shared_ptr instead of value from cache, to avoid copies.

Remove unneeded qualifiers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92133

Files:
  clang-tools-extra/clangd/TidyProvider.cpp

Index: clang-tools-extra/clangd/TidyProvider.cpp
===
--- clang-tools-extra/clangd/TidyProvider.cpp
+++ clang-tools-extra/clangd/TidyProvider.cpp
@@ -8,7 +8,9 @@
 
 #include "TidyProvider.h"
 #include "Config.h"
+#include "support/FileCache.h"
 #include "support/Logger.h"
+#include "support/ThreadsafeFS.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
@@ -19,53 +21,119 @@
 
 namespace clang {
 namespace clangd {
+namespace {
 
-static void mergeCheckList(llvm::Optional ,
-   llvm::StringRef List) {
-  if (List.empty())
-return;
-  if (!Checks || Checks->empty()) {
-Checks.emplace(List);
-return;
+// Access to config from a .clang-tidy file, caching IO and parsing.
+class DotClangTidyCache : private FileCache {
+  // We cache and expose shared_ptr to avoid copying the value on every lookup
+  // when we're ultimately just going to pass it to mergeWith.
+  mutable std::shared_ptr Value;
+
+public:
+  DotClangTidyCache(PathRef Path) : FileCache(Path) {}
+
+  std::shared_ptr
+  get(const ThreadsafeFS ,
+  std::chrono::steady_clock::time_point FreshTime) const {
+std::shared_ptr Result;
+read(
+TFS, FreshTime,
+[this](llvm::Optional Data) {
+  Value.reset();
+  if (Data && !Data->empty()) {
+if (auto Parsed = tidy::parseConfiguration(*Data))
+  Value = std::make_shared(
+  std::move(*Parsed));
+else
+  elog("Error parsing clang-tidy configuration in {0}: {1}", path(),
+   Parsed.getError().message());
+  }
+},
+[&]() { Result = Value; });
+return Result;
   }
-  *Checks = llvm::join_items(",", *Checks, List);
-}
+};
 
-static llvm::Optional
-tryReadConfigFile(llvm::vfs::FileSystem *FS, llvm::StringRef Directory) {
-  assert(!Directory.empty());
-  // We guaranteed that child directories of Directory exist, so this assert
-  // should hopefully never fail.
-  assert(FS->exists(Directory));
+// Access to combined config from .clang-tidy files governing a source file.
+// Each config file is cached and the caches are shared for affected sources.
+//
+// FIXME: largely duplicates config::Provider::fromAncestorRelativeYAMLFiles.
+// Potentially useful for compile_commands.json too. Extract?
+class DotClangTidyTree {
+  const ThreadsafeFS 
+  std::string RelPath;
+  std::chrono::steady_clock::duration MaxStaleness;
 
-  llvm::SmallString<128> ConfigFile(Directory);
-  llvm::sys::path::append(ConfigFile, ".clang-tidy");
+  mutable std::mutex Mu;
+  // Keys are the ancestor directory, not the actual config path within it.
+  // We only insert into this map, so pointers to values are stable forever.
+  // Mutex guards the map itself, not the values (which are threadsafe).
+  mutable llvm::StringMap Cache;
 
-  llvm::ErrorOr FileStatus = FS->status(ConfigFile);
+public:
+  DotClangTidyTree(const ThreadsafeFS )
+  : FS(FS), RelPath(".clang-tidy"), MaxStaleness(std::chrono::seconds(5)) {}
 
-  if (!FileStatus || !FileStatus->isRegularFile())
-return llvm::None;
+  void apply(tidy::ClangTidyOptions , PathRef AbsPath) {
+namespace path = llvm::sys::path;
+assert(path::is_absolute(AbsPath));
 
-  llvm::ErrorOr> Text =
-  FS->getBufferForFile(ConfigFile);
-  if (std::error_code EC = Text.getError()) {
-elog("Can't read '{0}': {1}", ConfigFile, EC.message());
-return llvm::None;
+// Compute absolute paths to all ancestors (substrings of P.Path).
+llvm::StringRef Parent = path::parent_path(AbsPath);
+llvm::SmallVector Ancestors;
+for (auto I = path::begin(Parent, path::Style::posix),
+  E = path::end(Parent);
+ I != E; ++I) {
+  // Avoid weird non-substring cases like phantom "." components.
+  // In practice, Component is a substring for all "normal" ancestors.
+  if (I->end() < Parent.begin() && I->end() > Parent.end())
+continue;
+  Ancestors.emplace_back(Parent.begin(), I->end() - Parent.begin());
+}
+// Ensure corresponding cache entries exist in the map.
+llvm::SmallVector Caches;
+{
+  std::lock_guard Lock(Mu);
+  for (llvm::StringRef Ancestor : Ancestors) {
+auto It = Cache.find(Ancestor);
+// Assemble the actual config file path only once.
+if (It == Cache.end()) {
+  llvm::SmallString<256> ConfigPath = Ancestor;
+  path::append(ConfigPath, RelPath);
+  It = 

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

TL;DR: clangd has some different concerns than clang, so we really need to 
approach this as a new feature with some design required (even if a mechanism 
already exists).
The most promising ideas I can think of involve doing the loading at clangd 
startup based on some trusted configuration, and *not* dynamically loading in 
the driver, instead allowing the use of plugins already loaded.

In D92155#2418884 , @psionic12 wrote:

>> Right, I understand (a little bit, at least!) what plugins *can* do. What 
>> I'm asking specifically is: this feature has a cost, how important is 
>> supporting it? Are there codebases where these attributes are widely used, 
>> and enforcement at development time is particularly valuable?
>
> Well, I can't tell you how widely plugin used, because they often used in 
> third-party projects. Firefox team seems use them a lot, the `ParsedAttrInfo` 
> plugin implementation seems write by them if I remember right. And our team 
> use both plugins and clangd a lot, that's why I'm willing to contribute to 
> make them work together. As about the importance, my personal opinion is that 
> since there's a feature, we should try to polish it...

Ah, thanks! If the widely-used plugins live in-tree, then it's more plausible 
to support just those, e.g.

- we don't need to consider the security concerns of loading unknown code
- we can fix them if they become e.g. thread-hostile

>> Sorry, I probably should have said "can plugins be thread-hostile".
>> Clangd runs several parsing actions in parallel, each on a single separate 
>> thread.
>> If a plugin assumes there's only a single instance of it running and uses 
>> static data accordingly, we're going to hit problems in clangd. 
>> The problem is that precisely because clang uses a single thread, (some) 
>> plugins may think this is a reasonable assumption. And it's not possible to 
>> tell "from the outside" whether a given plugin is unsafe in this way.
>
> That seems a serious problem, but I got a question, what if the clang itself 
> declared some static data (I'll investigate later, just ask for now)?

You mean if clang itself were thread-hostile? We've found and fixed such 
problems over the years as part of developing clangd and other tools that use 
the clang frontend in a multithreaded environment. This cost some time and 
effort, but had a high reward, we had access to all the code, and the problem 
was bounded. So it was feasible and worthwhile!

>> It looks like this patch moves `LoadLibraryPermanently` to a different 
>> location, per the patch description clangd does not currently load plugins. 
>> That's why I'm concerned this patch may introduce unsafe loading of 
>> untrusted .sos.
>
> Well, since it called plugin, it's users choose to use it or not, I 
> personally think we can't check if an `.so` is trusted, and neither should we 
> care...

We need to be careful about what "user" and "choose" means.
e.g. if you clone an untrusted git repo and open `src/foo.cpp` in your editor...
(and unknown to you, `src/compile_flags.txt` contains `-plugin=../lib/pwn.so`)
(and unknown to you, `lib/pwn.so` wipes your hard disk when loaded)
Have you made a meaningful choice to use that plugin? That's the security 
context clangd is in - the decision to parse the file is implicit, and the 
compile flags are potentially attacker-controlled.

While the *clang* flags to parse a particular file are attacker-controlled, the 
*clangd* flags are not, and are a viable place to put security-sensitive 
configuration. So one potential design idea is to pass `-clang-plugin=foo.so` 
to clangd, and load foo.so on startup (not in the clang driver). Then when it 
comes time to parse a file, we can allow its args to activate plugins that are 
already loaded, but not to load new ones.
(There's precedent for this: the clangd --query-driver flag is designed to 
mitigate a similar clang-flags-lead-to-arbitary-code-execution concern)

I think this might be viable but as you can imagine it's considerably more 
involved than just enabling the plugin code in the driver.

>> - the filesystem access to the libraries isn't virtualized (vfs) and I don't 
>> know if it really can be.
>
> Sorry I still can't understand this, could you help to explain more?

clangd (and other clang tools) get deployed in environments where all access to 
the filesystem goes through a llvm::vfs::Filesystem, and all filenames referred 
to in the compile command are within that virtual filesystem rather than the 
real one.
(Again, this isn't an issue if we require these paths to be specified on the 
*clangd* startup command line, as opposed to the clang flags that form the 
compile command)

> What I mean before is that since this part of code is used in clang driver as 
> well, it should be no problem get called by clangd.

To my knowledge, clang itself is never used in such an environment, so only the 
parts that 

[PATCH] D90173: [PowerPC] Exploit splat instruction xxsplti32dx in Power10

[Victor Huang via Phabricator via cfe-commits]

NeHuang requested changes to this revision.
NeHuang added inline comments.
This revision now requires changes to proceed.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9189
HasAnyUndefs, 0, !Subtarget.isLittleEndian());
+  bool LE = Subtarget.isLittleEndian();
 

please follow the naming convention in this file to use ` IsLE`



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9204
+
+  uint32_t top = (uint32_t) ((APSplatBits.getZExtValue() & 
0xLL) >> 32);
+  uint32_t bot = (uint32_t) (APSplatBits.getZExtValue() & 
0xLL);

Agree with Amy it is better to use `Hi` and Lo` as the variable names. 
Also it is still taking more than 80 characters. Please double check if you 
properly `clang-format` it.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9205
+  uint32_t top = (uint32_t) ((APSplatBits.getZExtValue() & 
0xLL) >> 32);
+  uint32_t bot = (uint32_t) (APSplatBits.getZExtValue() & 
0xLL);
+  SDValue SplatNode;

85 characters in this line, please double check you properly do `clang-format` 



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9213
+
+  if (bot) {
+SplatNode = DAG.getNode(

I do not quite understand the `if` and `else` logic here, current logic seems 
we can overwrite `SplatNode` after we execute `SplatNode = 
DAG.getTargetConstant(0, dl, MVT::v2i64);` 
Is that as expected? 



Comment at: llvm/test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll:17
 ; CHECK:   # %bb.0: # %entry
-; CHECK-NEXT:plxv vs34, .LCPI0_0@PCREL(0), 1
+; CHECK-NEXT:xxlxor vs34, vs34, vs34
+; CHECK-LE-NEXT:xxsplti32dx vs34, 1, 1081435463

alignment issue 



Comment at: llvm/test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll:19
+; CHECK-LE-NEXT:xxsplti32dx vs34, 1, 1081435463
+; CHECK-BE-NEXT: xxsplti32dx vs34, 0, 1081435463
 ; CHECK-NEXT:blr

alignment issue 



Comment at: llvm/test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll:43
 ; CHECK-NOPCREL:   # %bb.0: # %entry
-; CHECK-NOPCREL-NEXT:addis r3, r2, .LCPI1_0@toc@ha
-; CHECK-NOPCREL-NEXT:addi r3, r3, .LCPI1_0@toc@l
-; CHECK-NOPCREL-NEXT:lxvx vs34, 0, r3
+; CHECK-NOPCREL-NEXT:xxlxor vs34, vs34, vs34
+; CHECK-NOPCREL-LE-NEXT:xxsplti32dx vs34, 1, 940259579

alignment issue 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90173

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

[PATCH] D89869: [OpenCL] Define OpenCL feature macros for all versions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

After a recent discussion with Khronos on 
https://github.com/KhronosGroup/OpenCL-Docs/issues/500 and more thinking I 
start to realize that there is absolutely nothing new in OpenCL 3.0 features as 
it isn't a new concept at all. They use to be explained as optional core 
features in the extension spec from early spec versions. So I think what have 
changed mainly in OpenCL 3.0 is that the features that were core have now 
become optional while the current design in clang assumes that core features 
can't become optional. I feel this is what we should reflect better in the 
design right now. So the categories we have are:

- extension (conditionally supported, can have pragma to alter certain 
functionality)
- optional feature (conditionally supported, pragma has no effect)
- core feature (unconditionally supported, pragma has no effect)

Then for every pair of optional functionality and OpenCL language version one 
of the above categories should be set.

Let's take `cl_khr_3d_image_writes`/`__opencl_c_3d_image_writes` as an example:

- In OpenCL C 1.0-1.2 it is an extension
- In OpenCL C 2.0 it is a core feature
- In OpenCL C 3.0 it is an optional feature

I am now trying to think perhaps the data structure in `OpenCLOptions` should 
be changed to better match the concept. Then perhaps the flow will become more 
clear? Right now I find it very complex to understand, mainly because there are 
features that are being set differently before and after OpenCL 3.0 and then 
overridden in some places or not set to the expected values. It also doesn't 
help of course that there are so many existing bugs like with the core 
features...

I didn't want to end up with a big refactoring from this patch but now I start 
to think that considering the situation it might be unavoidable. :(




Comment at: clang/include/clang/Basic/OpenCLOptions.h:42
+
+  // For pre-OpenCL C 3.0 features are supported unconditionally
+  bool isSupportedFeatureForPreOCL30(llvm::StringRef Feat) const {

I think the comment is misleading a bit because you have a condition on OpenCL 
version.



Comment at: clang/include/clang/Basic/OpenCLOptions.h:128
+  if (isSupportedFeatureForPreOCL30(Ext))
+Info.Supported = true;
+} else

This is a violation of the API, as its expected to set the feature to a certain 
value` V`, but here it is forced to be `true`.


Perhaps a better way to handle this is to define whether the feature is core or 
not and then skip over core features here.

The OpenCL 3.0 features will be defined as core in OpenCL 2.0 but optional in 
OpenCL 3.0.

Then we can update the API with a more consistent and clear behavior.



Comment at: clang/include/clang/Basic/OpenCLOptions.h:133
+
+  void addSupport(const OpenCLOptions ) {
+for (auto  : Opts.OptMap)

We really need to follow up with some clean up of the duplication and the need 
to copy entries over.



Comment at: clang/include/clang/Basic/OpenCLOptions.h:37
+  // OpenCL Version
+  unsigned CLVer = 120;
+  bool IsOpenCLCPlusPlus = false;

Anastasia wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > Ok, I see why you are adding this field but I am not very happy with it. 
> > > However, if you prefer to keep it I suggest to add a comment otherwise it 
> > > is mislediang because ideally in Clang we should be using versions from 
> > > the LangOpts everywhere. Alternatively we could consider a helper 
> > > function to calculate the version although it doesn't eliminate the need 
> > > to the comment.
> > > However, if you prefer to keep it I suggest to add a comment otherwise it 
> > > is mislediang because ideally in Clang we should be using versions from 
> > > the LangOpts everywhere
> > 
> > The reason for this is that `LangOptions` is not always available for 
> > proper settings. So this just needed internally. Also, we could add 
> > `TargetInfo::setOpenCLOptions(const LangOptions )` where we can set 
> > all extensions/features in one step (invoke `::setSupportedOpenCLOpts` and 
> > `::setOpenCLExtensionOpts`) to make sure `OpenCLOptions` will get proper 
> > OpenCL version. What do you think of that?
> Yes this feels neater!
I feel this just results in more layering violations than we have already made 
in the past. The version should really be stored in the `LangOpts` and we can 
either have a reference to it here or/and a helper function to compute the lang 
version equivalent for the extensions given `LangOpts`.

Perhaps this relates to the fact that `OpenCLOptions` are completely duplicated 
in Sema... maybe they should have just been there and not in the target 
settings after all... Or perhaps we are in need for some bigger restructuring 
in order to avoid making more mess...



Comment at: clang/include/clang/Basic/TargetInfo.h:1488
+  /// Set supported OpenCL 

[PATCH] D92186: [clangd] AddUsing: do not crash on non-namespace using decls.

[Adam Czachorowski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d87739f664b: [clangd] AddUsing: do not crash on 
non-namespace using decls. (authored by adamcz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92186

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2884,6 +2884,17 @@
 void fun() {
   yy();
 }
+)cpp"},
+// Existing using with non-namespace part.
+{R"cpp(
+#include "test.hpp"
+using one::two::ee::ee_one;
+one::t^wo::cc c;
+)cpp",
+ R"cpp(
+#include "test.hpp"
+using one::two::cc;using one::two::ee::ee_one;
+cc c;
 )cpp"}};
   llvm::StringMap EditedFiles;
   for (const auto  : Cases) {
@@ -2892,7 +2903,7 @@
 namespace one {
 void oo() {}
 namespace two {
-enum ee {};
+enum ee {ee_one};
 void ff() {}
 class cc {
 public:
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -152,12 +152,14 @@
 if (SM.isBeforeInTranslationUnit(Inputs.Cursor, U->getUsingLoc()))
   // "Usings" is sorted, so we're done.
   break;
-if (U->getQualifier()->getAsNamespace()->getCanonicalDecl() ==
-QualifierToRemove.getNestedNameSpecifier()
-->getAsNamespace()
-->getCanonicalDecl() &&
-U->getName() == Name) {
-  return InsertionPointData();
+if (const auto *Namespace = U->getQualifier()->getAsNamespace()) {
+  if (Namespace->getCanonicalDecl() ==
+  QualifierToRemove.getNestedNameSpecifier()
+  ->getAsNamespace()
+  ->getCanonicalDecl() &&
+  U->getName() == Name) {
+return InsertionPointData();
+  }
 }
 
 // Insertion point will be before last UsingDecl that affects cursor


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2884,6 +2884,17 @@
 void fun() {
   yy();
 }
+)cpp"},
+// Existing using with non-namespace part.
+{R"cpp(
+#include "test.hpp"
+using one::two::ee::ee_one;
+one::t^wo::cc c;
+)cpp",
+ R"cpp(
+#include "test.hpp"
+using one::two::cc;using one::two::ee::ee_one;
+cc c;
 )cpp"}};
   llvm::StringMap EditedFiles;
   for (const auto  : Cases) {
@@ -2892,7 +2903,7 @@
 namespace one {
 void oo() {}
 namespace two {
-enum ee {};
+enum ee {ee_one};
 void ff() {}
 class cc {
 public:
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -152,12 +152,14 @@
 if (SM.isBeforeInTranslationUnit(Inputs.Cursor, U->getUsingLoc()))
   // "Usings" is sorted, so we're done.
   break;
-if (U->getQualifier()->getAsNamespace()->getCanonicalDecl() ==
-QualifierToRemove.getNestedNameSpecifier()
-->getAsNamespace()
-->getCanonicalDecl() &&
-U->getName() == Name) {
-  return InsertionPointData();
+if (const auto *Namespace = U->getQualifier()->getAsNamespace()) {
+  if (Namespace->getCanonicalDecl() ==
+  QualifierToRemove.getNestedNameSpecifier()
+  ->getAsNamespace()
+  ->getCanonicalDecl() &&
+  U->getName() == Name) {
+return InsertionPointData();
+  }
 }
 
 // Insertion point will be before last UsingDecl that affects cursor
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92176: Don't use sysroot/include when sysroot is empty.

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

lgtm but not an expert in this area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92176

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

[clang-tools-extra] 9d87739 - [clangd] AddUsing: do not crash on non-namespace using decls.

[Adam Czachorowski via cfe-commits]

Author: Adam Czachorowski
Date: 2020-11-26T20:07:56+01:00
New Revision: 9d87739f664b5b454ff78a3016ab05a1987f0d7c

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

LOG: [clangd] AddUsing: do not crash on non-namespace using decls.

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

Added: 


Modified: 
clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
index 4b3fbc02c411..b00c2716005c 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -152,12 +152,14 @@ findInsertionPoint(const Tweak::Selection ,
 if (SM.isBeforeInTranslationUnit(Inputs.Cursor, U->getUsingLoc()))
   // "Usings" is sorted, so we're done.
   break;
-if (U->getQualifier()->getAsNamespace()->getCanonicalDecl() ==
-QualifierToRemove.getNestedNameSpecifier()
-->getAsNamespace()
-->getCanonicalDecl() &&
-U->getName() == Name) {
-  return InsertionPointData();
+if (const auto *Namespace = U->getQualifier()->getAsNamespace()) {
+  if (Namespace->getCanonicalDecl() ==
+  QualifierToRemove.getNestedNameSpecifier()
+  ->getAsNamespace()
+  ->getCanonicalDecl() &&
+  U->getName() == Name) {
+return InsertionPointData();
+  }
 }
 
 // Insertion point will be before last UsingDecl that affects cursor

diff  --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp 
b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index 4a2360dda739..eefc50d754e2 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2884,6 +2884,17 @@ using xx::yy;
 void fun() {
   yy();
 }
+)cpp"},
+// Existing using with non-namespace part.
+{R"cpp(
+#include "test.hpp"
+using one::two::ee::ee_one;
+one::t^wo::cc c;
+)cpp",
+ R"cpp(
+#include "test.hpp"
+using one::two::cc;using one::two::ee::ee_one;
+cc c;
 )cpp"}};
   llvm::StringMap EditedFiles;
   for (const auto  : Cases) {
@@ -2892,7 +2903,7 @@ void fun() {
 namespace one {
 void oo() {}
 namespace two {
-enum ee {};
+enum ee {ee_one};
 void ff() {}
 class cc {
 public:



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

[PATCH] D92186: [clangd] AddUsing: do not crash on non-namespace using decls.

[Haojian Wu via Phabricator via cfe-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92186

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

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

[Sylvain Audi via Phabricator via cfe-commits]

saudi created this revision.
saudi added reviewers: arphaman, jkolek, Bigcheese, kousikk.
saudi added a project: clang.
Herald added subscribers: cfe-commits, tschuett, ormris.
saudi requested review of this revision.

clang-scan-deps contains some command line parsing and modifications.
This patch adds support for clang-cl command options.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92191

Files:
  clang/test/ClangScanDeps/Inputs/has_include_if_elif.json
  clang/test/ClangScanDeps/Inputs/header_stat_before_open_cdb.json
  clang/test/ClangScanDeps/Inputs/headerwithdirname.json
  clang/test/ClangScanDeps/Inputs/headerwithdirnamefollowedbyinclude.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json
  clang/test/ClangScanDeps/Inputs/no-werror.json
  clang/test/ClangScanDeps/Inputs/regular_cdb_clangcl.json
  clang/test/ClangScanDeps/Inputs/static-analyzer-cdb.json
  clang/test/ClangScanDeps/Inputs/strip_diag_serialize.json
  clang/test/ClangScanDeps/Inputs/vfsoverlay_cdb.json
  clang/test/ClangScanDeps/error.cpp
  clang/test/ClangScanDeps/has_include_if_elif.cpp
  clang/test/ClangScanDeps/header_stat_before_open.m
  clang/test/ClangScanDeps/headerwithdirname.cpp
  clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules.cpp
  clang/test/ClangScanDeps/no-werror.cpp
  clang/test/ClangScanDeps/regular_cdb.cpp
  clang/test/ClangScanDeps/static-analyzer.c
  clang/test/ClangScanDeps/strip_diag_serialize.cpp
  clang/test/ClangScanDeps/vfsoverlay.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -12,6 +12,8 @@
 #include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/InitLLVM.h"
@@ -49,7 +51,8 @@
   /// option and cache the results for reuse. \returns resource directory path
   /// associated with the given invocation command or empty string if the
   /// compiler path is NOT an absolute path.
-  StringRef findResourceDir(const tooling::CommandLineArguments ) {
+  StringRef findResourceDir(const tooling::CommandLineArguments ,
+bool ClangCLMode) {
 if (Args.size() < 1)
   return "";
 
@@ -65,8 +68,12 @@
 if (CachedResourceDir != Cache.end())
   return CachedResourceDir->second;
 
-std::vector PrintResourceDirArgs{ClangBinaryName,
-"-print-resource-dir"};
+std::vector PrintResourceDirArgs{ClangBinaryName};
+if (ClangCLMode)
+  PrintResourceDirArgs.push_back("/clang:-print-resource-dir");
+else
+  PrintResourceDirArgs.push_back("-print-resource-dir");
+
 llvm::SmallString<64> OutputFile, ErrorFile;
 llvm::sys::fs::createTemporaryFile("print-resource-dir-output",
"" /*no-suffix*/, OutputFile);
@@ -421,43 +428,80 @@
 bool HasMQ = false;
 bool HasMD = false;
 bool HasResourceDir = false;
+bool ClangCLMode = false;
+
 // We need to find the last -o value.
 if (!Args.empty()) {
+  ClangCLMode =
+  llvm::sys::path::stem(Args[0]).contains_lower("clang-cl") ||
+  llvm::is_contained(Args, "--driver-mode=cl");
+
   std::size_t Idx = Args.size() - 1;
   for (auto It = Args.rbegin(); It != Args.rend(); ++It) {
-if (It != Args.rbegin()) {
-  if (Args[Idx] == "-o")
+StringRef Arg = Args[Idx];
+if (ClangCLMode) {
+  if (Arg.startswith("/Fo") || Arg.startswith("-Fo"))
+LastO = Arg.drop_front(3).str();
+  if (Arg == "/clang:-MT")
+HasMT = true;
+  if (Arg == "/clang:-MQ")
+HasMQ = true;
+  if (Arg == "/clang:-MD")
+HasMD = true;
+} else {
+  if (It != Args.rbegin() && Arg == "-o")
 LastO = Args[Idx + 1];
-  if (Args[Idx] == "-MT")
+  if (Arg == "-MT")
 HasMT = true;
-  if (Args[Idx] == "-MQ")
+  if (Arg == "-MQ")
 HasMQ = true;
-  if (Args[Idx] == "-MD")
+  if (Arg == "-MD")
 HasMD = true;
-  if (Args[Idx] == "-resource-dir")
-HasResourceDir = true;
 }
+if (Arg == "-resource-dir")
+  HasResourceDir = true;
 --Idx;
   }
 }
 // If 

[PATCH] D78903: [Driver] Add option -fproc-stat-report

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added inline comments.



Comment at: clang/test/Driver/report-stat.c:1
+// RUN: %clang -c -fproc-stat-report %s | FileCheck %s
+// CHECK: clang{{.*}}: output={{.*}}.o, total={{[0-9.]+}} ms, user={{[0-9.]+}} 
ms, mem={{[0-9]+}} Kb

Hi @sepavloff, is it possible to add `-fintegrated-as` to the clang invocation? 
 

As it is, the test fails on our AIX target because we don't use the integrated 
assembler by default. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78903

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

[PATCH] D90944: [clang-tidy] implement concurrency-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 307903.
segoon added a comment.

- revert decls marking


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
  clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
  clang-tools-extra/clang-tidy/concurrency/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/concurrency/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp
  clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp
  clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t -- -config='{CheckOptions: [{key: "concurrency-mt-unsafe.FunctionSet", value: "posix"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  ::sleep(2);
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [concurrency-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+}
Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t -- -config='{CheckOptions: [{key: "concurrency-mt-unsafe.FunctionSet", value: "glibc"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  dirname(nullptr);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [concurrency-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -138,6 +138,7 @@
`clang-analyzer-valist.CopyToSelf `_,
`clang-analyzer-valist.Uninitialized `_,
`clang-analyzer-valist.Unterminated `_,
+   `concurrency-mt-unsafe `_,
`cppcoreguidelines-avoid-goto `_,
`cppcoreguidelines-avoid-non-const-global-variables `_,
`cppcoreguidelines-init-variables `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst
@@ -0,0 +1,52 @@
+.. title:: clang-tidy - concurrency-mt-unsafe
+
+concurrency-mt-unsafe
+=
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without

[PATCH] D91373: [OpenMP5.0] Support more kinds of lvalues in map clauses

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16869
+// Allow results of method calls to be mapped.
+if (isa(ME->getMemberDecl())) {
+  RelevantExpr = ME;

I don't think it should always return `true`. What about `map(s.foo)` where 
`foo` is a member function?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16939
+// forward to the source expression.
+return Visit(OVE->getSourceExpr()->IgnoreParenImpCasts());
+  }

Same, too general check, plus in some cases `OVE->getSourceExpr()` may return 
`nullptr`.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:17072
   bool VisitUnaryOperator(UnaryOperator *UO) {
-if (SemaRef.getLangOpts().OpenMP < 50 || !UO->isLValue() ||
-UO->getOpcode() != UO_Deref) {
+if (SemaRef.getLangOpts().OpenMP < 50 ||
+(Components.empty() &&

What is this for?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:17115-17124
+  bool VisitCallExpr(CallExpr *CE) {
+if (SemaRef.getLangOpts().OpenMP < 50 ||
+(Components.empty() && !CE->isLValue())) {
+  emitErrorMsg();
+  return false;
+}
+assert(!RelevantExpr && "RelevantExpr is expected to be nullptr");

```
int a;
int () {return a;}

...
#pragma omp target map(foo())
 foo() = 0;
...

```
How is this supposed to work?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:17125-17144
+  bool VisitCastExpr(CastExpr *CE) {
+if (SemaRef.getLangOpts().OpenMP < 50 ||
+(Components.empty() && !CE->isLValue())) {
+  emitErrorMsg();
+  return false;
+}
+assert(!RelevantExpr && "RelevantExpr is expected to be nullptr");

Same questions here, how's the actual mapping is supposed to work? Need some 
more detailed description. I don't think this is going to be easy to implement 
it directly. We map the addresses of the base declarations but in your cases 
there is just no base declarations. SO, you need to create one. To me, this 
should look like this:
```
#pragma omp target map()
{
   = xxx;
  yyy = ;
}
```
|
V
```
auto  = ;
#pragma omp target map(mapped)
{
  mapped = xxx;
  yyy = mapped;
}
```



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

https://reviews.llvm.org/D91373

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

Do we have any buildbot coverage for gnux32?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D91531: [RFC][OpenCL] Provide mechanisms for defining extension macros

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D91531#2418237 , @mantognini wrote:

> When reading the documentation [1] for -cl-ext (which I've never used so 
> far), I've noticed nothing is said about non-standard configurations (such as 
> disabling cl_khr_depth_images with CL2.0). Quickly testing this shows that 
> options can be specified to produce non-standard behaviour, as shown by 
> https://godbolt.org/z/1Yz1Md.
>
> Is it intentional that -cl-ext allows such non-standard behaviour? (This 
> question is not necessarily address to @Anastasia.)
> /If so/, then these statements
>
>> Defining __undef_cl_khr_depth_images can alter the default behavior of the 
>> predefined macro. This is equivalent to passing -cl-ext=-cl_khr_depth_images.
>
> and
>
>> cl_khr_depth_images is a core functionality of CL2.0 and thefore defining 
>> __undef_cl_khr_depth_images doesn't modify the default behavior.
>
> are slightly contradicting each other: the approach with __undef macros seems 
> to ensure a more conformant behaviour.
>
> I'm mainly asking for clarification in order to know in which direction we 
> want to go, as one could also argue the present documentation doesn't imply 
> non-standard behaviour is desired and that the current implementation of 
> -cl-ext is buggy.
>
> [1] https://clang.llvm.org/docs/UsersManual.html#cmdoption-cl-ext

Thanks! This is a very good point. I am not entirely clear of the intended use 
case. In this patch I have only tried to preserve existing functionality. To my 
memory the flag was added to simplify life of downstream implementations. 
However if the intent of it is not clear now perhaps it shouldn't really be 
part of upstream code base. Otherwise all it does is complicating future 
development. I suggest we try to clarify the following:

1. How/where `-cl-ext` is used.
2. Does its use (if any) still justify maintenance and further development?
3. Do we need equivalent functionality for extensions that are not part of 
clang i.e. library extensions?


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

https://reviews.llvm.org/D91531

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

[PATCH] D90944: [clang-tidy] implement concurrency-mt-unsafe

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D90944#2418792 , @segoon wrote:

> - move static vectors out of namespace

Nice.

> - mark mt-unsafe decls and check for marks in exprs

Eeeh.
I was thinking of either some smart matcher "match any function declaration
with name from these lists, and then match every call to said decl".
But the current implementation, i'm not sure this approach is even legal for 
checks.
So i guess let's stick with the original solution..


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

https://reviews.llvm.org/D90944

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

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added a comment.

> Right, I understand (a little bit, at least!) what plugins *can* do. What I'm 
> asking specifically is: this feature has a cost, how important is supporting 
> it? Are there codebases where these attributes are widely used, and 
> enforcement at development time is particularly valuable?

Well, I can't tell you how widely plugin used, because they often used in 
third-party projects. Firefox team seems use them a lot, the `ParsedAttrInfo` 
plugin implementation seems write by them if I remember right. And our team use 
both plugins and clangd a lot, that's why I'm willing to contribute to make 
them work together. As about the importance, my personal opinion is that since 
there's a feature, we should try to polish it...

> Sorry, I probably should have said "can plugins be thread-hostile".
> Clangd runs several parsing actions in parallel, each on a single separate 
> thread.
> If a plugin assumes there's only a single instance of it running and uses 
> static data accordingly, we're going to hit problems in clangd. 
> The problem is that precisely because clang uses a single thread, (some) 
> plugins may think this is a reasonable assumption. And it's not possible to 
> tell "from the outside" whether a given plugin is unsafe in this way.

That seems a serious problem, but I got a question, what if the clang itself 
declared some static data (I'll investigate later, just ask for now)?

> It looks like this patch moves `LoadLibraryPermanently` to a different 
> location, per the patch description clangd does not currently load plugins. 
> That's why I'm concerned this patch may introduce unsafe loading of untrusted 
> .sos.

Well, since it called plugin, it's users choose to use it or not, I personally 
think we can't check if an `.so` is trusted, and neither should we care...

> SkipFunctionBodies is an option in FrontendOpts that produces a different AST 
> that clang itself (which does not set this option) can't produce. Basically 
> I'm saying "can we tell if a plugin is going to work with our weird ASTs, or 
> do we just have to try and maybe crash".

I'll test this.

> - the filesystem access to the libraries isn't virtualized (vfs) and I don't 
> know if it really can be.

Sorry I still can't understand this, could you help to explain more? What I 
mean before is that since this part of code is used in clang driver as well, it 
should be no problem get called by clangd.

>>> - consequences of bad behavior/crash are worse, because clangd is 
>>> long-running
>>
>> Does clangd shares the same process with an `FrontendAction`? Because in 
>> clang, if a plugin carshes, the whole process does terminated, clang didn't 
>> handle this yet(just a core dump).
>
> Yes. And I'm not convinced it's possible to handle this safely without large 
> architectural changes.

Well, I guess currently I can do nothing to improve this, but if a user uses a 
plugin, it crashed, that's should be the plugin's problem...

> Absent that, I think we probably shouldn't enable them (beyond an experiment).

How about capsule the loading functionalities to a function, and call it from 
clangd if experimental feature is checked? In this way nothing changed if the 
clangd's experimental feature is off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D91311: Add new 'preferred_name' attribute.

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D91311#2417293 , @rsmith wrote:

> In D91311#2416940 , @ldionne wrote:
>
>> LGTM from the libc++ point of view. The CI is passing -- those failures are 
>> flaky modules tests that we need to fix.
>
> Perhaps we need to specify `-fmodules-validate-system-headers` in the test so 
> Clang doesn't assume that system headers are unchanged?

Oh, would that be it? We're not including libc++ headers as system headers when 
running the tests, though (and we're disabling the system header pragma). Do 
you think that might still be the issue?

I tried a dumb workaround with D92131 , but 
it's arguably not great. I'd love to have your thoughts on that. We've been 
seeing these issues for a while when we make some types of changes in the 
`__config_site`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311

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

[PATCH] D90928: [OpenCL] Check for extension string extension lookup

[Anastasia Stulova via Phabricator via cfe-commits]

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

Great! LGTM! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90928

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

[PATCH] D90944: [clang-tidy] implement concurrency-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon added a comment.

In D90944#2418845 , @lebedev.ri wrote:

> In D90944#2418792 , @segoon wrote:
>
>> - mark mt-unsafe decls and check for marks in exprs
>
> Eeeh.
> I was thinking of either some smart matcher "match any function declaration
> with name from these lists, and then match every call to said decl".

I tried to utilize bind()+equalsBoundNode(), but it seems it's impossible to 
mark and use the mark in a single matcher.

> But the current implementation, i'm not sure this approach is even legal for 
> checks.

The trick is stolen from abseil/UpgradeDurationConversionsCheck.h. If it's 
invalid here, then abseil should be fixed too.


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

https://reviews.llvm.org/D90944

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

[PATCH] D92175: [clang-tidy] Add support for loading configs for specific files.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D92175#2418651 , @sammccall wrote:

> Currently AFAICS there's just *one* check that ever reads the config for 
> other files, and it could be redesigned not to do so - by conservatively 
> renaming only identifiers declared in the current file.

You've missed the point behind this. The purpose for this functionality is 
clang-tidy (usually) only gets ran on implementation files, This makes it hard 
to enforce specific checks on header files.
You're best bet is hoping that all the header files you include are using the 
same .clang-tidy config. 
This gets even more challenging when using the run_clang_tidy script over a 
compilation database. If there isn't one single project wide configuration, 
You'll likely get conflicting errors and fixes.
Also there is only *one* check at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92175

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

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: clang/lib/AST/ASTImporter.cpp:5161
 
-  // FIXME: Import default argument  and constraint expression.
+  // FIXME: Import constraint expression.
 

martong wrote:
> I wonder whether this FIXME is already fixed, at line 5182?
I think so. I removed the whole comment instead. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92103

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

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3f6c856bb5ae: [ASTImporter] Import the default argument of 
TemplateTypeParmDecl (authored by teemperor).
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D92103?vs=307614=307891#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92103

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  
lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/list/TestListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/stack/TestStackFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
  
lldb/test/API/commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py

Index: lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -22,7 +22,7 @@
 
 self.runCmd("settings set target.import-std-module true")
 
-vector_type = "std::vector >"
+vector_type = "std::vector"
 size_type = vector_type + "::size_type"
 value_type = "std::__vector_base >::value_type"
 iterator = vector_type + "::iterator"
Index: lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
@@ -20,13 +20,9 @@
   "// Set break point at this line.",
   lldb.SBFileSpec("main.cpp"))
 
-vector_type = "std::vector >"
-vector_of_vector_type = "std::vector<" + vector_type + \
-", std::allocator > > >"
-size_type = (
-"std::vector >, " +
-"std::allocator > >" +
-" >::size_type")
+vector_type = "std::vector"
+vector_of_vector_type = "std::vector<" + vector_type + " >"
+size_type = vector_of_vector_type + "::size_type"
 value_type = "std::__vector_base >::value_type"
 
 self.runCmd("settings set target.import-std-module true")
@@ -35,13 +31,13 @@
 "a",
 result_type=vector_of_vector_type,
 result_children=[
-ValueCheck(type="std::vector >",
+ValueCheck(type="std::vector",
children=[
ValueCheck(value='1'),
ValueCheck(value='2'),
ValueCheck(value='3'),
]),
-ValueCheck(type="std::vector >",
+ValueCheck(type="std::vector",
children=[
ValueCheck(value='3'),
ValueCheck(value='2'),
Index: lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
@@ -23,7 +23,7 @@
 
 

[clang] 3f6c856 - [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via cfe-commits]

Author: Raphael Isemann
Date: 2020-11-26T18:01:30+01:00
New Revision: 3f6c856bb5ae4426a586426bca9f1ef2848a2b12

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

LOG: [ASTImporter] Import the default argument of TemplateTypeParmDecl

The test case isn't using the AST matchers for all checks as there doesn't seem 
to be support for
matching TemplateTypeParmDecl default arguments. Otherwise this is simply 
importing the
default arguments.

Also updates several LLDB tests that now as intended omit the default template
arguments of several std templates.

Reviewed By: martong

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

Added: 


Modified: 
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp

lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py

lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py

lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py

lldb/test/API/commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py

lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py

lldb/test/API/commands/expression/import-std-module/list/TestListFromStdModule.py

lldb/test/API/commands/expression/import-std-module/queue/TestQueueFromStdModule.py

lldb/test/API/commands/expression/import-std-module/stack/TestStackFromStdModule.py

lldb/test/API/commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py

lldb/test/API/commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py

lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py

lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py

lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py

lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py

Removed: 




diff  --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 835551528e0d..5159682da85f 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -5158,8 +5158,6 @@ 
ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   // context. This context will be fixed when the actual template declaration
   // is created.
 
-  // FIXME: Import default argument  and constraint expression.
-
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
 return BeginLocOrErr.takeError();
@@ -5206,6 +5204,14 @@ 
ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
 ToIDC);
   }
 
+  if (D->hasDefaultArgument()) {
+Expected ToDefaultArgOrErr =
+import(D->getDefaultArgumentInfo());
+if (!ToDefaultArgOrErr)
+  return ToDefaultArgOrErr.takeError();
+ToD->setDefaultArgument(*ToDefaultArgOrErr);
+  }
+
   return ToD;
 }
 

diff  --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 33e4b7226fba..5a93a7348e7a 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -880,6 +880,25 @@ TEST_P(ImportExpr, DependentSizedArrayType) {
  has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTypeParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl("template struct X {};", Lang_CXX03);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTypeParmDecl(hasName("T")));
+  TemplateTypeParmDecl *To = Import(From, Lang_CXX03);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTypeParmDeclDefaultArg) {
+  Decl *FromTU =
+  getTuDecl("template struct X {};", Lang_CXX03);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTypeParmDecl(hasName("T")));
+  TemplateTypeParmDecl *To = Import(From, Lang_CXX03);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  QualType ToArg = To->getDefaultArgument();
+  ASSERT_EQ(ToArg, QualType(To->getASTContext().IntTy));
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, ImportBeginLocOfDeclRefExpr) {
   Decl *FromTU =
   getTuDecl("class A { public: static int X; }; void f() { (void)A::X; }",

diff  --git 
a/lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
 
b/lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
index 18bd8ae37ff9..0eaa50a12727 100644
--- 

[PATCH] D91656: [clang-tidy] add concurrency module

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 307883.
segoon added a comment.

- fix sort order


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

https://reviews.llvm.org/D91656

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
  clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst

Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -64,6 +64,8 @@
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``concurrency-``   Checks related to concurrent programming (including
+   threads, fibers, coroutines, etc.).
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``darwin-``Checks related to Darwin coding conventions.
 ``fuchsia-``   Checks related to Fuchsia coding conventions.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -82,6 +82,11 @@
   `Altera SDK for OpenCL: Best Practices Guide
   `_.
 
+- New ``concurrency`` module.
+
+  Includes checks related to concurrent programming (e.g. threads, fibers,
+  coroutines, etc.).
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
@@ -0,0 +1,33 @@
+//===--- ConcurrencyTidyModule.cpp - clang-tidy ---===//
+//
+// 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
+//
+//===--===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+
+namespace clang {
+namespace tidy {
+namespace concurrency {
+
+class ConcurrencyModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories ) override {}
+};
+
+} // namespace concurrency
+
+// Register the ConcurrencyTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add
+X("concurrency-module", "Adds concurrency checks.");
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the ConcurrencyModule.
+volatile int ConcurrencyModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
@@ -0,0 +1,22 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_clang_library(clangTidyConcurrencyModule
+  ConcurrencyTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUtils
+  )
+
+clang_target_link_libraries(clangTidyConcurrencyModule
+  PRIVATE
+  clangAnalysis
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangSerialization
+  clangTooling
+  )
Index: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
===
--- clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -45,6 +45,11 @@
 static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination =
 CERTModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ConcurrencyModule.
+extern volatile int ConcurrencyModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ConcurrencyModuleAnchorDestination =
+ConcurrencyModuleAnchorSource;
+
 // This anchor is used to force the linker to link the CppCoreGuidelinesModule.
 extern volatile int CppCoreGuidelinesModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -55,6 +55,7 @@
 add_subdirectory(boost)
 add_subdirectory(bugprone)
 add_subdirectory(cert)
+add_subdirectory(concurrency)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(darwin)
 add_subdirectory(fuchsia)
@@ -81,6 +82,7 @@
   

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3902
 
+  // Load any requested plugins.
+  for (const std::string  : Res.getFrontendOpts().Plugins) {

sammccall wrote:
> how does this code behave if CLANG_PLUGIN_SUPPORT is off?
If `CLANG_PLUGIN_SUPPORT ` is off, than no symbols are exported from a 
executable, than load will failed with `undefined symbols`, a diagnostic will 
be reported and the plugin will not be loaded. The main procedure will not be 
affected.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3905
+std::string Error;
+if (llvm::sys::DynamicLibrary::LoadLibraryPermanently(Path.c_str(), 
))
+  Diags.Report(diag::err_fe_unable_to_load_plugin) << Path << Error;

sammccall wrote:
> is this threadsafe on all platforms?
While I can't tell, clang driver use this for a while and seems no problem, 
could you help to point out what's the worst case your concerned about?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3914
+if (P->getActionType() == PluginASTAction::ReplaceAction) {
+  Res.getFrontendOpts().ProgramAction = clang::frontend::PluginAction;
+  Res.getFrontendOpts().ActionName = Plugin.getName().str();

sammccall wrote:
> we can't support replacement of the main action in clangd. What's the plan 
> there - ignore the plugin?
Could you help to explain why action replacements are not supported? 

As far as I know, replacement of actions is related with Actions, which does 
not have directly relationship with clangd,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D43159: [libc++] Replace several uses of 0 by nullptr

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 307888.
ldionne marked 3 inline comments as done.
ldionne added a comment.

Apply review comments. Thanks for the catches!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43159

Files:
  libcxx/include/__locale
  libcxx/include/__sso_allocator
  libcxx/include/__string
  libcxx/include/__threading_support
  libcxx/include/algorithm
  libcxx/include/bitset
  libcxx/include/chrono
  libcxx/include/fstream
  libcxx/include/functional
  libcxx/include/ios
  libcxx/include/istream
  libcxx/include/iterator
  libcxx/include/locale
  libcxx/include/memory
  libcxx/include/regex
  libcxx/include/sstream
  libcxx/include/streambuf
  libcxx/include/string
  libcxx/include/strstream
  libcxx/include/system_error
  libcxx/include/valarray
  libcxx/src/new.cpp
  libcxxabi/src/stdlib_new_delete.cpp

Index: libcxxabi/src/stdlib_new_delete.cpp
===
--- libcxxabi/src/stdlib_new_delete.cpp
+++ libcxxabi/src/stdlib_new_delete.cpp
@@ -27,7 +27,7 @@
 if (size == 0)
 size = 1;
 void* p;
-while ((p = ::malloc(size)) == 0)
+while ((p = ::malloc(size)) == nullptr)
 {
 // If malloc fails and there is a new_handler,
 // call it to try free up memory.
@@ -48,7 +48,7 @@
 void*
 operator new(size_t size, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCXXABI_NO_EXCEPTIONS
 try
 {
@@ -74,7 +74,7 @@
 void*
 operator new[](size_t size, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCXXABI_NO_EXCEPTIONS
 try
 {
@@ -170,7 +170,7 @@
 void*
 operator new(size_t size, std::align_val_t alignment, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCXXABI_NO_EXCEPTIONS
 try
 {
@@ -196,7 +196,7 @@
 void*
 operator new[](size_t size, std::align_val_t alignment, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCXXABI_NO_EXCEPTIONS
 try
 {
Index: libcxx/src/new.cpp
===
--- libcxx/src/new.cpp
+++ libcxx/src/new.cpp
@@ -64,7 +64,7 @@
 if (size == 0)
 size = 1;
 void* p;
-while ((p = ::malloc(size)) == 0)
+while ((p = ::malloc(size)) == nullptr)
 {
 // If malloc fails and there is a new_handler,
 // call it to try free up memory.
@@ -85,7 +85,7 @@
 void*
 operator new(size_t size, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCPP_NO_EXCEPTIONS
 try
 {
@@ -111,7 +111,7 @@
 void*
 operator new[](size_t size, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCPP_NO_EXCEPTIONS
 try
 {
@@ -207,7 +207,7 @@
 void*
 operator new(size_t size, std::align_val_t alignment, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCPP_NO_EXCEPTIONS
 try
 {
@@ -233,7 +233,7 @@
 void*
 operator new[](size_t size, std::align_val_t alignment, const std::nothrow_t&) _NOEXCEPT
 {
-void* p = 0;
+void* p = nullptr;
 #ifndef _LIBCPP_NO_EXCEPTIONS
 try
 {
Index: libcxx/include/valarray
===
--- libcxx/include/valarray
+++ libcxx/include/valarray
@@ -802,7 +802,7 @@
 public:
 // construct/destroy:
 _LIBCPP_INLINE_VISIBILITY
-valarray() : __begin_(0), __end_(0) {}
+valarray() : __begin_(nullptr), __end_(nullptr) {}
 inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1
 explicit valarray(size_t __n);
 _LIBCPP_INLINE_VISIBILITY
@@ -2750,8 +2750,8 @@
 template 
 inline
 valarray<_Tp>::valarray(size_t __n)
-: __begin_(0),
-  __end_(0)
+: __begin_(nullptr),
+  __end_(nullptr)
 {
 if (__n)
 {
@@ -2776,16 +2776,16 @@
 template 
 inline
 valarray<_Tp>::valarray(const value_type& __x, size_t __n)
-: __begin_(0),
-  __end_(0)
+: __begin_(nullptr),
+  __end_(nullptr)
 {
 resize(__n, __x);
 }
 
 template 
 valarray<_Tp>::valarray(const value_type* __p, size_t __n)
-: __begin_(0),
-  __end_(0)
+: __begin_(nullptr),
+  __end_(nullptr)
 {
 if (__n)
 {
@@ -2809,8 +2809,8 @@
 
 template 
 valarray<_Tp>::valarray(const valarray& __v)
-: __begin_(0),
-  __end_(0)
+: __begin_(nullptr),
+  __end_(nullptr)
 {
 if (__v.size())
 {
@@ -2845,8 +2845,8 @@
 
 template 
 valarray<_Tp>::valarray(initializer_list __il)
-: __begin_(0),
-  __end_(0)
+: __begin_(nullptr),
+  __end_(nullptr)
 {
 const size_t __n = __il.size();
 if (__n)
@@ -2874,8 +2874,8 @@
 
 template 
 valarray<_Tp>::valarray(const slice_array& __sa)
-: __begin_(0),
-  __end_(0)
+: __begin_(nullptr),
+  __end_(nullptr)
 {
 const size_t __n = 

[PATCH] D92186: [clangd] AddUsing: do not crash on non-namespace using decls.

[Adam Czachorowski via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92186

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2884,6 +2884,17 @@
 void fun() {
   yy();
 }
+)cpp"},
+// Existing using with non-namespace part.
+{R"cpp(
+#include "test.hpp"
+using one::two::ee::ee_one;
+one::t^wo::cc c;
+)cpp",
+ R"cpp(
+#include "test.hpp"
+using one::two::cc;using one::two::ee::ee_one;
+cc c;
 )cpp"}};
   llvm::StringMap EditedFiles;
   for (const auto  : Cases) {
@@ -2892,7 +2903,7 @@
 namespace one {
 void oo() {}
 namespace two {
-enum ee {};
+enum ee {ee_one};
 void ff() {}
 class cc {
 public:
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -152,12 +152,14 @@
 if (SM.isBeforeInTranslationUnit(Inputs.Cursor, U->getUsingLoc()))
   // "Usings" is sorted, so we're done.
   break;
-if (U->getQualifier()->getAsNamespace()->getCanonicalDecl() ==
-QualifierToRemove.getNestedNameSpecifier()
-->getAsNamespace()
-->getCanonicalDecl() &&
-U->getName() == Name) {
-  return InsertionPointData();
+if (const auto *Namespace = U->getQualifier()->getAsNamespace()) {
+  if (Namespace->getCanonicalDecl() ==
+  QualifierToRemove.getNestedNameSpecifier()
+  ->getAsNamespace()
+  ->getCanonicalDecl() &&
+  U->getName() == Name) {
+return InsertionPointData();
+  }
 }
 
 // Insertion point will be before last UsingDecl that affects cursor


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2884,6 +2884,17 @@
 void fun() {
   yy();
 }
+)cpp"},
+// Existing using with non-namespace part.
+{R"cpp(
+#include "test.hpp"
+using one::two::ee::ee_one;
+one::t^wo::cc c;
+)cpp",
+ R"cpp(
+#include "test.hpp"
+using one::two::cc;using one::two::ee::ee_one;
+cc c;
 )cpp"}};
   llvm::StringMap EditedFiles;
   for (const auto  : Cases) {
@@ -2892,7 +2903,7 @@
 namespace one {
 void oo() {}
 namespace two {
-enum ee {};
+enum ee {ee_one};
 void ff() {}
 class cc {
 public:
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -152,12 +152,14 @@
 if (SM.isBeforeInTranslationUnit(Inputs.Cursor, U->getUsingLoc()))
   // "Usings" is sorted, so we're done.
   break;
-if (U->getQualifier()->getAsNamespace()->getCanonicalDecl() ==
-QualifierToRemove.getNestedNameSpecifier()
-->getAsNamespace()
-->getCanonicalDecl() &&
-U->getName() == Name) {
-  return InsertionPointData();
+if (const auto *Namespace = U->getQualifier()->getAsNamespace()) {
+  if (Namespace->getCanonicalDecl() ==
+  QualifierToRemove.getNestedNameSpecifier()
+  ->getAsNamespace()
+  ->getCanonicalDecl() &&
+  U->getName() == Name) {
+return InsertionPointData();
+  }
 }
 
 // Insertion point will be before last UsingDecl that affects cursor
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90944: [clang-tidy] implement concurrency-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 307884.
segoon added a comment.

- move static vectors out of namespace
- mark mt-unsafe decls and check for marks in exprs


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
  clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
  clang-tools-extra/clang-tidy/concurrency/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/concurrency/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp
  clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp
  clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t -- -config='{CheckOptions: [{key: "concurrency-mt-unsafe.FunctionSet", value: "posix"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  ::sleep(2);
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [concurrency-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+}
Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t -- -config='{CheckOptions: [{key: "concurrency-mt-unsafe.FunctionSet", value: "glibc"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  dirname(nullptr);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [concurrency-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -138,6 +138,7 @@
`clang-analyzer-valist.CopyToSelf `_,
`clang-analyzer-valist.Uninitialized `_,
`clang-analyzer-valist.Unterminated `_,
+   `concurrency-mt-unsafe `_,
`cppcoreguidelines-avoid-goto `_,
`cppcoreguidelines-avoid-non-const-global-variables `_,
`cppcoreguidelines-init-variables `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst
@@ -0,0 +1,52 @@
+.. title:: clang-tidy - concurrency-mt-unsafe
+
+concurrency-mt-unsafe
+=
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe 

[PATCH] D92091: [OpenCL] Allow pointer-to-pointer kernel args beyond CL 1.2

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:8638
 // array, this recursive call only happens once.
 return getOpenCLKernelParameterType(S, QualType(UnderlyingTy, 0));
   }

Btw I am surprised that we recurse to check the underlying type of an array but 
not the pointer. I guess this spec wording implies that pointed types should be 
checked too?

> The   size   in   bytes   of   these   types   areimplementation-defined  and 
>  in  addition  can  also  be  different  for  the  OpenCL  device  and  the 
> host processor making it difficult to allocate buffer objects to be passed as 
> arguments to a kernel declared as pointer to these types.


Also I can't find anything specific to the arrays...

Do you think we need to chase this with the spec?



Comment at: clang/lib/Sema/SemaDecl.cpp:8669
+// Loop through inner pointer types to ensure address spaces are valid.
+QualType NestedPT = PT->getPointeeType();
+do {

I feel this breaks the original flow a little bit. It would have been nicer if 
this could be part of `getOpenCLKernelParameterType`, but it might become more 
complex then... We would probably need to introduce something like 
`ValidPtrPtrKernelParam` and `InvalidAddrSpacePtrPtrKernelParam`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92091

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

[PATCH] D80450: [CUDA][HIP] Fix implicit HD function resolution

[Yaxun Liu via Phabricator via cfe-commits]

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

reopen for fixing the regression


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80450

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

[PATCH] D80450: [CUDA][HIP] Fix implicit HD function resolution

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D80450#2088129 , @tra wrote:

> In D80450#2087938 , @tra wrote:
>
>> Reproducer for the regression. 
>> https://gist.github.com/Artem-B/183e9cfc28c6b04c1c862c853b5d9575
>> It's not particularly small, but that's as far as I could get it reduced.
>>
>> With the patch, an attempt to instantiate `ag` on line 36 (in the reproducer 
>> sources I linked to above) results in ambiguity between two templates on 
>> lines 33 and 24 that are in different namespaces.
>> Previously it picked the template on line 28.
>
> Managed to simplify the reproducer down to this which now reports that a host 
> candidate has been ignored. This may explain why we ended up with the 
> ambiguity when other overloads were present.
>
>   template  struct a {};
>   namespace b {
>   struct c : a {};
>   template  void ag(d);
>   } // namespace b
>   template 
>   __attribute__((host)) __attribute__((device)) int ag(a) {
> ae e;
> ag(e);
>   }
>   void f() { ag; }

The error only happens in device compilation.

For the call `ag(e)`. There are two candidates:

1. `ag` in namespace `b`. The function arguments can match. However it is a 
host function, therefore is a wrong-sided candidate and not viable.

2. `ag` in default name space. It is a host device function. However the 
function arguments requires `a`, therefore cannot match.

Before my patch, wrong-sided candidate is allowed. clang resolves to candidate 
1 and this results in a diagnostic about host function referenced in device 
host function, which can be deferred. Since `f()` is not emitted on device 
side, the deferred diags is not emitted.

After my patch, wrong-sided candidate is not allowed. clang resolves to 
candidate 2, which results in a diagnostic that no matching function, which is 
not a deferred diagnostics by default and is emitted even if `f()` is not 
emitted on device side.

In a sense, the diagnostic is correct, since `ag(a)` cannot be emitted on 
device side. This can be fixed by either make `ag(a)` a host function or 
make `ag(d)` a host device function.

In the original testcase 
(https://gist.github.com/Artem-B/183e9cfc28c6b04c1c862c853b5d9575)

Before my change,  call at line 36 resolves to wrong-sided candidate at line 29 
since that is the best match for argument types. This results in a deferred 
diag which allows device compilation to pass.

After my change, call at line 36 resolves to two host device candidates. This 
results in diagnostics about ambiguity which is not deferred by default. 
Therefore the compilation fails.

Basically it all boils down to the issue that overloading resolution 
diagnostics are not deferred by default.

I think first of all we need to exclude wrong-sided candidates as this patch 
does, otherwise we cannot have correct hostness based overloading resolution 
and fix bugs like https://bugs.llvm.org/show_bug.cgi?id=46922 .

However by doing this we changes the existing overloading resolution incur some 
overloading resolution diags. Unless we defer these diags, we may break some 
existing CUDA/HIP code.

Fortunately https://reviews.llvm.org/D84364 is already landed, which allows 
deferring overloading resolution diags under option -fgpu-defer-diags.

I think a conservative solution is that we keep the old overloading resolution 
behavior by default (i.e. do not exclude wrong-sided candidates), whereas 
enable the correct hostness based overloading resolution when -fgpu-defer-diags 
is on. If developers would like correct hostness based overloading resolution, 
they can use -fgpu-defer-diags. Then as -fgpu-defer-diags become stable, we 
turn it on by default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80450

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

[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield requested changes to this revision.
JonChesterfield added a comment.
This revision now requires changes to proceed.

I don't believe the contents of this patch is necessary for codegen on amdgpu. 
One of the internal/weak distinctions works around a bug in the gfx800 
toolchain, but we should root cause and fix that bug instead. The kern_desc 
object is redundant. I think amdgpu-flat-work-group-size is already emitted, 
but if not, we might want that.

The wg_size code is interesting but architecture independent, and it's probably 
more user friendly for nvptx and amdgcn to have the same handling of wg_size 
constraints.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86097

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

[PATCH] D51650: Implement target_clones multiversioning

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.
Herald added a subscriber: jdoerfert.

In D51650#1305509 , @erichkeane wrote:

> Fix @rsmith s comments, rebase on the big CPUDispatch refactor.

Ping. What's the status here?


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

https://reviews.llvm.org/D51650

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

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D92155#2418693 , @psionic12 wrote:

> In D92155#2418617 , @sammccall wrote:
>
>> - what are the plugins you want to use with clangd? (i.e what kinds of 
>> plugins are most important, what are they useful for)
>
> Any plugins which will report diagnostics, especially customized attribute 
> plugins, you can check `clang/examples/` for details. Examples like 
> `AnnotationFunctions`, `Attribute` and `CallSuperAttribute`(which is wrote by 
> me) all defined a customized attribute and apply some rules on it, if the 
> code do not followed the rule, diagnostics are reported.

Right, I understand (a little bit, at least!) what plugins *can* do. What I'm 
asking specifically is: this feature has a cost, how important is supporting 
it? Are there codebases where these attributes are widely used, and enforcement 
at development time is particularly valuable?

>> - are plugins always threadsafe?
>
> Could you give an example about what kind of thread-safety problem you are 
> imagining? As long as I wrote clang specific plugins, only callbacks are 
> needed to implement, no thread functionalities are used, and as far as I 
> know, clang use single thread to deal with lex, parser, and sema.  So I think 
> thread safety won't be a problem here.

Sorry, I probably should have said "can plugins be thread-hostile".
Clangd runs several parsing actions in parallel, each on a single separate 
thread.
If a plugin assumes there's only a single instance of it running and uses 
static data accordingly, we're going to hit problems in clangd. 
The problem is that precisely because clang uses a single thread, (some) 
plugins may think this is a reasonable assumption. And it's not possible to 
tell "from the outside" whether a given plugin is unsafe in this way.

>> - are there any meaningful restrictions from a security perspective (e.g. 
>> can it load arbitrary .sos from disk, given control over compile args)
>
> Sorry I can't tell, but I think this patch is some sort of NFC patch, what 
> you concern will or will not exist with or without this patch.

It looks like this patch moves `LoadLibraryPermanently` to a different 
location, per the patch description clangd does not currently load plugins. 
That's why I'm concerned this patch may introduce unsafe loading of untrusted 
.sos.

>> - how robust are plugins when the main frontendaction is nonstandard (e.g. 
>> uses preambles, skips function bodies)
>
> Different plugins have different callbacks, these callbacks will only get 
> called if the compile work flow goes there, that is to say if the 
> frontendcation skip the bodies somehow(sorry I don't know how could it be 
> done, could you help to explain more?), the callback will not triggered.

SkipFunctionBodies is an option in FrontendOpts that produces a different AST 
that clang itself (which does not set this option) can't produce. Basically I'm 
saying "can we tell if a plugin is going to work with our weird ASTs, or do we 
just have to try and maybe crash".

>> - the filesystem access to the libraries isn't virtualized (vfs) and I don't 
>> know if it really can be.
>
> Sorry I don't quite understand, as long as I know the plugin is no more than 
> a shared library, and the driver is no more than a executable, the load 
> procedure seems no different with other `dlopen` cases.

We don't support any other dlopen cases in clangd.

>> - consequences of bad behavior/crash are worse, because clangd is 
>> long-running
>
> Does clangd shares the same process with an `FrontendAction`? Because in 
> clang, if a plugin carshes, the whole process does terminated, clang didn't 
> handle this yet(just a core dump).

Yes. And I'm not convinced it's possible to handle this safely without large 
architectural changes.

> Anyway, some of your concerns seems out of my scope, since clangd have never 
> worked with plugins before, do you think it's better to add some one who 
> knows these? (I just don't know who to add...)

I think we need someone who understands plugins well why they're safe (or how 
they can be made safe) to work with clangd, and who's willing to do much of the 
work to do so.
Absent that, I think we probably shouldn't enable them (beyond an experiment).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D92167: [OpenMP][NFC] Encapsulate some CGOpenMPRuntime static methods in a utility class

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield requested changes to this revision.
JonChesterfield added a comment.
This revision now requires changes to proceed.

This moves some private free functions into a CodeGenUtil class in the header 
so that the amdgpu plugin can call them in order to construct the kern_desc 
object. Said kern_desc object has the review comment "This kern_desc object 
should be deleted, not upstreamed." in the patch from which you extracted this 
change.

Instead of this, the kern_desc part of D86097  
should have been removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92167

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

[PATCH] D71124: [RISCV] support clang driver to select cpu

[Ivan Serdyuk via Phabricator via cfe-commits]

oceanfish81 added a comment.

@khchen , what is the recomended way to get the current CPU features?

I see that there are some ISA extensions defined here 
 .

So RISC-V back-end has another API, for both identifying available CPU (or 
vCPU, if under Qemu) features?
How does the CLI options correspond with the back-end capabilities?

There is some code, for gollvm project 
https://go.googlesource.com/gollvm/+/e3ede8ef8009429eaae91bbae2d67542ddaa5853/tools/capture-fcn-attributes.go
 - and it relies on 
https://github.com/llvm/llvm-project/blob/48ddf5e182c61cb93d66325f5690312d9e9226eb/llvm/lib/Support/Host.cpp
 (auto-generation).
I need to understand whethershould Host.cpp be patched or should I use an arch. 
specific/dedicated workaround.

I am forcasting sgnificant contrast in gollvm's adoption, between various 
RISC-V boards, since of an active usage of different CPU features (available on 
one micro-controller, with others unavailable - and vice versa).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71124

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

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added a comment.

In D92155#2418617 , @sammccall wrote:

> - what are the plugins you want to use with clangd? (i.e what kinds of 
> plugins are most important, what are they useful for)

Any plugins which will report diagnostics, especially customized attribute 
plugins, you can check `clang/examples/` for details. Examples like 
`AnnotationFunctions`, `Attribute` and `CallSuperAttribute`(which is wrote by 
me) all defined a customized attribute and apply some rules on it, if the code 
do not followed the rule, diagnostics are reported.

> - are plugins always threadsafe?

Could you give an example about what kind of thread-safety problem you are 
imagining? As long as I wrote clang specific plugins, only callbacks are needed 
to implement, no thread functionalities are used, and as far as I know, clang 
use single thread to deal with lex, parser, and sema.  So I think thread safety 
won't be a problem here.

> - are there any meaningful restrictions from a security perspective (e.g. can 
> it load arbitrary .sos from disk, given control over compile args)

Sorry I can't tell, but I think this patch is some sort of NFC patch, what you 
concern will or will not exist with or without this patch.

> - how robust are plugins when the main frontendaction is nonstandard (e.g. 
> uses preambles, skips function bodies)

Different plugins have different callbacks, these callbacks will only get 
called if the compile work flow goes there, that is to say if the 
frontendcation skip the bodies somehow(sorry I don't know how could it be done, 
could you help to explain more?), the callback will not triggered.

> - the filesystem access to the libraries isn't virtualized (vfs) and I don't 
> know if it really can be.

Sorry I don't quite understand, as long as I know the plugin is no more than a 
shared library, and the driver is no more than a executable, the load procedure 
seems no different with other `dlopen` cases.

> - consequences of bad behavior/crash are worse, because clangd is long-running

Does clangd shares the same process with an `FrontendAction`? Because in clang, 
if a plugin carshes, the whole process does terminated, clang didn't handle 
this yet(just a core dump).

> - if a plugin works with clang but not clangd, it's unclear whether/where 
> there's a bug

I think that depends. Since clangd use `syntax-only`, plugins other than 
`frontendAction` seems won't get called. As to `frontendActions`, if it works 
with clang, it should works with clangd, because `frontendActions` is more 
about syntax check before generate IR code, clang or clangd or chang-check 
makes no difference from an `frontendAction`s perspective.

Anyway, some of your concerns seems out of my scope, since clangd have never 
worked with plugins before, do you think it's better to add some one who knows 
these? (I just don't know who to add...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D92181: [clangd] NFC: Add client-side logging for remote index requests

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307869.
kbobyrev added a comment.

Switch Deadline type to `const auto`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92181

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp


Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -42,10 +42,13 @@
 SPAN_ATTACH(Tracer, "Request", RPCRequest.DebugString());
 grpc::ClientContext Context;
 Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
-std::chrono::system_clock::time_point Deadline =
-std::chrono::system_clock::now() + DeadlineWaitingTime;
+const std::chrono::system_clock::time_point StartTime =
+std::chrono::system_clock::now();
+const auto Deadline = StartTime + DeadlineWaitingTime;
 Context.set_deadline(Deadline);
 auto Reader = (Stub.get()->*RPCCall)(, RPCRequest);
+vlog("Sending RPC Request {0}: {1}", RequestT::descriptor()->name(),
+ RPCRequest.DebugString());
 ReplyT Reply;
 unsigned Successful = 0;
 unsigned FailedToParse = 0;
@@ -65,6 +68,11 @@
   Callback(*Response);
   ++Successful;
 }
+const auto Millis = std::chrono::duration_cast(
+std::chrono::system_clock::now() - StartTime)
+.count();
+vlog("RPC Request {0} => OK: {1} results in {2}ms",
+ RequestT::descriptor()->name(), Successful, Millis);
 SPAN_ATTACH(Tracer, "Status", Reader->Finish().ok());
 SPAN_ATTACH(Tracer, "Successful", Successful);
 SPAN_ATTACH(Tracer, "Failed to parse", FailedToParse);


Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -42,10 +42,13 @@
 SPAN_ATTACH(Tracer, "Request", RPCRequest.DebugString());
 grpc::ClientContext Context;
 Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
-std::chrono::system_clock::time_point Deadline =
-std::chrono::system_clock::now() + DeadlineWaitingTime;
+const std::chrono::system_clock::time_point StartTime =
+std::chrono::system_clock::now();
+const auto Deadline = StartTime + DeadlineWaitingTime;
 Context.set_deadline(Deadline);
 auto Reader = (Stub.get()->*RPCCall)(, RPCRequest);
+vlog("Sending RPC Request {0}: {1}", RequestT::descriptor()->name(),
+ RPCRequest.DebugString());
 ReplyT Reply;
 unsigned Successful = 0;
 unsigned FailedToParse = 0;
@@ -65,6 +68,11 @@
   Callback(*Response);
   ++Successful;
 }
+const auto Millis = std::chrono::duration_cast(
+std::chrono::system_clock::now() - StartTime)
+.count();
+vlog("RPC Request {0} => OK: {1} results in {2}ms",
+ RequestT::descriptor()->name(), Successful, Millis);
 SPAN_ATTACH(Tracer, "Status", Reader->Finish().ok());
 SPAN_ATTACH(Tracer, "Successful", Successful);
 SPAN_ATTACH(Tracer, "Failed to parse", FailedToParse);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92181: [clangd] NFC: Add client-side logging for remote index requests

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kbobyrev requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Figuring out whether the server is responding and debugging issues with remote
index setup is no easy task: add verbose logging for client side RPC requests
to relieve some pain.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92181

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp


Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -42,10 +42,14 @@
 SPAN_ATTACH(Tracer, "Request", RPCRequest.DebugString());
 grpc::ClientContext Context;
 Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
-std::chrono::system_clock::time_point Deadline =
-std::chrono::system_clock::now() + DeadlineWaitingTime;
+const std::chrono::system_clock::time_point StartTime =
+std::chrono::system_clock::now();
+const std::chrono::system_clock::time_point Deadline =
+StartTime + DeadlineWaitingTime;
 Context.set_deadline(Deadline);
 auto Reader = (Stub.get()->*RPCCall)(, RPCRequest);
+vlog("Sending RPC Request {0}: {1}", RequestT::descriptor()->name(),
+ RPCRequest.DebugString());
 ReplyT Reply;
 unsigned Successful = 0;
 unsigned FailedToParse = 0;
@@ -65,6 +69,11 @@
   Callback(*Response);
   ++Successful;
 }
+const auto Millis = std::chrono::duration_cast(
+std::chrono::system_clock::now() - StartTime)
+.count();
+vlog("RPC Request {0} => OK: {1} results in {2}ms",
+ RequestT::descriptor()->name(), Successful, Millis);
 SPAN_ATTACH(Tracer, "Status", Reader->Finish().ok());
 SPAN_ATTACH(Tracer, "Successful", Successful);
 SPAN_ATTACH(Tracer, "Failed to parse", FailedToParse);


Index: clang-tools-extra/clangd/index/remote/Client.cpp
===
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ clang-tools-extra/clangd/index/remote/Client.cpp
@@ -42,10 +42,14 @@
 SPAN_ATTACH(Tracer, "Request", RPCRequest.DebugString());
 grpc::ClientContext Context;
 Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
-std::chrono::system_clock::time_point Deadline =
-std::chrono::system_clock::now() + DeadlineWaitingTime;
+const std::chrono::system_clock::time_point StartTime =
+std::chrono::system_clock::now();
+const std::chrono::system_clock::time_point Deadline =
+StartTime + DeadlineWaitingTime;
 Context.set_deadline(Deadline);
 auto Reader = (Stub.get()->*RPCCall)(, RPCRequest);
+vlog("Sending RPC Request {0}: {1}", RequestT::descriptor()->name(),
+ RPCRequest.DebugString());
 ReplyT Reply;
 unsigned Successful = 0;
 unsigned FailedToParse = 0;
@@ -65,6 +69,11 @@
   Callback(*Response);
   ++Successful;
 }
+const auto Millis = std::chrono::duration_cast(
+std::chrono::system_clock::now() - StartTime)
+.count();
+vlog("RPC Request {0} => OK: {1} results in {2}ms",
+ RequestT::descriptor()->name(), Successful, Millis);
 SPAN_ATTACH(Tracer, "Status", Reader->Finish().ok());
 SPAN_ATTACH(Tracer, "Successful", Successful);
 SPAN_ATTACH(Tracer, "Failed to parse", FailedToParse);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91930: [clangd] Implement textDocument/codeLens

[WangWei via Phabricator via cfe-commits]

lightmelodies added a comment.

I have also considered these problems, and here is some points of my view. 
**Latency**
Consider llvm-project itself, **textDocument/codeLens** can be done in <500ms 
generally and <100ms if collect hierarchy is disabled in my notebook. Latency 
is significant in some rare case like //Type.h// which has too complex 
hierarchy relations. Meanwhile, vscode is smart to remember previous codelens's 
positions so the text rendering will not look too weird during long-time 
request. In fact, the biggest performance issue I find is that building AST 
when open a  new file costs too much time, which makes 
**textDocument/codeLens** quite slow, as well as other action such as  
**textDocument/documentSymbols**. That's why I modified //ClangdLSPServer.cpp// 
to keep AST for recent closed files.

| file| line count | cost| cost (without hierarchy) |
| ASTVector.h | 411| 4ms | 1ms  |
| APValue.h   | 693| 16 ms   | 8ms  |
| Decl.h  | 4599   | 213 ms  | 40 ms|
| Expr.h  | 6383   | 447 ms  | 61 ms|
| Type.h  | 7222   | 3765 ms | 47 ms|
|

**Batch**
That's definitely a problem with remote index since vscode can only do 
resolution per symbol. Maybe it will be better to resolve directly in  
**textDocument/codeLens** requests when deal with remote index. Just need add 
an option to allow users switch the strategy.

**Useless Lens**
Yes, we can see the base directly in class declaration. However c++ has forward 
declaration and type alias, in such case the base can not be seen directly. So 
someone may prefer to have these codelens. Typescript in vscode has options to 
allow users disable specific type of codelens. We can use similar strategy, 
e.g. **-codelens=references,base,derived** instead of **-codelens=true**.

I'm currently working on an implemention by traversing AST directly instead of 
using **textDocument/documentSymbols**, since symbols defined by macro are 
often reported a wrong range. It should save much time of **findNamedDeclAt** 
comparing to current implemention, and make it easier to add additional filters 
on symbols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91930

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

[PATCH] D91949: [clang-format] Add BreakBeforeStructInitialization configuration

[Anastasiia Lukianenko via Phabricator via cfe-commits]

anastasiia_lukianenko added a comment.

Thanks for the detailed review. After more detailed testing, I will upload a 
new version of the patches, which will fix configuration influence on other 
formatting options.




Comment at: clang/lib/Format/TokenAnnotator.cpp:3492
   const FormatToken  = *Right.Previous;
+   if (Style.BreakBeforeStructInitialization && Right.is(tok::l_brace) &&
+   (Right.is(BK_BracedInit) || Left.is(tok::equal)))

MyDeveloperDay wrote:
> This feels quite general and as its high up in this function I'm thinking its 
> going to get hit alot...
> 
> To me it feels like any construct like this could potentially fall into this 
> trap correct
> 
> ` = { `
> 
> what would happen to
> 
> `std::vector v = {2, 1};`
> 
> did the FormatTests run cleanly, (assuming they did wow!)
Thank you for paying attention to this problem. I tested these configurations 
mostly on C code and now I see that it has undefined behavior in other 
languages... I think this condition must have more specific parameters that 
will format the code only for structures.



Comment at: clang/unittests/Format/FormatTest.cpp:5052
+  Style.ColumnLimit = 80;
+  Style.BreakBeforeStructInitialization = true;
+  verifyFormat("struct new_struct struct_name = \n"

MyDeveloperDay wrote:
> This is never initialized in the main code? so what value will it have by 
> default?
I'll improve tests and upload new version. 


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

https://reviews.llvm.org/D91949

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

[PATCH] D92175: [clang-tidy] Add support for loading configs for specific files.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I get the motivation for wanting to have a good, accessible way to do this 
thing that is possible today, but at the same time... I'm not sure we should 
pave this cow path, or even keep it.

Clang-tidy's configuration system is *really* complicated, and I don't think 
the complexity justified by the value it brings.
Currently AFAICS there's just *one* check that ever reads the config for other 
files, and it could be redesigned not to do so - by conservatively renaming 
only identifiers declared in the current file.

As you're aware Nathan but others aren't - this ability to request config for 
other files was a significant source of difficulty in D91029 
 - i.e. complexity of embedding clang-tidy.
Other features like inheritance, local vs global names, check option value 
priority, ConfigFileHandlers etc have contributed to a really complex model and 
greatly hinder the prospect of replacing ClangTidyOptionsProvider with 
something simpler, which is sorely needed IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92175

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

[PATCH] D90871: [Sema] Fold VLAs to constant arrays in a few more contexts

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ping!


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

https://reviews.llvm.org/D90871

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

[PATCH] D92179: [clang-tidy] Catch more unwanted implicit conversions in performance-implicit-conversion-in-loop

[Clement Courbet via Phabricator via cfe-commits]

courbet marked 2 inline comments as done.
courbet added a comment.

Thanks, comments addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92179

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

[PATCH] D92179: [clang-tidy] Catch more unwanted implicit conversions in performance-implicit-conversion-in-loop

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 307863.
courbet added a comment.

Add to release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92179

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp
  clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
@@ -14,7 +14,7 @@
   sources = [
 "FasterStringFindCheck.cpp",
 "ForRangeCopyCheck.cpp",
-"ImplicitConversionInLoopCheck.cpp",
+"ImplicitConversionCheck.cpp",
 "InefficientAlgorithmCheck.cpp",
 "InefficientStringConcatenationCheck.cpp",
 "InefficientVectorOperationCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-implicit-conversion-in-loop %t
+// RUN: %check_clang_tidy %s performance-implicit-conversion %t
 
 // -- Classes used in the tests --
 
@@ -98,7 +98,7 @@
 
 void ImplicitSimpleClassIterator() {
   for (const ImplicitWrapper& foo : SimpleView()) {}
-  // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the loop variable 'foo' is different from the one returned by the iterator and generates an implicit conversion; you can either change the type to the matching one ('const SimpleClass &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [performance-implicit-conversion-in-loop]
+  // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the variable 'foo' is different from the one on the right hand side: this generates an implicit conversion; you can either change the type to the matching one ('const SimpleClass &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [performance-implicit-conversion]
   // for (ImplicitWrapper& foo : SimpleView()) {}
   for (const ImplicitWrapper foo : SimpleView()) {}
   for (ImplicitWrapper foo : SimpleView()) {}
@@ -195,3 +195,9 @@
   for (const OperatorWrapper foo : array) {}
   for (OperatorWrapper foo : array) {}
 }
+
+void NonLoopVariable() {
+  ComplexClass array[5];
+  const OperatorWrapper  = *array;
+  // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
@@ -0,0 +1,32 @@
+.. title:: clang-tidy - performance-implicit-conversion
+
+performance-implicit-conversion
+===
+
+This warning appears a when a variable of const ref type binds to a temporary
+that has a different type than the right hand side of the initialization.
+In that case, an implicit conversion happens, which can for example
+result in expensive deep copies.
+
+Example (loop variable):
+
+.. code-block:: c++
+
+  map> my_map;
+  for (const pair>& p : my_map) {}
+
+Example (block scope variable):
+
+.. code-block:: c++
+
+  map> my_map;
+  const pair>& p = *my_map.begin();
+
+In both cases, the iterator type is in fact ``pair>``,
+which means that the compiler added a conversion, resulting in a copy of the
+vectors.
+
+The easiest solution is usually to use ``const auto&`` instead of writing the
+type manually.
+If the conversion if desired, avoid using lifetime extension to make it clear
+that the intent is to create a new object (i.e., remove the ``&``).
Index: 

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I don't have a lot of background in clang plugins, either on the technical side 
or how they're used in practice, so some of this may be a bit underinformed.
This sounds OK for experiments as long as it's off by default, but there may be 
barriers to going any further.

Some questions to try to understand how this is going to play with clangd:

- what are the plugins you want to use with clangd? (i.e what kinds of plugins 
are most important, what are they useful for)
- are plugins always threadsafe?
- are there any meaningful restrictions from a security perspective (e.g. can 
it load arbitrary .sos from disk, given control over compile args)
- how robust are plugins when the main frontendaction is nonstandard (e.g. uses 
preambles, skips function bodies)

Other points that i'm fairly sure will be problematic:

- the filesystem access to the libraries isn't virtualized (vfs) and I don't 
know if it really can be.
- consequences of bad behavior/crash are worse, because clangd is long-running
- if a plugin works with clang but not clangd, it's unclear whether/where 
there's a bug




Comment at: clang-tools-extra/clangd/tool/CMakeLists.txt:44
+# Support plugins.
+if(CLANG_PLUGIN_SUPPORT)
+  export_executable_symbols_for_plugins(clangd)

This looks like it's on by default - it should be off at least by default until:
 - support surface of this is understood by maintainers
 - security is understood and safe (command-line arguments are effectively 
attacker-controlled for clangd)
 - we've verified that this actually works well with important plugins



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3902
 
+  // Load any requested plugins.
+  for (const std::string  : Res.getFrontendOpts().Plugins) {

how does this code behave if CLANG_PLUGIN_SUPPORT is off?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3905
+std::string Error;
+if (llvm::sys::DynamicLibrary::LoadLibraryPermanently(Path.c_str(), 
))
+  Diags.Report(diag::err_fe_unable_to_load_plugin) << Path << Error;

is this threadsafe on all platforms?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3914
+if (P->getActionType() == PluginASTAction::ReplaceAction) {
+  Res.getFrontendOpts().ProgramAction = clang::frontend::PluginAction;
+  Res.getFrontendOpts().ActionName = Plugin.getName().str();

we can't support replacement of the main action in clangd. What's the plan 
there - ignore the plugin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155

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

[PATCH] D92179: [clang-tidy] Catch more unwanted implicit conversions in performance-implicit-conversion-in-loop

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention renaming in Release Notes.




Comment at: 
clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp:1
-//===--- ImplicitConversionInLoopCheck.cpp - 
clang-tidy===//
+//===--- ImplicitConversionCheck.cpp - 
clang-tidy--===//
 //

Please use space after clang-tidy.



Comment at: clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h:1
-//===--- ImplicitConversionInLoopCheck.h - clang-tidy*- C++ 
-*-===//
+//===--- ImplicitConversionCheck.h - clang-tidy*- C++ -*-===//
 //

Please fix length and use space after clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92179

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

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

[Raphael Isemann via Phabricator via cfe-commits]

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

I think the test could be made a bit stricter (see inline comment), but 
otherwise this LGTM. Thanks!




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5900
+  ASSERT_TRUE(ToD);
+  EXPECT_EQ(FromD->isCopyDeductionCandidate(), 
ToD->isCopyDeductionCandidate());
+  // Check that the deduced class template is also imported.

Not sure if that's actually testing what it should. `FromD` is not the copy 
deduction candidate, so this is just comparing that both Decls have the default 
value `false`?

If we pick the copy deduction candidate in this test then we could test that we 
actually copy the value over and not just have the default 'false' flag for 
`isCopyDeductionCandidate` (something like 
`cxxDeductionGuideDecl(hasParameter(0, hasType(asString("A"` should find 
the copy deduction candidate).

Then we could also simplify this to 
`EXPECT_TRUE(ToD->isCopyDeductionCandidate());`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92109

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

[PATCH] D92179: [clang-tidy] Catch more unwanted implicit conversions in performance-implicit-conversion-in-loop

[Clement Courbet via Phabricator via cfe-commits]

courbet created this revision.
courbet added a reviewer: pilki.
Herald added subscribers: llvm-commits, xazax.hun, mgorny.
Herald added projects: clang, LLVM.
courbet requested review of this revision.

It turns out that there is no reason to restrict to loop variables. This
allows catching stuff like:

  const std::pair& kv = *map.begin();

(did you notice the missing `const uint32_t`) ?

On our codebase, this roughly doubles the number of warnings compared to
the loop-only case.

The warnings mainly fall in two categories:

1. Lifetime extended temporary:

  const std::function<...>& c = [](...) {};

Which is better written as one of:

  const std::function<...> c = [](...) {};  // explicit
  const auto c = [](...) {};  // no std::function



2. Copy + lifetime extension.

  const std::pair& kv = *map.begin();

Better written as:

  // no copy:
  const std::pair& kv = *map.begin();
  const auto& kv = *map.begin();
  // explicit
  const std::pair kv = *map.begin();
  // explicit, allows automatic move.
  std::pair kv = *map.begin();

And rename the check.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92179

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp
  clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
  
clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
  
clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
@@ -14,7 +14,7 @@
   sources = [
 "FasterStringFindCheck.cpp",
 "ForRangeCopyCheck.cpp",
-"ImplicitConversionInLoopCheck.cpp",
+"ImplicitConversionCheck.cpp",
 "InefficientAlgorithmCheck.cpp",
 "InefficientStringConcatenationCheck.cpp",
 "InefficientVectorOperationCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-implicit-conversion-in-loop %t
+// RUN: %check_clang_tidy %s performance-implicit-conversion %t
 
 // -- Classes used in the tests --
 
@@ -98,7 +98,7 @@
 
 void ImplicitSimpleClassIterator() {
   for (const ImplicitWrapper& foo : SimpleView()) {}
-  // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the loop variable 'foo' is different from the one returned by the iterator and generates an implicit conversion; you can either change the type to the matching one ('const SimpleClass &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [performance-implicit-conversion-in-loop]
+  // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the variable 'foo' is different from the one on the right hand side: this generates an implicit conversion; you can either change the type to the matching one ('const SimpleClass &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [performance-implicit-conversion]
   // for (ImplicitWrapper& foo : SimpleView()) {}
   for (const ImplicitWrapper foo : SimpleView()) {}
   for (ImplicitWrapper foo : SimpleView()) {}
@@ -195,3 +195,9 @@
   for (const OperatorWrapper foo : array) {}
   for (OperatorWrapper foo : array) {}
 }
+
+void NonLoopVariable() {
+  ComplexClass array[5];
+  const OperatorWrapper  = *array;
+  // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
@@ -0,0 +1,32 @@
+.. title:: clang-tidy - performance-implicit-conversion
+
+performance-implicit-conversion
+===
+
+This warning 

[PATCH] D43159: [libc++] Replace several uses of 0 by nullptr

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: libcxx/include/functional:1759
 typedef __base<_Rp(_ArgTypes...)> __func;
 __func* __f_;
 

All uses of `__f_` should also use `nullptr`. If my search counted correctly, 
there are 15 of them in `__value_func`.



Comment at: libcxx/include/locale:4335
 {
 wbuffer_convert* __rt = 0;
 if (__cv_ != 0 && __bufptr_ != 0)

You missed this one.



Comment at: libcxx/include/locale:4340
 if ((__cm_ & ios_base::out) && sync())
 __rt = 0;
 }

And here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43159

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

[PATCH] D83698: [clang][cli] Port Target option flags to new option parsing system

[Jan Svoboda 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 rGa3258566868b: [clang][cli] Port Target option flags to new 
option parsing system (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83698

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3753,9 +3753,6 @@
   Opts.LinkerVersion =
   std::string(Args.getLastArgValue(OPT_target_linker_version));
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
-  Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
-  Opts.NVPTXUseShortPointers = Args.hasFlag(
-  options::OPT_fcuda_short_ptr, options::OPT_fno_cuda_short_ptr, false);
   Opts.AllowAMDGPUUnsafeFPAtomics =
   Args.hasFlag(options::OPT_munsafe_fp_atomics,
options::OPT_mno_unsafe_fp_atomics, false);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -693,7 +693,8 @@
 def : Flag<["-"], "fcuda-rdc">, Alias;
 def : Flag<["-"], "fno-cuda-rdc">, Alias;
 defm cuda_short_ptr : OptInFFlag<"cuda-short-ptr",
-  "Use 32-bit pointers for accessing const/local/shared address spaces">;
+  "Use 32-bit pointers for accessing const/local/shared address spaces", "", 
"",
+  [], "TargetOpts->NVPTXUseShortPointers">;
 def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group,
   HelpText<"ROCm installation path, used for finding and automatically linking 
required bitcode libraries.">;
 def rocm_device_lib_path_EQ : Joined<["--"], "rocm-device-lib-path=">, 
Group,
@@ -1049,7 +1050,7 @@
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group;
 defm jump_tables : OptOutFFlag<"jump-tables", "Use", "Do not use", " jump 
tables for lowering switches">;
-defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", 
"Disable", " support for int128_t type">;
+defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", 
"Disable", " support for int128_t type", [], "TargetOpts->ForceEnableInt128">;
 defm keep_static_consts : OptInFFlag<"keep-static-consts", "Keep", "Don't 
keep", " static const variables if unused", [NoXarchOption]>;
 defm fixed_point : OptInFFlag<"fixed-point", "Enable", "Disable", " fixed 
point types">;
 defm cxx_static_destructors : OptOutFFlag<"c++-static-destructors", "",


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3753,9 +3753,6 @@
   Opts.LinkerVersion =
   std::string(Args.getLastArgValue(OPT_target_linker_version));
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
-  Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
-  Opts.NVPTXUseShortPointers = Args.hasFlag(
-  options::OPT_fcuda_short_ptr, options::OPT_fno_cuda_short_ptr, false);
   Opts.AllowAMDGPUUnsafeFPAtomics =
   Args.hasFlag(options::OPT_munsafe_fp_atomics,
options::OPT_mno_unsafe_fp_atomics, false);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -693,7 +693,8 @@
 def : Flag<["-"], "fcuda-rdc">, Alias;
 def : Flag<["-"], "fno-cuda-rdc">, Alias;
 defm cuda_short_ptr : OptInFFlag<"cuda-short-ptr",
-  "Use 32-bit pointers for accessing const/local/shared address spaces">;
+  "Use 32-bit pointers for accessing const/local/shared address spaces", "", "",
+  [], "TargetOpts->NVPTXUseShortPointers">;
 def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group,
   HelpText<"ROCm installation path, used for finding and automatically linking required bitcode libraries.">;
 def rocm_device_lib_path_EQ : Joined<["--"], "rocm-device-lib-path=">, Group,
@@ -1049,7 +1050,7 @@
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group;
 defm jump_tables : OptOutFFlag<"jump-tables", "Use", "Do not use", " jump tables for lowering switches">;
-defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", "Disable", " support for int128_t type">;
+defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", "Disable", " support for int128_t type", [], "TargetOpts->ForceEnableInt128">;
 defm keep_static_consts : OptInFFlag<"keep-static-consts", 

[clang] a325856 - [clang][cli] Port Target option flags to new option parsing system

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2020-11-26T15:32:38+01:00
New Revision: a3258566868b3a16b131e8963932ac21888cb90b

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

LOG: [clang][cli] Port Target option flags to new option parsing system

Depends on D83697

Original patch by Daniel Grumberg.

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

Added: 


Modified: 
clang/include/clang/Driver/Options.td
clang/lib/Frontend/CompilerInvocation.cpp

Removed: 




diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 88af70116304..0014ced5dca7 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -693,7 +693,8 @@ defm gpu_rdc : OptInFFlag<"gpu-rdc",
 def : Flag<["-"], "fcuda-rdc">, Alias;
 def : Flag<["-"], "fno-cuda-rdc">, Alias;
 defm cuda_short_ptr : OptInFFlag<"cuda-short-ptr",
-  "Use 32-bit pointers for accessing const/local/shared address spaces">;
+  "Use 32-bit pointers for accessing const/local/shared address spaces", "", 
"",
+  [], "TargetOpts->NVPTXUseShortPointers">;
 def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group,
   HelpText<"ROCm installation path, used for finding and automatically linking 
required bitcode libraries.">;
 def rocm_device_lib_path_EQ : Joined<["--"], "rocm-device-lib-path=">, 
Group,
@@ -1049,7 +1050,7 @@ def fbracket_depth_EQ : Joined<["-"], "fbracket-depth=">, 
Group, Flags<
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group;
 defm jump_tables : OptOutFFlag<"jump-tables", "Use", "Do not use", " jump 
tables for lowering switches">;
-defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", 
"Disable", " support for int128_t type">;
+defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", 
"Disable", " support for int128_t type", [], "TargetOpts->ForceEnableInt128">;
 defm keep_static_consts : OptInFFlag<"keep-static-consts", "Keep", "Don't 
keep", " static const variables if unused", [NoXarchOption]>;
 defm fixed_point : OptInFFlag<"fixed-point", "Enable", "Disable", " fixed 
point types">;
 defm cxx_static_destructors : OptOutFFlag<"c++-static-destructors", "",

diff  --git a/clang/lib/Frontend/CompilerInvocation.cpp 
b/clang/lib/Frontend/CompilerInvocation.cpp
index 144bbe84abeb..fb79dea06320 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3753,9 +3753,6 @@ static void ParseTargetArgs(TargetOptions , ArgList 
,
   Opts.LinkerVersion =
   std::string(Args.getLastArgValue(OPT_target_linker_version));
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
-  Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
-  Opts.NVPTXUseShortPointers = Args.hasFlag(
-  options::OPT_fcuda_short_ptr, options::OPT_fno_cuda_short_ptr, false);
   Opts.AllowAMDGPUUnsafeFPAtomics =
   Args.hasFlag(options::OPT_munsafe_fp_atomics,
options::OPT_mno_unsafe_fp_atomics, false);



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

[PATCH] D83698: [clang][cli] Port Target option flags to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 307855.
jansvoboda11 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83698

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3753,9 +3753,6 @@
   Opts.LinkerVersion =
   std::string(Args.getLastArgValue(OPT_target_linker_version));
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
-  Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
-  Opts.NVPTXUseShortPointers = Args.hasFlag(
-  options::OPT_fcuda_short_ptr, options::OPT_fno_cuda_short_ptr, false);
   Opts.AllowAMDGPUUnsafeFPAtomics =
   Args.hasFlag(options::OPT_munsafe_fp_atomics,
options::OPT_mno_unsafe_fp_atomics, false);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -693,7 +693,8 @@
 def : Flag<["-"], "fcuda-rdc">, Alias;
 def : Flag<["-"], "fno-cuda-rdc">, Alias;
 defm cuda_short_ptr : OptInFFlag<"cuda-short-ptr",
-  "Use 32-bit pointers for accessing const/local/shared address spaces">;
+  "Use 32-bit pointers for accessing const/local/shared address spaces", "", 
"",
+  [], "TargetOpts->NVPTXUseShortPointers">;
 def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group,
   HelpText<"ROCm installation path, used for finding and automatically linking 
required bitcode libraries.">;
 def rocm_device_lib_path_EQ : Joined<["--"], "rocm-device-lib-path=">, 
Group,
@@ -1049,7 +1050,7 @@
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group;
 defm jump_tables : OptOutFFlag<"jump-tables", "Use", "Do not use", " jump 
tables for lowering switches">;
-defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", 
"Disable", " support for int128_t type">;
+defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", 
"Disable", " support for int128_t type", [], "TargetOpts->ForceEnableInt128">;
 defm keep_static_consts : OptInFFlag<"keep-static-consts", "Keep", "Don't 
keep", " static const variables if unused", [NoXarchOption]>;
 defm fixed_point : OptInFFlag<"fixed-point", "Enable", "Disable", " fixed 
point types">;
 defm cxx_static_destructors : OptOutFFlag<"c++-static-destructors", "",


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3753,9 +3753,6 @@
   Opts.LinkerVersion =
   std::string(Args.getLastArgValue(OPT_target_linker_version));
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
-  Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
-  Opts.NVPTXUseShortPointers = Args.hasFlag(
-  options::OPT_fcuda_short_ptr, options::OPT_fno_cuda_short_ptr, false);
   Opts.AllowAMDGPUUnsafeFPAtomics =
   Args.hasFlag(options::OPT_munsafe_fp_atomics,
options::OPT_mno_unsafe_fp_atomics, false);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -693,7 +693,8 @@
 def : Flag<["-"], "fcuda-rdc">, Alias;
 def : Flag<["-"], "fno-cuda-rdc">, Alias;
 defm cuda_short_ptr : OptInFFlag<"cuda-short-ptr",
-  "Use 32-bit pointers for accessing const/local/shared address spaces">;
+  "Use 32-bit pointers for accessing const/local/shared address spaces", "", "",
+  [], "TargetOpts->NVPTXUseShortPointers">;
 def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group,
   HelpText<"ROCm installation path, used for finding and automatically linking required bitcode libraries.">;
 def rocm_device_lib_path_EQ : Joined<["--"], "rocm-device-lib-path=">, Group,
@@ -1049,7 +1050,7 @@
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group;
 defm jump_tables : OptOutFFlag<"jump-tables", "Use", "Do not use", " jump tables for lowering switches">;
-defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", "Disable", " support for int128_t type">;
+defm force_enable_int128 : OptInFFlag<"force-enable-int128", "Enable", "Disable", " support for int128_t type", [], "TargetOpts->ForceEnableInt128">;
 defm keep_static_consts : OptInFFlag<"keep-static-consts", "Keep", "Don't keep", " static const variables if unused", [NoXarchOption]>;
 defm fixed_point : OptInFFlag<"fixed-point", "Enable", "Disable", " fixed point types">;
 defm 

[PATCH] D92136: [clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows

[Sylvain Audi via Phabricator via cfe-commits]

saudi updated this revision to Diff 307854.
saudi added a comment.

Update patch : clang-format fixes.

There remains some clang-tidy warnings in `Path.cpp` and `Path.h` from 
`llvm/Support`.
I would like extra opinions on this, because fixing them would break the 
homogeneity with the rest of the file, which exposes snake_case - like function 
names.
More code from `llvm/Support` have that clang-tidy mismatch, but adding Support 
to the clang-tidy exclusion seems a bit extreme...


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

https://reviews.llvm.org/D92136

Files:
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/reloc-windows.c
  clang/test/PCH/reloc.c
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1474,6 +1474,21 @@
   EXPECT_EQ("c", Path2);
 }
 
+TEST(Support, StartsWith) {
+  EXPECT_FALSE(starts_with("/foo/bar", "/bar", path::Style::posix));
+  EXPECT_FALSE(starts_with("/foo/bar", "/bar", path::Style::windows));
+  EXPECT_TRUE(starts_with("/foo/bar", "/fo", path::Style::posix));
+  EXPECT_TRUE(starts_with("/foo/bar", "/fo", path::Style::windows));
+  EXPECT_FALSE(starts_with("/foo/bar", "/Fo", path::Style::posix));
+  EXPECT_TRUE(starts_with("/foo/bar", "/Fo", path::Style::windows));
+  EXPECT_TRUE(starts_with("/foo/bar", "/foo", path::Style::posix));
+  EXPECT_TRUE(starts_with("/foo/bar", "/foo", path::Style::windows));
+  EXPECT_TRUE(starts_with("/foo/bar", "/foo/", path::Style::posix));
+  EXPECT_TRUE(starts_with("/foo/bar", "/foo/", path::Style::windows));
+  EXPECT_FALSE(starts_with("A:\\Foo\\Bar", "a:\\foo/bar", path::Style::posix));
+  EXPECT_TRUE(starts_with("A:\\Foo\\Bar", "a:\\foo/bar", path::Style::windows));
+}
+
 TEST(Support, ReplacePathPrefix) {
   SmallString<64> Path1("/foo");
   SmallString<64> Path2("/old/foo");
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -494,8 +494,7 @@
   path.append(ext.begin(), ext.end());
 }
 
-static bool starts_with(StringRef Path, StringRef Prefix,
-Style style = Style::native) {
+bool starts_with(StringRef Path, StringRef Prefix, Style style) {
   // Windows prefix matching : case and separator insensitive
   if (real_style(style) == Style::windows) {
 if (Path.size() < Prefix.size())
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -148,6 +148,28 @@
 void replace_extension(SmallVectorImpl , const Twine ,
Style style = Style::native);
 
+/// Test for matching path with a prefix.
+///
+/// @code
+///   /foo, /old, /new => /foo
+///   /old, /old, /new => /new
+///   /old, /old/, /new => /old
+///   /old/foo, /old, /new => /new/foo
+///   /old/foo, /old/, /new => /new/foo
+///   /old/foo, /old/, /new/ => /new/foo
+///   /oldfoo, /old, /new => /oldfoo
+///   /foo, , /new => /new/foo
+///   /foo, , new => new/foo
+///   /old/foo, /old,  => /foo
+/// @endcode
+///
+/// @param Path The path to check for the prefix
+/// @param Prefix The path prefix to match in \a Path.
+/// @param style The style used to match the prefix. Exact match using
+/// Posix style, case/separator insensitive match for Windows style.
+/// @result true if \a Path begins with Prefix, false otherwise
+bool starts_with(StringRef Path, StringRef Prefix, Style style = Style::native);
+
 /// Replace matching path prefix with another path.
 ///
 /// @code
Index: clang/test/PCH/reloc.c
===
--- clang/test/PCH/reloc.c
+++ clang/test/PCH/reloc.c
@@ -1,8 +1,18 @@
-// RUN: %clang -target x86_64-apple-darwin10 --relocatable-pch -o %t \
-// RUN:   -isysroot %S/Inputs/libroot %S/Inputs/libroot/usr/include/reloc.h
-// RUN: %clang -target x86_64-apple-darwin10 -fsyntax-only \
-// RUN:   -include-pch %t -isysroot %S/Inputs/libroot %s -Xclang -verify
-// RUN: not %clang -target x86_64-apple-darwin10 -include-pch %t %s
+// RUN: rm -rf %t.dir
+// RUN: mkdir -p %t.dir/Inputs
+// RUN: cp -R %S/Inputs/libroot %t.dir/Inputs/libroot
+// RUN: sed -e "s|TMPDIR|%/t.dir|g" %s > %t.dir/reloc.c
+
+// For this test, we rename the -isysroot folder between the pch file creation and its use.
+// We create the pch using a relative path to avoid introducing absolute file paths in it.
+// Renaming the folder leaves the .h files mtime unchanged, so it doesn't break the pch validation upon use.
+
+// RUN: cd %t.dir && %clang -target x86_64-apple-darwin10 --relocatable-pch -o reloc.pch \
+// RUN:   -isysroot %t.dir/Inputs/libroot 

[PATCH] D92176: Don't use sysroot/include when sysroot is empty.

[Hafiz Abid Qadeer via Phabricator via cfe-commits]

abidh created this revision.
abidh added reviewers: jroelofs, manojgupta, cjdb.
abidh added a project: clang.
Herald added a subscriber: cfe-commits.
abidh requested review of this revision.

Baremetal toolchain add Driver.SysRoot/include to the system include
paths without checking if Driver.SysRoot is empty. This resulted in
"-internal-isystem" "include" in the command. This patch adds check for
empty sysroot.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92176

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/test/Driver/baremetal.cpp


Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -93,6 +93,10 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-RTLIB-GCC
 // CHECK-RTLIB-GCC: -lgcc
 
+// RUN: %clang -### -target arm-none-eabi -v %s 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
+// CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -107,8 +107,10 @@
 
   if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) {
 SmallString<128> Dir(getDriver().SysRoot);
-llvm::sys::path::append(Dir, "include");
-addSystemInclude(DriverArgs, CC1Args, Dir.str());
+if (!Dir.empty()) {
+  llvm::sys::path::append(Dir, "include");
+  addSystemInclude(DriverArgs, CC1Args, Dir.str());
+}
   }
 }
 


Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -93,6 +93,10 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-RTLIB-GCC
 // CHECK-RTLIB-GCC: -lgcc
 
+// RUN: %clang -### -target arm-none-eabi -v %s 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=CHECK-SYSROOT-INC
+// CHECK-SYSROOT-INC-NOT: "-internal-isystem" "include"
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target riscv64-unknown-elf \
 // RUN: -L some/directory/user/asked/for \
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -107,8 +107,10 @@
 
   if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) {
 SmallString<128> Dir(getDriver().SysRoot);
-llvm::sys::path::append(Dir, "include");
-addSystemInclude(DriverArgs, CC1Args, Dir.str());
+if (!Dir.empty()) {
+  llvm::sys::path::append(Dir, "include");
+  addSystemInclude(DriverArgs, CC1Args, Dir.str());
+}
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92157: [clangd] Add language metrics for recovery AST usage.

[Haojian Wu via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/Selection.cpp:40
 
+std::string getLanguage(const clang::LangOptions ) {
+  if (Lang.C99 || Lang.C11 || Lang.C17 || Lang.C2x)

njames93 wrote:
> Nit: any reason to use std::string here instead StringRef. 
changed to `const char *`



Comment at: clang-tools-extra/clangd/Selection.cpp:51
 // Measure the fraction of selections that were enabled by recovery AST.
-void recordMetrics(const SelectionTree ) {
+void recordMetrics(const SelectionTree , const ASTContext ) {
   static constexpr trace::Metric SelectionUsedRecovery(

kadircet wrote:
> nit: it might be nice to bail out early if no tracer is attached with 
> `trace::enabled`
good catch, thanks! (I didn't know there is such a thing).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92157

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

[PATCH] D43159: [libc++] Replace several uses of 0 by nullptr

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: libcxx/include/memory:4371
 bool expired() const _NOEXCEPT
-{return __cntrl_ == 0 || __cntrl_->use_count() == 0;}
+{return __cntrl_ == nullptr || __cntrl_->use_count() == nullptr;}
 shared_ptr<_Tp> lock() const _NOEXCEPT;

Unneeded chang for `use_count()`. It's a `long`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43159

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

[PATCH] D92157: [clangd] Add language metrics for recovery AST usage.

[Haojian Wu via Phabricator via cfe-commits]

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

address comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92157

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -18,6 +18,7 @@
 namespace clang {
 namespace clangd {
 namespace {
+using ::testing::ElementsAreArray;
 using ::testing::UnorderedElementsAreArray;
 
 // Create a selection tree corresponding to a point or pair of points.
@@ -469,7 +470,7 @@
   EXPECT_TRUE(verifyCommonAncestor(T.root(), T.commonAncestor(), C.Code))
   << C.Code;
   EXPECT_THAT(Tracer.takeMetric("selection_recovery"),
-  testing::ElementsAreArray({0}));
+  ElementsAreArray({0}));
 }
   }
 }
@@ -494,10 +495,11 @@
   auto AST = TestTU::withCode(Annotations(Code).code()).build();
   trace::TestTracer Tracer;
   auto T = makeSelectionTree(Code, AST);
-  EXPECT_THAT(Tracer.takeMetric("selection_recovery"),
-  testing::ElementsAreArray({1}));
+  EXPECT_THAT(Tracer.takeMetric("selection_recovery"), ElementsAreArray({1}));
   EXPECT_THAT(Tracer.takeMetric("selection_recovery_type"),
-  testing::ElementsAreArray({1}));
+  ElementsAreArray({1}));
+  EXPECT_THAT(Tracer.takeMetric("selection_recovery_available", "C++"),
+  ElementsAreArray({1}));
 }
 
 // FIXME: Doesn't select the binary operator node in
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -37,17 +37,32 @@
 using Node = SelectionTree::Node;
 using ast_type_traits::DynTypedNode;
 
+const char *getLanguage(const clang::LangOptions ) {
+  if (Lang.C99 || Lang.C11 || Lang.C17 || Lang.C2x)
+return "C";
+  if (Lang.ObjC)
+return "ObjC";
+  if (Lang.CPlusPlus)
+return "C++";
+  return "Others"; // others we don't care.
+}
+
 // Measure the fraction of selections that were enabled by recovery AST.
-void recordMetrics(const SelectionTree ) {
+void recordMetrics(const SelectionTree , const ASTContext ) {
+  if (!trace::enabled())
+return;
   static constexpr trace::Metric SelectionUsedRecovery(
   "selection_recovery", trace::Metric::Distribution);
   static constexpr trace::Metric RecoveryType("selection_recovery_type",
   trace::Metric::Distribution);
+  static constexpr trace::Metric RecoveryAvailable(
+  "selection_recovery_available", trace::Metric::Counter, "languages");
   const auto *Common = S.commonAncestor();
   for (const auto *N = Common; N; N = N->Parent) {
 if (const auto *RE = N->ASTNode.get()) {
   SelectionUsedRecovery.record(1); // used recovery ast.
   RecoveryType.record(RE->isTypeDependent() ? 0 : 1);
+  RecoveryAvailable.record(1, getLanguage(AST.getLangOpts()));
   return;
 }
   }
@@ -834,7 +849,7 @@
.printToString(SM));
   Nodes = SelectionVisitor::collect(AST, Tokens, PrintPolicy, Begin, End, FID);
   Root = Nodes.empty() ? nullptr : ();
-  recordMetrics(*this);
+  recordMetrics(*this, AST);
   dlog("Built selection tree\n{0}", *this);
 }
 


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -18,6 +18,7 @@
 namespace clang {
 namespace clangd {
 namespace {
+using ::testing::ElementsAreArray;
 using ::testing::UnorderedElementsAreArray;
 
 // Create a selection tree corresponding to a point or pair of points.
@@ -469,7 +470,7 @@
   EXPECT_TRUE(verifyCommonAncestor(T.root(), T.commonAncestor(), C.Code))
   << C.Code;
   EXPECT_THAT(Tracer.takeMetric("selection_recovery"),
-  testing::ElementsAreArray({0}));
+  ElementsAreArray({0}));
 }
   }
 }
@@ -494,10 +495,11 @@
   auto AST = TestTU::withCode(Annotations(Code).code()).build();
   trace::TestTracer Tracer;
   auto T = makeSelectionTree(Code, AST);
-  EXPECT_THAT(Tracer.takeMetric("selection_recovery"),
-  testing::ElementsAreArray({1}));
+  EXPECT_THAT(Tracer.takeMetric("selection_recovery"), ElementsAreArray({1}));
   EXPECT_THAT(Tracer.takeMetric("selection_recovery_type"),
-  testing::ElementsAreArray({1}));
+  ElementsAreArray({1}));
+  EXPECT_THAT(Tracer.takeMetric("selection_recovery_available", "C++"),
+  ElementsAreArray({1}));
 }
 
 // FIXME: Doesn't select the binary operator node in
Index: 

[PATCH] D92119: [ASTImporter] Import the default argument of TemplateTemplateParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG39a5dd164ca8: [ASTImporter] Import the default argument of 
TemplateTemplateParmDecl (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92119

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -901,6 +901,39 @@
   .isValid());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   TemplateTemplateParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template typename TT> struct Y 
{};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template struct X {};
+   template typename TT = X> struct 
Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  const TemplateArgument  = 
To->getDefaultArgument().getArgument();
+  ASSERT_TRUE(To->isTemplateDecl());
+  TemplateDecl *ToTemplate = ToDefaultArg.getAsTemplate().getAsTemplateDecl();
+
+  // Find the default argument template 'X' in the AST and compare it against
+  // the default argument we got.
+  auto ToExpectedDecl = FirstDeclMatcher().match(
+  To->getTranslationUnitDecl(), classTemplateDecl(hasName("X")));
+  ASSERT_EQ(ToTemplate, ToExpectedDecl);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX03);
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5250,15 +5250,22 @@
   if (!TemplateParamsOrErr)
 return TemplateParamsOrErr.takeError();
 
-  // FIXME: Import default argument.
-
   TemplateTemplateParmDecl *ToD = nullptr;
-  (void)GetImportedOrCreateDecl(
-  ToD, D, Importer.getToContext(),
-  Importer.getToContext().getTranslationUnitDecl(), *LocationOrErr,
-  D->getDepth(), D->getPosition(), D->isParameterPack(),
-  (*NameOrErr).getAsIdentifierInfo(),
-  *TemplateParamsOrErr);
+  if (GetImportedOrCreateDecl(
+  ToD, D, Importer.getToContext(),
+  Importer.getToContext().getTranslationUnitDecl(), *LocationOrErr,
+  D->getDepth(), D->getPosition(), D->isParameterPack(),
+  (*NameOrErr).getAsIdentifierInfo(), *TemplateParamsOrErr))
+return ToD;
+
+  if (D->hasDefaultArgument()) {
+Expected ToDefaultArgOrErr =
+import(D->getDefaultArgument());
+if (!ToDefaultArgOrErr)
+  return ToDefaultArgOrErr.takeError();
+ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
+  }
+
   return ToD;
 }
 


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -901,6 +901,39 @@
   .isValid());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   TemplateTemplateParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template typename TT> struct Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template struct X {};
+   template typename TT = X> struct Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  const TemplateArgument  = To->getDefaultArgument().getArgument();
+  ASSERT_TRUE(To->isTemplateDecl());
+  TemplateDecl *ToTemplate = ToDefaultArg.getAsTemplate().getAsTemplateDecl();
+
+ 

[clang] 39a5dd1 - [ASTImporter] Import the default argument of TemplateTemplateParmDecl

[Raphael Isemann via cfe-commits]

Author: Raphael Isemann
Date: 2020-11-26T15:12:45+01:00
New Revision: 39a5dd164ca8648e24525869c934c9137c4887ef

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

LOG: [ASTImporter] Import the default argument of TemplateTemplateParmDecl

Same idea as in D92103 and D92106, but I realised after creating those reviews 
that there are
also TemplateTemplateParmDecls that can have default arguments, so here's 
hopefully the
last patch for default template arguments.

Reviewed By: martong

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

Added: 


Modified: 
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp

Removed: 




diff  --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 0886980fe905..835551528e0d 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -5250,15 +5250,22 @@ 
ASTNodeImporter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
   if (!TemplateParamsOrErr)
 return TemplateParamsOrErr.takeError();
 
-  // FIXME: Import default argument.
-
   TemplateTemplateParmDecl *ToD = nullptr;
-  (void)GetImportedOrCreateDecl(
-  ToD, D, Importer.getToContext(),
-  Importer.getToContext().getTranslationUnitDecl(), *LocationOrErr,
-  D->getDepth(), D->getPosition(), D->isParameterPack(),
-  (*NameOrErr).getAsIdentifierInfo(),
-  *TemplateParamsOrErr);
+  if (GetImportedOrCreateDecl(
+  ToD, D, Importer.getToContext(),
+  Importer.getToContext().getTranslationUnitDecl(), *LocationOrErr,
+  D->getDepth(), D->getPosition(), D->isParameterPack(),
+  (*NameOrErr).getAsIdentifierInfo(), *TemplateParamsOrErr))
+return ToD;
+
+  if (D->hasDefaultArgument()) {
+Expected ToDefaultArgOrErr =
+import(D->getDefaultArgument());
+if (!ToDefaultArgOrErr)
+  return ToDefaultArgOrErr.takeError();
+ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
+  }
+
   return ToD;
 }
 

diff  --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 97a18a76622b..33e4b7226fba 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -901,6 +901,39 @@ TEST_P(ASTImporterOptionSpecificTestBase, 
ImportBeginLocOfDeclRefExpr) {
   .isValid());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   TemplateTemplateParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template typename TT> struct Y 
{};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template struct X {};
+   template typename TT = X> struct 
Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  const TemplateArgument  = 
To->getDefaultArgument().getArgument();
+  ASSERT_TRUE(To->isTemplateDecl());
+  TemplateDecl *ToTemplate = ToDefaultArg.getAsTemplate().getAsTemplateDecl();
+
+  // Find the default argument template 'X' in the AST and compare it against
+  // the default argument we got.
+  auto ToExpectedDecl = FirstDeclMatcher().match(
+  To->getTranslationUnitDecl(), classTemplateDecl(hasName("X")));
+  ASSERT_EQ(ToTemplate, ToExpectedDecl);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX03);



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

[PATCH] D92175: [clang-tidy] Add support for loading configs for specific files.

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh, gribozavr2.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, xazax.hun.
Herald added a project: clang.
njames93 requested review of this revision.

There was already a basic version of this in IdentifierNamingCheck, controlled 
by an option GetConfigPerFile. This has now been changed to a global option 
UsePerFileConfig. As there hasn't been a release with GetConfigPerFile, This 
change shouldn't break anyones config.

Now there is support for this built into the base class directly, along with 
some basic caching.
As the clang-tidy providers used only have at best directory resolution, this 
caches based on directory rather than filename. This can potentially save alot 
of redundant config lookups.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92175

Files:
  clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.h
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/clangd/TidyProvider.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
@@ -14,7 +14,7 @@
 // RUN:  -config='{ InheritParentConfig: true, CheckOptions: [ \
 // RUN:   {key: readability-identifier-naming.FunctionCase, value: camelBack}, \
 // RUN:   {key: readability-identifier-naming.ParameterCase, value: CamelCase}, \
-// RUN:   {key: readability-identifier-naming.GetConfigPerFile, value: true} \
+// RUN:   {key: readability-identifier-naming.UsePerFileConfig, value: true} \
 // RUN:  ]}' -header-filter='.*' -- -I%theaders
 
 // On DISABLED run, everything should be made 'camelBack'.
@@ -25,7 +25,7 @@
 // RUN:  -config='{ InheritParentConfig: false, CheckOptions: [ \
 // RUN:   {key: readability-identifier-naming.FunctionCase, value: camelBack}, \
 // RUN:   {key: readability-identifier-naming.ParameterCase, value: CamelCase}, \
-// RUN:   {key: readability-identifier-naming.GetConfigPerFile, value: false} \
+// RUN:   {key: readability-identifier-naming.UsePerFileConfig, value: false} \
 // RUN:  ]}' -header-filter='.*' -- -I%theaders
 
 #include "global-style1/header.h"
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -51,7 +51,6 @@
  - :option:`EnumCase`, :option:`EnumPrefix`, :option:`EnumSuffix`, :option:`EnumIgnoredRegexp`
  - :option:`EnumConstantCase`, :option:`EnumConstantPrefix`, :option:`EnumConstantSuffix`, :option:`EnumConstantIgnoredRegexp`
  - :option:`FunctionCase`, :option:`FunctionPrefix`, :option:`FunctionSuffix`, :option:`FunctionIgnoredRegexp`
- - :option:`GetConfigPerFile`
  - :option:`GlobalConstantCase`, :option:`GlobalConstantPrefix`, :option:`GlobalConstantSuffix`, :option:`GlobalConstantIgnoredRegexp`
  - :option:`GlobalConstantPointerCase`, :option:`GlobalConstantPointerPrefix`, :option:`GlobalConstantPointerSuffix`, :option:`GlobalConstantPointerIgnoredRegexp`
  - :option:`GlobalFunctionCase`, :option:`GlobalFunctionPrefix`, :option:`GlobalFunctionSuffix`, :option:`GlobalFunctionIgnoredRegexp`
@@ -86,6 +85,7 @@
  - :option:`TypedefCase`, :option:`TypedefPrefix`, :option:`TypedefSuffix`, :option:`TypedefIgnoredRegexp`
  - :option:`TypeTemplateParameterCase`, :option:`TypeTemplateParameterPrefix`, :option:`TypeTemplateParameterSuffix`, :option:`TypeTemplateParameterIgnoredRegexp`
  - :option:`UnionCase`, :option:`UnionPrefix`, :option:`UnionSuffix`, :option:`UnionIgnoredRegexp`
+ - :option:`UsePerFileConfig`
  - :option:`ValueTemplateParameterCase`, :option:`ValueTemplateParameterPrefix`, :option:`ValueTemplateParameterSuffix`, :option:`ValueTemplateParameterIgnoredRegexp`
  - :option:`VariableCase`, :option:`VariablePrefix`, :option:`VariableSuffix`, :option:`VariableIgnoredRegexp`
  - :option:`VirtualMethodCase`, :option:`VirtualMethodPrefix`, :option:`VirtualMethodSuffix`, :option:`VirtualMethodIgnoredRegexp`
@@ -790,13 +790,6 @@
 
 char pre_my_function_string_post();
 
-.. option:: GetConfigPerFile
-
-When `true` the check will look for the 

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks for the review! Your comment made me think about whether we handle the 
deduced template class properly (`getDeducedTemplate`). Then I realized we do 
import that when we import the `DeclarationName`. Anyway I added a check for 
that in the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92109

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

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 307848.
martong added a comment.

- Add test for user-defined ctad
- Handle IsCopyDeductionCandidate
- Fix typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92109

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5884,6 +5884,46 @@
   EXPECT_EQ(Record, CompletedTags.front());
 }
 
+TEST_P(ImportFunctions, CTADImplicit) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  template  struct A {
+A(T);
+  };
+  A a{(int)0};
+  )",
+  Lang_CXX17, "input.cc");
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, cxxDeductionGuideDecl());
+  auto *ToD = Import(FromD, Lang_CXX17);
+  ASSERT_TRUE(ToD);
+  EXPECT_EQ(FromD->isCopyDeductionCandidate(), ToD->isCopyDeductionCandidate());
+  // Check that the deduced class template is also imported.
+  EXPECT_TRUE(findFromTU(FromD)->Importer->GetAlreadyImportedOrNull(
+  FromD->getDeducedTemplate()));
+}
+
+TEST_P(ImportFunctions, CTADUserDefinedExplicit) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  template  struct A {
+A(T);
+  };
+  template  explicit A(T) -> A;
+  A a{(int)0}; // calls A::A(float)
+  )",
+  Lang_CXX17, "input.cc");
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, cxxDeductionGuideDecl(unless(isImplicit(;
+  // Not-implicit: i.e. not compiler-generated, user defined.
+  ASSERT_FALSE(FromD->isImplicit());
+  ASSERT_TRUE(FromD->isExplicit()); // Has the explicit keyword.
+  auto *ToD = Import(FromD, Lang_CXX17);
+  ASSERT_TRUE(ToD);
+  EXPECT_FALSE(FromD->isImplicit());
+  EXPECT_TRUE(ToD->isExplicit());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -499,6 +499,7 @@
 ExpectedDecl VisitCXXConstructorDecl(CXXConstructorDecl *D);
 ExpectedDecl VisitCXXDestructorDecl(CXXDestructorDecl *D);
 ExpectedDecl VisitCXXConversionDecl(CXXConversionDecl *D);
+ExpectedDecl VisitCXXDeductionGuideDecl(CXXDeductionGuideDecl *D);
 ExpectedDecl VisitFieldDecl(FieldDecl *D);
 ExpectedDecl VisitIndirectFieldDecl(IndirectFieldDecl *D);
 ExpectedDecl VisitFriendDecl(FriendDecl *D);
@@ -3328,6 +3329,17 @@
   return ToPOrErr.takeError();
   }
 
+  // Common code to import an explicit specifier of different kind of functions.
+  auto ImportExplicitExpr = [this, ](auto *Fun) -> ExpectedExpr {
+Expr *ExplicitExpr = nullptr;
+if (Fun->getExplicitSpecifier().getExpr()) {
+  ExplicitExpr = importChecked(Err, Fun->getExplicitSpecifier().getExpr());
+  if (Err)
+return std::move(Err);
+}
+return ExplicitExpr;
+  };
+
   // Create the imported function.
   FunctionDecl *ToFunction = nullptr;
   if (auto *FromConstructor = dyn_cast(D)) {
@@ -3369,17 +3381,13 @@
 ToDtor->setOperatorDelete(ToOperatorDelete, ToThisArg);
   } else if (CXXConversionDecl *FromConversion =
  dyn_cast(D)) {
-Expr *ExplicitExpr = nullptr;
-if (FromConversion->getExplicitSpecifier().getExpr()) {
-  auto Imp = import(FromConversion->getExplicitSpecifier().getExpr());
-  if (!Imp)
-return Imp.takeError();
-  ExplicitExpr = *Imp;
-}
+ExpectedExpr ExplicitExpr = ImportExplicitExpr(FromConversion);
+if (!ExplicitExpr)
+  return ExplicitExpr.takeError();
 if (GetImportedOrCreateDecl(
 ToFunction, D, Importer.getToContext(), cast(DC),
 ToInnerLocStart, NameInfo, T, TInfo, D->isInlineSpecified(),
-ExplicitSpecifier(ExplicitExpr,
+ExplicitSpecifier(*ExplicitExpr,
   FromConversion->getExplicitSpecifier().getKind()),
 D->getConstexprKind(), SourceLocation(), TrailingRequiresClause))
   return ToFunction;
@@ -3390,6 +3398,18 @@
 Method->isInlineSpecified(), D->getConstexprKind(),
 SourceLocation(), TrailingRequiresClause))
   return ToFunction;
+  } else if (auto *Guide = dyn_cast(D)) {
+ExpectedExpr ExplicitExpr = ImportExplicitExpr(Guide);
+if (!ExplicitExpr)
+  return ExplicitExpr.takeError();
+if (GetImportedOrCreateDecl(
+ToFunction, D, Importer.getToContext(), DC, ToInnerLocStart,
+ExplicitSpecifier(*ExplicitExpr,
+  Guide->getExplicitSpecifier().getKind()),
+NameInfo, T, TInfo, ToEndLoc))
+  return ToFunction;
+cast(ToFunction)
+

[PATCH] D71124: [RISCV] support clang driver to select cpu

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

khchen added a comment.

In D71124#2415424 , @oceanfish81 wrote:

> I tried to build the following:
>
> https://gist.github.com/advancedwebdeveloper/09033869445a09a2e297a73387d46c41/raw/bf7dac33e256621c445f9e61f88a832e50d901a3/llvm_cpu_features_investigation.cpp
>
> but it didn't show me any CPU features, on 
> https://fedoraproject.org/wiki/Architectures/RISC-V/Installing#Boot_under_QEMU

It seems 

 `getHostCPUFeatures` does not support RISCV target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71124

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

[PATCH] D83211: [clang][cli] Factor out call to EXTRACTOR in generateCC1CommandLine (NFC)

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 307839.
jansvoboda11 added a comment.

Undo whitespace change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211

Files:
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -4015,13 +4015,17 @@
 
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
 #define OPTION_WITH_MARSHALLING(   
\
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  
\
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)  
\
-  if (((FLAGS) & options::CC1Option) &&
\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {  
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   
\
+  if ((FLAGS)::CC1Option) {
\
+[&](const auto ) {   
\
+  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) 
\
+DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);  
\
+}(EXTRACTOR(this->KEYPATH));   
\
   }
 
 #define OPTION_WITH_MARSHALLING_BOOLEAN(   
\
@@ -4029,10 +4033,10 @@
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  
\
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,  
\
 NEG_SPELLING)  
\
-  if (((FLAGS)::CC1Option) &&  
\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+  if ((FLAGS)::CC1Option) {
\
+bool Extracted = EXTRACTOR(this->KEYPATH); 
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))   
\
+  DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);  
\
   }
 
 #include "clang/Driver/Options.inc"


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -4015,13 +4015,17 @@
 
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
 #define OPTION_WITH_MARSHALLING(   \
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)  \
-  if (((FLAGS) & options::CC1Option) &&\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {  \
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   \
+  if ((FLAGS)::CC1Option) {\
+[&](const auto ) {   \
+  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) \
+DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);  \
+}(EXTRACTOR(this->KEYPATH));   \
   }
 
 #define OPTION_WITH_MARSHALLING_BOOLEAN(   \
@@ -4029,10 +4033,10 @@
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,  \
 NEG_SPELLING)  \
-  if (((FLAGS)::CC1Option) &&  \
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,\
- EXTRACTOR(this->KEYPATH));\
+  if ((FLAGS)::CC1Option) {\
+bool 

[PATCH] D83211: [clang][cli] Factor out call to EXTRACTOR in generateCC1CommandLine (NFC)

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 307838.
jansvoboda11 added a comment.

Remove lifetime comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/unittests/Option/CMakeLists.txt


Index: llvm/unittests/Option/CMakeLists.txt
===
--- llvm/unittests/Option/CMakeLists.txt
+++ llvm/unittests/Option/CMakeLists.txt
@@ -6,6 +6,7 @@
 set(LLVM_TARGET_DEFINITIONS Opts.td)
 
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
 add_public_tablegen_target(OptsTestTableGen)
 
 add_llvm_unittest(OptionTests
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -4015,13 +4015,17 @@
 
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
 #define OPTION_WITH_MARSHALLING(   
\
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  
\
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)  
\
-  if (((FLAGS) & options::CC1Option) &&
\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {  
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   
\
+  if ((FLAGS)::CC1Option) {
\
+[&](const auto ) {   
\
+  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) 
\
+DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);  
\
+}(EXTRACTOR(this->KEYPATH));   
\
   }
 
 #define OPTION_WITH_MARSHALLING_BOOLEAN(   
\
@@ -4029,10 +4033,10 @@
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  
\
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,  
\
 NEG_SPELLING)  
\
-  if (((FLAGS)::CC1Option) &&  
\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+  if ((FLAGS)::CC1Option) {
\
+bool Extracted = EXTRACTOR(this->KEYPATH); 
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))   
\
+  DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);  
\
   }
 
 #include "clang/Driver/Options.inc"


Index: llvm/unittests/Option/CMakeLists.txt
===
--- llvm/unittests/Option/CMakeLists.txt
+++ llvm/unittests/Option/CMakeLists.txt
@@ -6,6 +6,7 @@
 set(LLVM_TARGET_DEFINITIONS Opts.td)
 
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
 add_public_tablegen_target(OptsTestTableGen)
 
 add_llvm_unittest(OptionTests
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -4015,13 +4015,17 @@
 
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
 #define OPTION_WITH_MARSHALLING(   \
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)  \
-  if (((FLAGS) & options::CC1Option) &&\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {  \
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   \
+  if ((FLAGS)::CC1Option) {\
+[&](const auto ) {   \
+  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) \
+DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);  \
+}(EXTRACTOR(this->KEYPATH)); 

[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 307837.
jansvoboda11 added a comment.
Herald added subscribers: llvm-commits, mgorny.
Herald added a project: LLVM.

Implement review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/unittests/Option/CMakeLists.txt
  llvm/unittests/Option/LifetimeOpts.td


Index: llvm/unittests/Option/LifetimeOpts.td
===
--- /dev/null
+++ llvm/unittests/Option/LifetimeOpts.td
@@ -0,0 +1,5 @@
+include "llvm/Option/OptParser.td"
+
+def lifetime_test : Flag<["-"], "lifetime-test">,
+  MarshallingInfoFlag<"FlagValue">,
+  ValueExtractor<"extractorRequiringLifetimeExtension">;
Index: llvm/unittests/Option/CMakeLists.txt
===
--- llvm/unittests/Option/CMakeLists.txt
+++ llvm/unittests/Option/CMakeLists.txt
@@ -4,8 +4,11 @@
   )
 
 set(LLVM_TARGET_DEFINITIONS Opts.td)
-
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
+set(LLVM_TARGET_DEFINITIONS LifetimeOpts.td)
+tablegen(LLVM LifetimeOpts.inc -gen-opt-parser-defs)
+
 add_public_tablegen_target(OptsTestTableGen)
 
 add_llvm_unittest(OptionTests
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -263,6 +263,9 @@
   return KeyPath | Value;
 }
 
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
 template  static T extractForwardValue(T KeyPath) {
   return KeyPath;
 }
@@ -4015,13 +4018,17 @@
 
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
 #define OPTION_WITH_MARSHALLING(   
\
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  
\
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)  
\
-  if (((FLAGS) & options::CC1Option) &&
\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {  
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   
\
+  if ((FLAGS)::CC1Option) {
\
+[&](const auto ) {   
\
+  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) 
\
+DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);  
\
+}(EXTRACTOR(this->KEYPATH));   
\
   }
 
 #define OPTION_WITH_MARSHALLING_BOOLEAN(   
\
@@ -4029,10 +4036,10 @@
 HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  
\
 NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,  
\
 NEG_SPELLING)  
\
-  if (((FLAGS)::CC1Option) &&  
\
-  (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+  if ((FLAGS)::CC1Option) {
\
+bool Extracted = EXTRACTOR(this->KEYPATH); 
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))   
\
+  DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);  
\
   }
 
 #include "clang/Driver/Options.inc"


Index: llvm/unittests/Option/LifetimeOpts.td
===
--- /dev/null
+++ llvm/unittests/Option/LifetimeOpts.td
@@ -0,0 +1,5 @@
+include "llvm/Option/OptParser.td"
+
+def lifetime_test : Flag<["-"], "lifetime-test">,
+  MarshallingInfoFlag<"FlagValue">,
+  ValueExtractor<"extractorRequiringLifetimeExtension">;
Index: llvm/unittests/Option/CMakeLists.txt
===
--- llvm/unittests/Option/CMakeLists.txt
+++ llvm/unittests/Option/CMakeLists.txt
@@ -4,8 +4,11 @@
   )
 
 set(LLVM_TARGET_DEFINITIONS Opts.td)
-
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
+set(LLVM_TARGET_DEFINITIONS LifetimeOpts.td)
+tablegen(LLVM LifetimeOpts.inc -gen-opt-parser-defs)
+
 add_public_tablegen_target(OptsTestTableGen)
 
 add_llvm_unittest(OptionTests
Index: clang/lib/Frontend/CompilerInvocation.cpp

[PATCH] D92041: [clangd] Add hover info for `this` expr

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks, LGTM!




Comment at: clang-tools-extra/clangd/Hover.cpp:644
+const NamedDecl *D = CTE->getType()->getPointeeType()->getAsTagDecl();
+HI = getHoverContents(D, Index);
+return HI;

nit: `return getHoverContents(D, Index);`


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

https://reviews.llvm.org/D92041

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307834.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.

Resolve another round of comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  R"cpp(
+template
+struct Foo {
+  T [[Vari^able]] = 42;
+};
+
+void foo() {
+  Foo f;
+  f.[[Varia^ble]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T Variable;
+  void bar() { ++Variable; }
+};
+
+template<>
+struct Foo {
+  unsigned [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,28 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(D)) {
+// This is a hacky way to do something like
+// CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
+// Clang AST does not store relevant information about the field that is
+// instantiated.
+const auto *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91949: [clang-format] Add BreakBeforeStructInitialization configuration

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Firstly thank you for the patch, and believe me I don't want to discourage you 
(as I remember my first patches), I think the rational is ok, but I think you 
might be laser focused on your use case and not looking at the impact on the 
change of the wider clang-format.

To be honest I can't really tell without checking it out and building it but 
certainly I think its missing some initialization which perhaps might mean its 
not actually doing anything, (perhaps in debug mode it is) but in release mode 
it could end up making random changes if it gets initialized incorrectly.

I think it "does what it says on the tin", but I think it could have some other 
consequences and side effects which we need to explore first, these are best 
exposed by adding some more tests to show other areas where the same pattern of 
tokens occurs do not change by the setting of this configuration option




Comment at: clang/lib/Format/ContinuationIndenter.cpp:957
+  if (Style.BreakBeforeStructInitialization)
+  Style.ContinuationIndentWidth = 0;
   unsigned ContinuationIndent =

gosh! this seems like it could throw a spanner in the works if we turn this on..



Comment at: clang/lib/Format/TokenAnnotator.cpp:3492
   const FormatToken  = *Right.Previous;
+   if (Style.BreakBeforeStructInitialization && Right.is(tok::l_brace) &&
+   (Right.is(BK_BracedInit) || Left.is(tok::equal)))

This feels quite general and as its high up in this function I'm thinking its 
going to get hit alot...

To me it feels like any construct like this could potentially fall into this 
trap correct

` = { `

what would happen to

`std::vector v = {2, 1};`

did the FormatTests run cleanly, (assuming they did wow!)



Comment at: clang/unittests/Format/FormatTest.cpp:5049
 
+TEST_F(FormatTest, BreakBeforeStructInitialization) {
+  FormatStyle Style = getLLVMStyle();

you are missing a PARSE check test for this



Comment at: clang/unittests/Format/FormatTest.cpp:5052
+  Style.ColumnLimit = 80;
+  Style.BreakBeforeStructInitialization = true;
+  verifyFormat("struct new_struct struct_name = \n"

This is never initialized in the main code? so what value will it have by 
default?


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

https://reviews.llvm.org/D91949

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

[PATCH] D91949: [clang-format] Add BreakBeforeStructInitialization configuration

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Ok, thank you for making the changes so its easier to review, Do you think this 
style should be part of the "BraceMapping" style?


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

https://reviews.llvm.org/D91949

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

[PATCH] D91950: [clang-format] Add BreakBeforeInlineASMColon configuration

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Do you think we should show what it looks like with more complex asm examples 
in the tests?

what would the following look like?

` asm volatile("rbit %[aVal], %[aVal]" : [aVal] "+r"(val) : "[aVal]"(val));`
`__asm__ __volatile__("str x0, %0" : "=m"(regs->regs[0]));`
`asm volatile ("get\t%0,rfsl" stringify(id) : "=d" (val))`

  asm ("mov %1, %0\n\t"
  "add $1, %0"
  : "=r" (dst) 
  : "r" (src));

For the "purpose of the tape" is there a reason or group of people who prefer 
this style? (gcc, linux kernel)


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

https://reviews.llvm.org/D91950

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+// instantiated.
+// FIXME: Maybe a proper way of fixing this would be enhancing Clang AST
+// although it might be a bigger effort.

not sure the `FIXME` is useful here, we may never do that, I'd just drop it.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:138
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+if (!FieldParent)
+  return Field->getCanonicalDecl();

nit: this if branch is not necessary, because the if below already handles that.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:150
+const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember();
+if (OriginalVD)
+  VD = OriginalVD;

nit: we can inline the OriginalVD declaration into the `if`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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

[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam marked 3 inline comments as done.
saiislam added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1344
 
+FieldDecl *clang::CodeGen::CodeGenUtil::CodeGenUtil::addFieldToRecordDecl(
+ASTContext , DeclContext *DC, QualType FieldTy) {

JonChesterfield wrote:
> This appears to be the same as the free function we had before, except now 
> all the call sites are prefixed CodegenUtil. Is there a functional change I'm 
> missing?
> 
> The rest of this patch would be easier to read with this part split off.
addFieldToRecordDecl and createGlobalStruct methods had file static scope. To 
make them callable from other files, from amdgcn specific file in this case, 
they were put in this utility class.

D92167 puts this change into a separate patch. Will update this patch once 
D92167 gets accepted.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:77
+
+void CGOpenMPRuntimeAMDGCN::emitNonSPMDKernelWrapper(
+const OMPExecutableDirective , StringRef ParentName,

JonChesterfield wrote:
> This is a very verbose way to say that amdgcn calls emitmetatdata at the end 
> of emitkernel and nvptx doesn't. Suggest unconditionally calling 
> emitmetatdata, and having emitmetatdata be a no-op for nvptx.
Won't the no-op approach be less extensible? Current way, though verbose, 
leaves scope for attaching prefix/suffix code as and when required around 
emitkernel. While in case of no-op, every implementing arch might have to use 
the exact same pattern of methods with and without code.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:89
+//
+void CGOpenMPRuntimeAMDGCN::setPropertyWorkGroupSize(CodeGenModule ,
+ StringRef Name,

JonChesterfield wrote:
> I think there's a credible chance this is useful to nvptx, so doesn't have to 
> be amdgcn specific
You are right, it can be useful for nvptx as well. May be we can club its 
generalization with the nvptx's use-case when it arrives in the future?



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:108
+  bool FlatAttrEmitted = false;
+  unsigned DefaultWorkGroupSz =
+  CGM.getTarget().getGridValue(llvm::omp::GVIDX::GV_Default_WG_Size);

JonChesterfield wrote:
> I think this is about computing a maximum workgroup size which the runtime 
> uses to limit the number of threads it launches. If so, this is probably 
> useful for nvptx and amdgcn. I'm having trouble working out what the 
> conditions are though. Maybe it's based on an openmp clause?
Yes, the if block in 111-147 corresponds to "number of threads" for 
thread_limit and num_threads clauses in teams and parallel directives.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:169
+  // Create all device images
+  llvm::Constant *AttrData[] = {
+  llvm::ConstantInt::get(CGM.Int16Ty, 2), // Version

JonChesterfield wrote:
> HostServices is unused. Mode is redundant with exec_mode. wg_size is 
> redundant with the other wg_size symbol added above. This kern_desc object 
> should be deleted, not upstreamed.
Ok, thanks. Will update in next revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86097

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

[PATCH] D92012: [clangd][query-driver] Extract target

[Aleksandr Platonov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1ca174b6420a: [clangd][query-driver] Extract target 
(authored by ArcsinX).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92012

Files:
  clang-tools-extra/clangd/QueryDriverDatabase.cpp
  clang-tools-extra/clangd/test/system-include-extractor.test

Index: clang-tools-extra/clangd/test/system-include-extractor.test
===
--- clang-tools-extra/clangd/test/system-include-extractor.test
+++ clang-tools-extra/clangd/test/system-include-extractor.test
@@ -12,6 +12,7 @@
 # RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >> %t.dir/my_driver.sh
 # RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >> %t.dir/my_driver.sh
 # RUN: echo 'echo line to ignore >&2' >> %t.dir/my_driver.sh
+# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >> %t.dir/my_driver.sh
 # RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >> %t.dir/my_driver.sh
 # RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/my_driver.sh
 # RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/my_driver.sh
@@ -45,7 +46,7 @@
   "uri": "file://INPUT_DIR/the-file.cpp",
   "languageId":"cpp",
   "version":1,
-  "text":"#include \n#include "
+  "text":"#include \n#include \n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif"
 }
   }
 }
Index: clang-tools-extra/clangd/QueryDriverDatabase.cpp
===
--- clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -33,6 +33,9 @@
 #include "support/Logger.h"
 #include "support/Path.h"
 #include "support/Trace.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
 #include "clang/Driver/Types.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/DenseMap.h"
@@ -56,55 +59,102 @@
 namespace clangd {
 namespace {
 
-std::vector parseDriverOutput(llvm::StringRef Output) {
+struct DriverInfo {
   std::vector SystemIncludes;
+  std::string Target;
+};
+
+bool isValidTarget(llvm::StringRef Triple) {
+  std::shared_ptr TargetOpts(new TargetOptions);
+  TargetOpts->Triple = Triple.str();
+  DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
+  new IgnoringDiagConsumer);
+  IntrusiveRefCntPtr Target =
+  TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  return bool(Target);
+}
+
+llvm::Optional parseDriverOutput(llvm::StringRef Output) {
+  DriverInfo Info;
   const char SIS[] = "#include <...> search starts here:";
   const char SIE[] = "End of search list.";
+  const char TS[] = "Target: ";
   llvm::SmallVector Lines;
   Output.split(Lines, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
 
-  auto StartIt = llvm::find_if(
-  Lines, [SIS](llvm::StringRef Line) { return Line.trim() == SIS; });
-  if (StartIt == Lines.end()) {
+  enum {
+Initial,// Initial state: searching for target or includes list.
+IncludesExtracting, // Includes extracting.
+Done// Includes and target extraction done.
+  } State = Initial;
+  bool SeenIncludes = false;
+  bool SeenTarget = false;
+  for (auto *It = Lines.begin(); State != Done && It != Lines.end(); ++It) {
+auto Line = *It;
+switch (State) {
+case Initial:
+  if (!SeenIncludes && Line.trim() == SIS) {
+SeenIncludes = true;
+State = IncludesExtracting;
+  } else if (!SeenTarget && Line.trim().startswith(TS)) {
+SeenTarget = true;
+llvm::StringRef TargetLine = Line.trim();
+TargetLine.consume_front(TS);
+// Only detect targets that clang understands
+if (!isValidTarget(TargetLine)) {
+  elog("System include extraction: invalid target \"{0}\", ignoring",
+   TargetLine);
+} else {
+  Info.Target = TargetLine.str();
+  vlog("System include extraction: target extracted: \"{0}\"",
+   TargetLine);
+}
+  }
+  break;
+case IncludesExtracting:
+  if (Line.trim() == SIE) {
+State = SeenTarget ? Done : Initial;
+  } else {
+Info.SystemIncludes.push_back(Line.trim().str());
+vlog("System include extraction: adding {0}", Line);
+  }
+  break;
+default:
+  llvm_unreachable("Impossible state of the driver output parser");
+  break;
+}
+  }
+  if (!SeenIncludes) {
 elog("System include extraction: start marker not found: {0}", Output);
-return {};
+return llvm::None;
   }
-  ++StartIt;
-  const auto EndIt =
-  llvm::find_if(llvm::make_range(StartIt, Lines.end()),
-[SIE](llvm::StringRef Line) { return Line.trim() == SIE; });
-  if (EndIt == Lines.end()) {
+  if 

[clang-tools-extra] 1ca174b - [clangd][query-driver] Extract target

[Aleksandr Platonov via cfe-commits]

Author: Aleksandr Platonov
Date: 2020-11-26T15:08:26+03:00
New Revision: 1ca174b6420a49bcd3331d6f86e237b627163597

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

LOG: [clangd][query-driver] Extract target

In some cases system includes extractions is not enough, we also need target 
specific defines.
The problems appears when clang default target is not the same as toolchain's 
one (GCC cross-compiler, MinGW on Windows).
After this patch `query-driver` also extracts target and adds 
`--target=` compile option.

Reviewed By: sammccall

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

Added: 


Modified: 
clang-tools-extra/clangd/QueryDriverDatabase.cpp
clang-tools-extra/clangd/test/system-include-extractor.test

Removed: 




diff  --git a/clang-tools-extra/clangd/QueryDriverDatabase.cpp 
b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
index 9d731f5563cf..d59263e73994 100644
--- a/clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -33,6 +33,9 @@
 #include "support/Logger.h"
 #include "support/Path.h"
 #include "support/Trace.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
 #include "clang/Driver/Types.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/DenseMap.h"
@@ -56,55 +59,102 @@ namespace clang {
 namespace clangd {
 namespace {
 
-std::vector parseDriverOutput(llvm::StringRef Output) {
+struct DriverInfo {
   std::vector SystemIncludes;
+  std::string Target;
+};
+
+bool isValidTarget(llvm::StringRef Triple) {
+  std::shared_ptr TargetOpts(new TargetOptions);
+  TargetOpts->Triple = Triple.str();
+  DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
+  new IgnoringDiagConsumer);
+  IntrusiveRefCntPtr Target =
+  TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  return bool(Target);
+}
+
+llvm::Optional parseDriverOutput(llvm::StringRef Output) {
+  DriverInfo Info;
   const char SIS[] = "#include <...> search starts here:";
   const char SIE[] = "End of search list.";
+  const char TS[] = "Target: ";
   llvm::SmallVector Lines;
   Output.split(Lines, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
 
-  auto StartIt = llvm::find_if(
-  Lines, [SIS](llvm::StringRef Line) { return Line.trim() == SIS; });
-  if (StartIt == Lines.end()) {
+  enum {
+Initial,// Initial state: searching for target or includes 
list.
+IncludesExtracting, // Includes extracting.
+Done// Includes and target extraction done.
+  } State = Initial;
+  bool SeenIncludes = false;
+  bool SeenTarget = false;
+  for (auto *It = Lines.begin(); State != Done && It != Lines.end(); ++It) {
+auto Line = *It;
+switch (State) {
+case Initial:
+  if (!SeenIncludes && Line.trim() == SIS) {
+SeenIncludes = true;
+State = IncludesExtracting;
+  } else if (!SeenTarget && Line.trim().startswith(TS)) {
+SeenTarget = true;
+llvm::StringRef TargetLine = Line.trim();
+TargetLine.consume_front(TS);
+// Only detect targets that clang understands
+if (!isValidTarget(TargetLine)) {
+  elog("System include extraction: invalid target \"{0}\", ignoring",
+   TargetLine);
+} else {
+  Info.Target = TargetLine.str();
+  vlog("System include extraction: target extracted: \"{0}\"",
+   TargetLine);
+}
+  }
+  break;
+case IncludesExtracting:
+  if (Line.trim() == SIE) {
+State = SeenTarget ? Done : Initial;
+  } else {
+Info.SystemIncludes.push_back(Line.trim().str());
+vlog("System include extraction: adding {0}", Line);
+  }
+  break;
+default:
+  llvm_unreachable("Impossible state of the driver output parser");
+  break;
+}
+  }
+  if (!SeenIncludes) {
 elog("System include extraction: start marker not found: {0}", Output);
-return {};
+return llvm::None;
   }
-  ++StartIt;
-  const auto EndIt =
-  llvm::find_if(llvm::make_range(StartIt, Lines.end()),
-[SIE](llvm::StringRef Line) { return Line.trim() == SIE; 
});
-  if (EndIt == Lines.end()) {
+  if (State == IncludesExtracting) {
 elog("System include extraction: end marker missing: {0}", Output);
-return {};
+return llvm::None;
   }
-
-  for (llvm::StringRef Line : llvm::make_range(StartIt, EndIt)) {
-SystemIncludes.push_back(Line.trim().str());
-vlog("System include extraction: adding {0}", Line);
-  }
-  return SystemIncludes;
+  return std::move(Info);
 }
 
-std::vector
-extractSystemIncludes(PathRef Driver, llvm::StringRef Lang,
-  llvm::ArrayRef 

[PATCH] D92012: [clangd][query-driver] Extract target

[Aleksandr Platonov via Phabricator via cfe-commits]

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

Thanks for review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92012

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307818.
kbobyrev marked 6 inline comments as done.
kbobyrev added a comment.

Resolve remaining comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  R"cpp(
+template
+struct Foo {
+  T [[Vari^able]] = 42;
+};
+
+void foo() {
+  Foo f;
+  f.[[Varia^ble]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T Variable;
+  void bar() { ++Variable; }
+};
+
+template<>
+struct Foo {
+  unsigned [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,33 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(D)) {
+// This is a hacky way to do something like
+// CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
+// Clang AST does not store relevant information about the field that is
+// instantiated.
+// FIXME: Maybe a proper way of fixing this would be enhancing Clang AST
+// although it might be a bigger effort.
+const auto *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember();
+if (OriginalVD)
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91531: [RFC][OpenCL] Provide mechanisms for defining extension macros

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

When reading the documentation [1] for -cl-ext (which I've never used so far), 
I've noticed nothing is said about non-standard configurations (such as 
disabling cl_khr_depth_images with CL2.0). Quickly testing this shows that 
options can be specified to produce non-standard behaviour, as shown by 
https://godbolt.org/z/1Yz1Md.

Is it intentional that -cl-ext allows such non-standard behaviour? (This 
question is not necessarily address to @Anastasia.)
/If so/, then these statements

> Defining __undef_cl_khr_depth_images can alter the default behavior of the 
> predefined macro. This is equivalent to passing -cl-ext=-cl_khr_depth_images.

and

> cl_khr_depth_images is a core functionality of CL2.0 and thefore defining 
> __undef_cl_khr_depth_images doesn't modify the default behavior.

are slightly contradicting each other: the approach with __undef macros seems 
to ensure a more conformant behaviour.

I'm mainly asking for clarification in order to know in which direction we want 
to go, as one could also argue the present documentation doesn't imply 
non-standard behaviour is desired and that the current implementation of 
-cl-ext is buggy.

[1] https://clang.llvm.org/docs/UsersManual.html#cmdoption-cl-ext


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

https://reviews.llvm.org/D91531

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

[PATCH] D92012: [clangd][query-driver] Extract target

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Still LG, thanks!




Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:344
+}
+return Cmd;
   }

kadircet wrote:
> kadircet wrote:
> > ofc we don't need it. but the thing is we are returning an 
> > `Optional` hence the return statement needs to invoke a 
> > constructor for `Optional` and without `std::move` it would invoke a copy 
> > constructor rather than a move-based one.
> > 
> > And even though rest of the calculations are cached per driver 
> > name, this copy is going to happen for every translation unit. So in theory 
> > it would be nice to prevent that.
> oh actually never mind, `Cmd` itself is also an `Optional`, 
> i thought it was a naked `tooling::Command`
It's moot here, but this isn't the case anymore, `return x` can call a 
converting move constructor: 
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579

This is part of C++14, and so we should be able to rely on it in LLVM. It 
wasn't true in the original C++11 spec, but since it was a defect report, 
reasonably-modern compilers allow the optimization even in C++11 mode.

Buggy compilers certainly exist, but I don't think we need to work around them 
for small performance issues. (If the code fails to compile on old-gcc without 
std::move, that's another story...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92012

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