[PATCH] D65725: [analyzer] Mention whether an event is about a condition in a bug report part 2

[Kristóf Umann via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:465
 void assert(int b) {
-  if (!b) // tracking-note{{Assuming 'b' is not equal to 0}}
+  if (!b) // tracking-note{{Assuming 'b' is not equal to 0, which will be (a 
part of a) condition}}
   // tracking-note@-1{{Taking false branch}}

NoQ wrote:
> Do i understand correctly that this test was passing previously, because 
> `expected-note` matches substrings rather than the whole string? If we stick 
> to the current wording, should we slowly switch to 
> `expected-note-re^}}...{{$` in order to avoid such situations?
I was thinking about this as well. I could do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65725



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

r368259 - [diagtool] Use `operator<<(Colors)` to print out colored output.

[Rui Ueyama via cfe-commits]

Author: ruiu
Date: Thu Aug  8 00:04:01 2019
New Revision: 368259

URL: http://llvm.org/viewvc/llvm-project?rev=368259&view=rev
Log:
[diagtool] Use `operator<<(Colors)` to print out colored output.

r368131 introduced this new API to print out messages in colors.
If the colored output is disabled, `operator<<(Colors)` becomes nop.
No functionality change intended.

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

Modified:
cfe/trunk/tools/diagtool/TreeView.cpp

Modified: cfe/trunk/tools/diagtool/TreeView.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/diagtool/TreeView.cpp?rev=368259&r1=368258&r2=368259&view=diff
==
--- cfe/trunk/tools/diagtool/TreeView.cpp (original)
+++ cfe/trunk/tools/diagtool/TreeView.cpp Thu Aug  8 00:04:01 2019
@@ -20,31 +20,14 @@ DEF_DIAGTOOL("tree", "Show warning flags
 using namespace clang;
 using namespace diagtool;
 
-static bool hasColors(const llvm::raw_ostream &out) {
-  if (&out != &llvm::errs() && &out != &llvm::outs())
-return false;
-  return llvm::errs().is_displayed() && llvm::outs().is_displayed();
-}
-
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream &out;
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream &out) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +54,11 @@ public:
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord &GR : Group.subgroups()) {
@@ -85,12 +67,10 @@ public:
 
 if (Internal) {
   for (const DiagnosticRecord &DR : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out << Colors::GREEN;
 out.indent(Indent * 2);
-out << DR.getName();
-resetColor();
-out << "\n";
+out << DR.getName() << Colors::RESET << "\n";
   }
 }
   }
@@ -136,13 +116,8 @@ public:
   }
 
   void showKey() {
-if (ShowColors) {
-  out << '\n';
-  setColor(llvm::raw_ostream::GREEN);
-  out << "GREEN";
-  resetColor();
-  out << " = enabled by default\n\n";
-}
+out << '\n' << Colors::GREEN << "GREEN" << Colors::RESET
+<< " = enabled by default\n\n";
   }
 };
 
@@ -182,6 +157,8 @@ int TreeView::run(unsigned int argc, cha
 return -1;
   }
 
+  out.enable_colors(out.has_colors());
+
   TreePrinter TP(out);
   TP.Internal = Internal;
   TP.showKey();


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

[PATCH] D65854: [diagtool] Use `operator<<(Colors)` to print out colored output.

[Rui Ueyama via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368259: [diagtool] Use `operator<<(Colors)` to print 
out colored output. (authored by ruiu, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65854?vs=213825&id=214071#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65854

Files:
  cfe/trunk/tools/diagtool/TreeView.cpp


Index: cfe/trunk/tools/diagtool/TreeView.cpp
===
--- cfe/trunk/tools/diagtool/TreeView.cpp
+++ cfe/trunk/tools/diagtool/TreeView.cpp
@@ -20,31 +20,14 @@
 using namespace clang;
 using namespace diagtool;
 
-static bool hasColors(const llvm::raw_ostream &out) {
-  if (&out != &llvm::errs() && &out != &llvm::outs())
-return false;
-  return llvm::errs().is_displayed() && llvm::outs().is_displayed();
-}
-
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream &out;
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream &out) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +54,11 @@
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord &GR : Group.subgroups()) {
@@ -85,12 +67,10 @@
 
 if (Internal) {
   for (const DiagnosticRecord &DR : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out << Colors::GREEN;
 out.indent(Indent * 2);
-out << DR.getName();
-resetColor();
-out << "\n";
+out << DR.getName() << Colors::RESET << "\n";
   }
 }
   }
@@ -136,13 +116,8 @@
   }
 
   void showKey() {
-if (ShowColors) {
-  out << '\n';
-  setColor(llvm::raw_ostream::GREEN);
-  out << "GREEN";
-  resetColor();
-  out << " = enabled by default\n\n";
-}
+out << '\n' << Colors::GREEN << "GREEN" << Colors::RESET
+<< " = enabled by default\n\n";
   }
 };
 
@@ -182,6 +157,8 @@
 return -1;
   }
 
+  out.enable_colors(out.has_colors());
+
   TreePrinter TP(out);
   TP.Internal = Internal;
   TP.showKey();


Index: cfe/trunk/tools/diagtool/TreeView.cpp
===
--- cfe/trunk/tools/diagtool/TreeView.cpp
+++ cfe/trunk/tools/diagtool/TreeView.cpp
@@ -20,31 +20,14 @@
 using namespace clang;
 using namespace diagtool;
 
-static bool hasColors(const llvm::raw_ostream &out) {
-  if (&out != &llvm::errs() && &out != &llvm::outs())
-return false;
-  return llvm::errs().is_displayed() && llvm::outs().is_displayed();
-}
-
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream &out;
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream &out) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +54,11 @@
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord &GR : Group.subgroups()) {
@@ -85,12 +67,10 @@
 
 if (Internal) {
   for (const DiagnosticRecord &DR : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out

[clang-tools-extra] r368261 - [clangd] Fix implicit template instatiations appearing as topLevelDecls.

[Johan Vikstrom via cfe-commits]

Author: jvikstrom
Date: Thu Aug  8 00:21:06 2019
New Revision: 368261

URL: http://llvm.org/viewvc/llvm-project?rev=368261&view=rev
Log:
[clangd] Fix implicit template instatiations appearing as topLevelDecls.

Summary: The parser gives implicit template instantiations to the action's 
HandleTopLevelDecls callback. This makes semantic highlighting highlight these 
templated functions multiple times. Fixed by filtering on if a Decl is an 
implicit function/variable/class instantiation. Also added a testcase to 
semantic highlighting on this.

Reviewers: hokein, ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/AST.cpp
clang-tools-extra/trunk/clangd/AST.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

Modified: clang-tools-extra/trunk/clangd/AST.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/AST.cpp?rev=368261&r1=368260&r2=368261&view=diff
==
--- clang-tools-extra/trunk/clangd/AST.cpp (original)
+++ clang-tools-extra/trunk/clangd/AST.cpp Thu Aug  8 00:21:06 2019
@@ -15,6 +15,7 @@
 #include "clang/AST/TemplateBase.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Casting.h"
@@ -41,13 +42,57 @@ getTemplateSpecializationArgLocs(const N
   // contain TemplateArgumentLoc information.
   return llvm::None;
 }
+
+template 
+bool isTemplateSpecializationKind(const NamedDecl *D,
+  TemplateSpecializationKind Kind) {
+  if (const auto *TD = dyn_cast(D))
+return TD->getTemplateSpecializationKind() == Kind;
+  return false;
+}
+
+bool isTemplateSpecializationKind(const NamedDecl *D,
+  TemplateSpecializationKind Kind) {
+  return isTemplateSpecializationKind(D, Kind) ||
+ isTemplateSpecializationKind(D, Kind) ||
+ isTemplateSpecializationKind(D, Kind);
+}
+
 } // namespace
 
+bool isImplicitTemplateInstantiation(const NamedDecl *D) {
+  return isTemplateSpecializationKind(D, TSK_ImplicitInstantiation);
+}
+
+bool isExplicitTemplateSpecialization(const NamedDecl *D) {
+  return isTemplateSpecializationKind(D, TSK_ExplicitSpecialization);
+}
+
 bool isImplementationDetail(const Decl *D) {
   return !isSpelledInSource(D->getLocation(),
 D->getASTContext().getSourceManager());
 }
 
+// Returns true if the complete name of decl \p D is spelled in the source 
code.
+// This is not the case for:
+//   * symbols formed via macro concatenation, the spelling location will
+// be ""
+//   * symbols controlled and defined by a compile command-line option
+// `-DName=foo`, the spelling location will be "".
+bool isSpelledInSourceCode(const Decl *D) {
+  const auto &SM = D->getASTContext().getSourceManager();
+  auto Loc = D->getLocation();
+  // FIXME: Revisit the strategy, the heuristic is limitted when handling
+  // macros, we should use the location where the whole definition occurs.
+  if (Loc.isMacroID()) {
+std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
+if (llvm::StringRef(PrintLoc).startswith(""))
+  return false;
+  }
+  return true;
+}
+
 SourceLocation findName(const clang::Decl *D) {
   return D->getLocation();
 }

Modified: clang-tools-extra/trunk/clangd/AST.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/AST.h?rev=368261&r1=368260&r2=368261&view=diff
==
--- clang-tools-extra/trunk/clangd/AST.h (original)
+++ clang-tools-extra/trunk/clangd/AST.h Thu Aug  8 00:21:06 2019
@@ -80,9 +80,23 @@ std::string printType(const QualType QT,
 /// take in to account using directives etc
 /// Example: shortenNamespace("ns1::MyClass", "ns1")
 ///--> "MyClass"
-std::string  shortenNamespace(const llvm::StringRef OriginalName,
-  const llvm::StringRef CurrentNamespace);
+std::string shortenNamespace(const llvm::StringRef OriginalName,
+ const llvm::StringRef CurrentNamespace);
 
+/// Indicates if \p D is a template instantiation implicitly generated by the
+/// compiler, e.g.
+/// template  struct vector {};
+/// vector v; // 'vector' is an implicit instantiation
+bool isImplicitTemplateInstantiation(const NamedDecl *D);
+/// Indicates if \p D is an explicit template specialization, e.g.
+///   template  struct vector {};
+///   template <> struct vector {}; // <-- explicit specialization
+///
+/// Note that explicit

[PATCH] D65510: [clangd] Fix implicit template instatiations appearing as topLevelDecls.

[Johan Vikström via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
jvikstrom marked 4 inline comments as done.
Closed by commit rL368261: [clangd] Fix implicit template instatiations 
appearing as topLevelDecls. (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65510?vs=213878&id=214074#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65510

Files:
  clang-tools-extra/trunk/clangd/AST.cpp
  clang-tools-extra/trunk/clangd/AST.h
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
  clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -9,6 +9,7 @@
 #include "ClangdUnit.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
+#include "AST.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "Headers.h"
@@ -19,6 +20,7 @@
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
@@ -70,6 +72,9 @@
   auto &SM = D->getASTContext().getSourceManager();
   if (!isInsideMainFile(D->getLocation(), SM))
 continue;
+  if (const NamedDecl *ND = dyn_cast(D))
+if (isImplicitTemplateInstantiation(ND))
+  continue;
 
   // ObjCMethodDecl are not actually top-level decls.
   if (isa(D))
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -249,6 +249,12 @@
 
   template
   void $Function[[foo]]($TemplateParameter[[T]] ...);
+)cpp",
+R"cpp(
+  template 
+  struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
+  extern template struct $Class[[Tmpl]];
+  template struct $Class[[Tmpl]];
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
@@ -6,13 +6,16 @@
 //
 //===--===//
 
+#include "AST.h"
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
 #include "TestTU.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -21,6 +24,8 @@
 namespace {
 
 using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
+using ::testing::AllOf;
 
 TEST(ClangdUnitTest, GetBeginningOfIdentifier) {
   std::string Preamble = R"cpp(
@@ -72,6 +77,31 @@
   return false;
 }
 
+// Matches if the Decl has template args equal to ArgName. If the decl is a
+// NamedDecl and ArgName is an empty string it also matches.
+MATCHER_P(WithTemplateArgs, ArgName, "") {
+  if (const FunctionDecl *FD = dyn_cast(arg)) {
+if (const auto *Args = FD->getTemplateSpecializationArgs()) {
+  std::string SpecializationArgs;
+  // Without the PrintingPolicy "bool" will be printed as "_Bool".
+  LangOptions LO;
+  PrintingPolicy Policy(LO);
+  Policy.adjustForCPlusPlus();
+  for (const auto Arg : Args->asArray()) {
+if (SpecializationArgs.size() > 0)
+  SpecializationArgs += ",";
+SpecializationArgs += Arg.getAsType().getAsString(Policy);
+  }
+  if (Args->size() == 0)
+return ArgName == SpecializationArgs;
+  return ArgName == "<" + SpecializationArgs + ">";
+}
+  }
+  if (const NamedDecl *ND = dyn_cast(arg))
+return printTemplateSpecializationArgs(*ND) == ArgName;
+  return false;
+}
+
 TEST(ClangdUnitTest, TopLevelDecls) {
   TestTU TU;
   TU.HeaderCode = R"(
@@ -103,6 +133,65 @@
   EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
 }
 
+TEST(ClangdUnitTest, DoesNotGetImplicitTemplateTopDecls) {
+  TestTU TU;
+  TU.Code = R"cpp(
+template
+void f(T) {}
+void s() {
+  f(10UL);
+}
+  )cpp";
+
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),

[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

[Kristóf Umann via Phabricator via cfe-commits]

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

After testing this patch out, I'm confident it works pretty well. Take a look 
at the following two runs: 138 notes 

 -> 20 notes 
.




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1753-1755
+  // It so happens that CFGBlock::getTerminatorCondition returns 'A' for block
+  // B1, 'A && B' for B2, and 'A && B || C' for B3. Let's check whether we
+  // reached the end of the condition!

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Clever trick, but why not match for logical operators directly? Something 
> > > like this:
> > > ```lang=c++
> > > if (auto B = dyn_cast(OuterCond))
> > >   if (B->isLogicalOp())
> > > return isAssertlikeBlock(Else, Context);
> > > ```
> > What about `assert(a + b && "Shouldn't be zero!");`?
> Mmm, could you elaborate? >.<
```lang=c++
if (auto B = dyn_cast(OuterCond))
  if (B->isLogicalOp())
return isAssertlikeBlock(Else, Context);
```
We don't need `isLogicalOp()` here.

Also, I should probably have a testcase for the elvis operator (`?:`).



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:469-471
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > I'm pretty sure you can define this function only once.
> > Note that it is in a namespace!
> I mean, why is it in a namespace? Why not just define it once for the whole 
> file? It's not like you're gonna be trying out different variants of 
> `__assert_fail`.
Yea, good point.


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

https://reviews.llvm.org/D65287



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

[PATCH] D65926: [clangd] Fixed printTemplateSpecializationArgs not printing partial variable specialization arguments.

[Johan Vikström via Phabricator via cfe-commits]

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

printTemplateSpecializationArgs was not printing partial variable 
specialization args. This adds an additional If clause where we check if it's a 
VariableTemplatePartialSpecializationDecl and returns the ArgumentLocs if 
that's the case.
Also adds tests for printTemplateSpecializationArgs in ASTTests.cpp.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65926

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


Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -188,7 +188,7 @@
 AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("i"), WithTemplateArgs("")),
 AllOf(DeclNamed("d"), WithTemplateArgs("")),
-AllOf(DeclNamed("foo"), WithTemplateArgs("<>")),
+AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("foo"), WithTemplateArgs(""))}));
 }
 
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -8,6 +8,7 @@
 
 #include "AST.h"
 #include "gtest/gtest.h"
+#include "TestTU.h"
 
 namespace clang {
 namespace clangd {
@@ -36,6 +37,44 @@
 "testns1::TestClass", "testns1"));
 }
 
+TEST(PrintTemplateSpecializationArgs, PrintsTemplateArgs) {
+  TestTU TU;
+  TU.Code = R"cpp(
+template
+void foo(T) {}
+template<>
+void foo(int) {}
+
+template
+struct K {};
+template
+struct K {};
+template<>
+struct K {};
+
+template
+T S = T(10);
+
+template 
+int S = 0;
+template <>
+int S = 0;
+  )cpp";
+
+  // The expected template args string representation for every top level decl
+  // in the TU.
+  std::vector ExpectedTemplateDeclArgs{
+  "", "", "", "", "", "", "", ""};
+
+  std::vector TopLevel = TU.build().getLocalTopLevelDecls();
+  std::vector ActualTemplateArgs;
+  for (const Decl *D : TopLevel) {
+if (const NamedDecl *ND = dyn_cast(D))
+  ActualTemplateArgs.push_back(printTemplateSpecializationArgs(*ND));
+  }
+
+  EXPECT_EQ(ExpectedTemplateDeclArgs, ActualTemplateArgs);
+}
 
 } // namespace
 } // namespace clangd
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -36,6 +36,10 @@
  llvm::dyn_cast(&ND)) {
 if (auto *Args = Cls->getTemplateArgsAsWritten())
   return Args->arguments();
+  } else if (auto *Var =
+ llvm::dyn_cast(&ND)) {
+if (auto *Args = Var->getTemplateArgsAsWritten())
+  return Args->arguments();
   } else if (auto *Var = llvm::dyn_cast(&ND))
 return Var->getTemplateArgsInfo().arguments();
   // We return None for ClassTemplateSpecializationDecls because it does not


Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -188,7 +188,7 @@
 AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("i"), WithTemplateArgs("")),
 AllOf(DeclNamed("d"), WithTemplateArgs("")),
-AllOf(DeclNamed("foo"), WithTemplateArgs("<>")),
+AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("foo"), WithTemplateArgs(""))}));
 }
 
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -8,6 +8,7 @@
 
 #include "AST.h"
 #include "gtest/gtest.h"
+#include "TestTU.h"
 
 namespace clang {
 namespace clangd {
@@ -36,6 +37,44 @@
 "testns1::TestClass", "testns1"));
 }
 
+TEST(PrintTemplateSpecializationArgs, PrintsTemplateArgs) {
+  TestTU TU;
+  TU.Code = R"cpp(
+template
+void foo(T) {}
+template<>
+void foo(int) {}
+
+template
+struct K {};
+template
+struct K {};
+template<>
+struct K {};
+
+template
+T S = T(10);
+
+template 
+int S = 0;
+template <>
+int S = 0;
+  )cpp";
+
+  // The expected template args string re

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Johan Vikström via Phabricator via cfe-commits]

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

Conversion operators contain invalid MemberLocs which causes 
SemanticHighlighting to emit a lot of error logs in large files as they can 
occur fairly often (for example converting StringRef to std string).
As the only thing happening was a lot of error logs being emited there doesn't 
really seem to be any way to test this (no erroneous tokens are added). But 
emiting as many logs as were being emited is not wanted.

Can't really be patched elsewhere. RecursiveASTVisitor should still traverse 
the expr as it can contain other "non-implicit" decls/exprs that must be 
visited (for example DeclRefs). A potential fix could be to special case the 
TraverseMemberExpr function to not Walk* the Expr if it has an invalid 
MemberLoc. But that would change the behaviour of RecursiveASTVisitor to be 
unexpected. (and to change the behaviour to be this for all exprs would change 
the contract of RecursiveASTVisitor to much)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65928

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


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,11 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (ME->getMemberLoc().isInvalid())
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying
+  // to add a token with an invalid range.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,11 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (ME->getMemberLoc().isInvalid())
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying
+  // to add a token with an invalid range.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r368267 - [Extract] Fixed SemicolonExtractionPolicy for SwitchStmt and SwitchCase

[Shaurya Gupta via cfe-commits]

Author: sureyeaah
Date: Thu Aug  8 01:37:49 2019
New Revision: 368267

URL: http://llvm.org/viewvc/llvm-project?rev=368267&view=rev
Log:
[Extract] Fixed SemicolonExtractionPolicy for SwitchStmt and SwitchCase

Reviewers: arphaman, sammccall

Subscribers: dexonsmith, cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp

Modified: cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp?rev=368267&r1=368266&r2=368267&view=diff
==
--- cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp Thu Aug  8 
01:37:49 2019
@@ -40,8 +40,11 @@ bool isSemicolonRequiredAfter(const Stmt
 return isSemicolonRequiredAfter(CXXFor->getBody());
   if (const auto *ObjCFor = dyn_cast(S))
 return isSemicolonRequiredAfter(ObjCFor->getBody());
+  if(const auto *Switch = dyn_cast(S))
+return isSemicolonRequiredAfter(Switch->getBody());
+  if(const auto *Case = dyn_cast(S))
+return isSemicolonRequiredAfter(Case->getSubStmt());
   switch (S->getStmtClass()) {
-  case Stmt::SwitchStmtClass:
   case Stmt::CXXTryStmtClass:
   case Stmt::ObjCAtSynchronizedStmtClass:
   case Stmt::ObjCAutoreleasePoolStmtClass:

Modified: cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp?rev=368267&r1=368266&r2=368267&view=diff
==
--- cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp (original)
+++ cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp Thu Aug  8 
01:37:49 2019
@@ -64,6 +64,7 @@ void extractStatementNotSemiSwitch() {
 // CHECK-NEXT: extracted();{{$}}
 // CHECK-NEXT: }
 
+
 void extractStatementNotSemiWhile() {
   /*range eextract=->+2:4*/while (true) {
 int x = 0;
@@ -190,3 +191,15 @@ void careForNonCompoundSemicolons2() {
 // CHECK-NEXT: extracted();{{$}}
 // CHECK-NEXT: //
 // CHECK-NEXT: }
+
+void careForSwitchSemicolon() {
+  /*range mextract=->+0:51*/switch(0) default: break;
+}
+// CHECK: 1 'mextract' results:
+// CHECK:  static void extracted() {
+// CHECK-NEXT: switch(0) default: break;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void careForSwitchSemicolon() {
+// CHECK-NEXT: extracted();{{$}}
+// CHECK-NEXT: }
+


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

[PATCH] D65883: [Extract] Fixed SemicolonExtractionPolicy for SwitchStmt and SwitchCase

[Shaurya Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368267: [Extract] Fixed SemicolonExtractionPolicy for 
SwitchStmt and SwitchCase (authored by SureYeaah, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65883?vs=213925&id=214084#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65883

Files:
  cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
  cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp


Index: cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
===
--- cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
+++ cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
@@ -64,6 +64,7 @@
 // CHECK-NEXT: extracted();{{$}}
 // CHECK-NEXT: }
 
+
 void extractStatementNotSemiWhile() {
   /*range eextract=->+2:4*/while (true) {
 int x = 0;
@@ -190,3 +191,15 @@
 // CHECK-NEXT: extracted();{{$}}
 // CHECK-NEXT: //
 // CHECK-NEXT: }
+
+void careForSwitchSemicolon() {
+  /*range mextract=->+0:51*/switch(0) default: break;
+}
+// CHECK: 1 'mextract' results:
+// CHECK:  static void extracted() {
+// CHECK-NEXT: switch(0) default: break;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void careForSwitchSemicolon() {
+// CHECK-NEXT: extracted();{{$}}
+// CHECK-NEXT: }
+
Index: cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
@@ -40,8 +40,11 @@
 return isSemicolonRequiredAfter(CXXFor->getBody());
   if (const auto *ObjCFor = dyn_cast(S))
 return isSemicolonRequiredAfter(ObjCFor->getBody());
+  if(const auto *Switch = dyn_cast(S))
+return isSemicolonRequiredAfter(Switch->getBody());
+  if(const auto *Case = dyn_cast(S))
+return isSemicolonRequiredAfter(Case->getSubStmt());
   switch (S->getStmtClass()) {
-  case Stmt::SwitchStmtClass:
   case Stmt::CXXTryStmtClass:
   case Stmt::ObjCAtSynchronizedStmtClass:
   case Stmt::ObjCAutoreleasePoolStmtClass:


Index: cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
===
--- cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
+++ cfe/trunk/test/Refactor/Extract/ExtractionSemicolonPolicy.cpp
@@ -64,6 +64,7 @@
 // CHECK-NEXT: extracted();{{$}}
 // CHECK-NEXT: }
 
+
 void extractStatementNotSemiWhile() {
   /*range eextract=->+2:4*/while (true) {
 int x = 0;
@@ -190,3 +191,15 @@
 // CHECK-NEXT: extracted();{{$}}
 // CHECK-NEXT: //
 // CHECK-NEXT: }
+
+void careForSwitchSemicolon() {
+  /*range mextract=->+0:51*/switch(0) default: break;
+}
+// CHECK: 1 'mextract' results:
+// CHECK:  static void extracted() {
+// CHECK-NEXT: switch(0) default: break;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void careForSwitchSemicolon() {
+// CHECK-NEXT: extracted();{{$}}
+// CHECK-NEXT: }
+
Index: cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Extract/SourceExtraction.cpp
@@ -40,8 +40,11 @@
 return isSemicolonRequiredAfter(CXXFor->getBody());
   if (const auto *ObjCFor = dyn_cast(S))
 return isSemicolonRequiredAfter(ObjCFor->getBody());
+  if(const auto *Switch = dyn_cast(S))
+return isSemicolonRequiredAfter(Switch->getBody());
+  if(const auto *Case = dyn_cast(S))
+return isSemicolonRequiredAfter(Case->getSubStmt());
   switch (S->getStmtClass()) {
-  case Stmt::SwitchStmtClass:
   case Stmt::CXXTryStmtClass:
   case Stmt::ObjCAtSynchronizedStmtClass:
   case Stmt::ObjCAutoreleasePoolStmtClass:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56
+if (ME->getMemberLoc().isInvalid())
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying

I think we only want to ignore the conversion operator, we could do an early 
return when MD is a cxx conversion decl, e.g. ` isa(MD)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928



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

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

hfinkel wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > > Telling the compiler `ps` is a device pointer only to create 
> > > > > > > > > a local uninitialized shadowing variable seems like an error 
> > > > > > > > > to me.
> > > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > > created in the context of the parallel region, not the target 
> > > > > > > > region. So, for OpenMP 5.0 we should not emit error here.
> > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > It means, that for OpenMP 5.0 we should emit a warning/error here. 
> > > > > > It is allowed according to the standard, we should allow it too.
> > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > 
> > > > > The last answer contradicts what you said earlier. I expect there is 
> > > > > a *not* missing, correct?
> > > > > 
> > > > > 
> > > > > Assuming you do not want an error, which is fine, I still think a 
> > > > > warning is appropriate as it seems to me there is never a reason to 
> > > > > have a `is_device_ptr` clause for a private value. I mean, it is 
> > > > > either a bug by the programmer, e.g., 5 letters of `firstprivate` 
> > > > > went missing, or simply nonsensical code for which we warn in other 
> > > > > situations as well.
> > > > Missed `not`.
> > > > These kind of construct are explicitly allowed in OpenMP. And we should 
> > > > obey the standard unconditionally.
> > > > Plus, there might be situations where user may require it explicitly. 
> > > > For example, the device pointer is dereferenced in one of the clauses 
> > > > for the subregions but in the deeper subregion it might be used as a 
> > > > private pointer. Why we should emit a warning here?
> > > If you have a different situation, e.g., the one you describe, you should 
> > > not have a warning. Though, that is not the point. If you have the 
> > > situation above (single directive), as per my reasoning, there doesn't 
> > > seem to be a sensible use case. If you have one and we should create an 
> > > explicit test for it.
> > > 
> > > > These kind of construct are explicitly allowed in OpenMP.
> > > `explicitly allowed` != `not forbidded` (yet)
> > > > And we should obey the standard unconditionally.
> > > Nobody says we should not. We warn people all the time even if it is 
> > > valid code.
> > Warnings may prevent successful compilation in some cases, e.g. when 
> > warnings are treated as errors. Why we should emit a warning if the 
> > construct is allowed by the standard? Ask to change the standard if you did 
> > not agree with it.
> Warnings are specifically for constructs which are legal, but likely wrong 
> (i.e., will not do what the user likely intended). Treating warnings as 
> errors is not a conforming compilation mode - by design (specifically because 
> that will reject conforming programs). Thus...
> 
> > Why we should emit a warning if the construct is allowed by the standard? 
> > Ask to change the standard if you did not agree with it.
> 
> This is the wrong way to approach this. Warnings are specifically for legal 
> code. They help users prevent errors, however, in cases where that legal code 
> is likely problematic or won't do what the user intends.
> 
Ok, we could emit wqrnings in some cases. But better to do it in the separate 
patches. Each particular case requires additional analysis.

> This is the wrong way to approach this.

I don't think so. If some cases are really meaningless, better to ask to 
prohibit them in the standard. It is always a good idea to change the 
requirements first, if you think that some scenarios are not described 
correctly rather than do the changes in the code. It leads to different 
behavior of different compilers in the same situation and it is not good for 
the users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65835



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

[PATCH] D62960: Add SVE opaque built-in types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm updated this revision to Diff 214095.
rsandifo-arm edited the summary of this revision.
rsandifo-arm added a comment.

- Remove pointless "SVE Types" comments
- Expand commentary about SVE type layout and future debug info handling
- Make macro formatting more consistent with the surrounding code


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/AArch64SVEACLETypes.def
  include/clang/Basic/TargetInfo.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTImporter.cpp
  lib/AST/ExprConstant.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/NSAPI.cpp
  lib/AST/PrintfFormatString.cpp
  lib/AST/Type.cpp
  lib/AST/TypeLoc.cpp
  lib/Basic/TargetInfo.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CodeGenTypes.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/Index/USRGeneration.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTCommon.cpp
  lib/Serialization/ASTReader.cpp
  test/AST/ast-dump-aarch64-sve-types.c
  test/CodeGen/aarch64-sve.c
  test/CodeGenCXX/aarch64-mangle-sve-vectors-msvc.cpp
  test/CodeGenCXX/aarch64-mangle-sve-vectors.cpp
  test/CodeGenCXX/aarch64-sve-typeinfo.cpp
  test/CodeGenObjC/aarch64-sve-types.m
  test/PCH/aarch64-sve-types.c
  test/Sema/aarch64-sve-types.c
  test/SemaObjC/aarch64-sve-types.m
  tools/libclang/CIndex.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -5236,6 +5236,44 @@
   EXPECT_EQ(ImportedX->getCanonicalDecl(), ToX->getCanonicalDecl());
 }
 
+struct SVEBuiltins : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(SVEBuiltins, ImportTypes) {
+  static const char *const TypeNames[] = {
+"__SVInt8_t",
+"__SVInt16_t",
+"__SVInt32_t",
+"__SVInt64_t",
+"__SVUint8_t",
+"__SVUint16_t",
+"__SVUint32_t",
+"__SVUint64_t",
+"__SVFloat16_t",
+"__SVFloat32_t",
+"__SVFloat64_t",
+"__SVBool_t"
+  };
+
+  TranslationUnitDecl *ToTU = getToTuDecl("", Lang_CXX);
+  TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cc");
+  for (auto *TypeName : TypeNames) {
+auto *ToTypedef = FirstDeclMatcher().match(
+  ToTU, typedefDecl(hasName(TypeName)));
+QualType ToType = ToTypedef->getUnderlyingType();
+
+auto *FromTypedef = FirstDeclMatcher().match(
+  FromTU, typedefDecl(hasName(TypeName)));
+QualType FromType = FromTypedef->getUnderlyingType();
+
+QualType ImportedType = ImportType(FromType, FromTypedef, Lang_CXX);
+EXPECT_EQ(ImportedType, ToType);
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins,
+::testing::Values(ArgVector{"-target",
+"aarch64-linux-gnu"}), );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -1527,6 +1527,9 @@
   case BuiltinType::OCLClkEvent:
   case BuiltinType::OCLQueue:
   case BuiltinType::OCLReserveID:
+#define SVE_TYPE(Name, Id, SingletonId) \
+  case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
 #define BUILTIN_TYPE(Id, SingletonId)
 #define SIGNED_TYPE(Id, SingletonId) case BuiltinType::Id:
 #define UNSIGNED_TYPE(Id, SingletonId) case BuiltinType::Id:
Index: test/SemaObjC/aarch64-sve-types.m
===
--- /dev/null
+++ test/SemaObjC/aarch64-sve-types.m
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -fsyntax-only -verify %s
+
+// Check that we don't abort when SVE types are made nullable.  This
+// interface is invalid anyway, but we won't diagnose that until the
+// sizeless type extension is added.
+@interface foo
+@property(nullable) __SVInt8_t s8; // expected-error {{cannot be applied to non-pointer type}}
+@property(nullable) __SVInt16_t s16; // expected-error {{cannot be applied to non-pointer type}}
+@property(nullable) __SVInt32_t s32; // expected-error {{cannot be applied to non-pointer type}}
+@property(nullable) __SVInt64_t s64; // expected-error {{cannot be applied to non-pointer type}}
+
+@property(nullable) __SVUint8_t u8; // expected-error {{cannot be applied to non-pointer type}}
+@property(nullable) __SVUint16_t u16; // expected-error {{cannot be applied to non-pointer type}}
+@property(nullable) __SVUint32_t u32; // expected-error {{cannot be applied to non-pointer type}}
+@property(nullable) __SVUint64_t u64; // expected-error {{cannot be applied to non-pointer type}}
+
+@property(nul

[PATCH] D62960: Add SVE opaque built-in types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm marked 7 inline comments as done.
rsandifo-arm added inline comments.



Comment at: lib/AST/ASTContext.cpp:1974
+  break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
 }

rjmccall wrote:
> Why do SVE predicates have 16-bit alignment?  Should this be 128-bit 
> (16-*byte*)?
> 
> I guess these alignments are reasonable to hard-code here since they're 
> target-specific for now.  That might be worth including in the comment.
Yeah, in retrospect this is sorely lacking a comment.  The reason for using 16 
is that there is one predicate bit for each vector byte, so the predicate size 
is a runtime multiple of 16 bits.  I've added a comment to say that and added a 
reference to the ABI that defines the alignments.



Comment at: lib/AST/ItaniumMangle.cpp:2680
+break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
   }

rjmccall wrote:
> rjmccall wrote:
> > rsandifo-arm wrote:
> > > rovka wrote:
> > > > erik.pilkington wrote:
> > > > > rsandifo-arm wrote:
> > > > > > erik.pilkington wrote:
> > > > > > > jfb wrote:
> > > > > > > > @rjmccall you probably should review this part.
> > > > > > > Sorry for the drive by comment, but: All of these mangling should 
> > > > > > > really be using the "vendor extension" production IMO:
> > > > > > > 
> > > > > > > ` ::= u `
> > > > > > > 
> > > > > > > As is, these manglings intrude on the users's namespace, (i.e. if 
> > > > > > > they had a type named `objc_selector` or something), and confuse 
> > > > > > > demanglers which incorrectly assume these are substitutable 
> > > > > > > (vendor extension builtin types are substitutable too though, but 
> > > > > > > that should be handled here).
> > > > > > It isn't obvious from the patch, but the SVE names that we're 
> > > > > > mangling are predefined names like __SVInt8_t. rather than 
> > > > > > user-facing names like svint8_t  The predefined names and their 
> > > > > > mangling are defined by the platform ABI 
> > > > > > (https://developer.arm.com/docs/100986/), so it wouldn't be 
> > > > > > valid for another part of the implementation to use those names for 
> > > > > > something else.
> > > > > > 
> > > > > > I realise you were making a general point here though, sorry.
> > > > > > 
> > > > > The mangling in the document you linked does use the vendor extension 
> > > > > production here though, i.e. the example is `void f(int8x8_t)`, which 
> > > > > mangles to _Z1f**u10__Int8x8_t**. It is true that this shouldn't ever 
> > > > > collide with another mangling in practice, but my point is there 
> > > > > isn't any need to smuggle it into the mangling by pretending it's a 
> > > > > user defined type, when the itanium grammar and related tools have a 
> > > > > special way for vendors to add builtin types.
> > > > I agree with Erik here, the example in the PCS document seems to 
> > > > suggest using u. I think either the patch needs to be updated or the 
> > > > document needs to be more clear about what the mangling is supposed to 
> > > > look like.
> > > Thanks for highlighting this problem, and sorry for not noticing myself 
> > > when pointing you at the doc.
> > > 
> > > Unfortunately, the specification and implementation already difer for the 
> > > Advanced SIMD types, with both clang and GCC omitting the 'u' despite the 
> > > spec saying it should be present.  So we're considering changing the spec 
> > > to match what's now the de facto ABI.
> > > 
> > > For SVE we do still have the opportunity to use 'u'.  I've left it as-is 
> > > for now though, until we've reached a decision about whether to follow 
> > > existing practice for Advanced SIMD or whether to do what the spec says.
> > These do seem more "builtin" than the SIMD types, but I don't think it 
> > deeply matters either way, since these are already reserved names.
> Did you actually make the decision to use the `u` mangling?
Yeah, we decided to stick with the 'u' as documented for SVE.  (It's too late 
for Advanced SIMD unfortunately.)



Comment at: lib/AST/MicrosoftMangle.cpp:2073
+#define SVE_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
   case BuiltinType::ShortAccum:

rjmccall wrote:
> Comment isn't adding anything here.
I've removed these comments from everything except Type.h, where having them 
matches the surrounding style.



Comment at: lib/CodeGen/CGDebugInfo.cpp:709
+  case BuiltinType::Id: \
+return nullptr;
+#include "clang/Basic/AArch64SVEACLETypes.def"

rjmccall wrote:
> rsandifo-arm wrote:
> > rovka wrote:
> > > I don't really know this code, but I can't help but notice that nullptr 
> > > is only ever used for the void type. Is it safe to also use it for the 
> > > SVE types, or can we do something else instead?
> > Fixed to report an error here too and return a "safe" value until the TODO 
> > is fixed.  Also 

[PATCH] D65456: [OpenCL] Add generic type handling for builtin functions

[Pierre GONDOIS via Phabricator via cfe-commits]

Pierre added a comment.

Just one comment,
Thank you again for making everything clearer :)




Comment at: clang/lib/Sema/OpenCLBuiltins.td:116
+// combination of Types and vector sizes.
+//
+// E.g.: If TypeListField =  and VectorList = <1, 2, 4>, then

Maybe it would be worth adding more information on how to use GenTypes. I was 
thinking of something like this : 

When using multiple generic types :
  * The maximal cardinal of the used  GenTypes must be the PGCM of all the 
cardinals.
  * The generic types combine as if it was an array like GenType[Type][VecSize].
I.e: With GT1 = [half, <1, 2>] and GT2 = [float, int, <1, 2>], they will 
combine as
, , , 
The maximal cardinal of GT1 and GT2 is 4 (= 2 types * 2 vector sizes).
4 is the PGCM of GT1 (=2) and GT2 (=4).


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

https://reviews.llvm.org/D65456



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

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

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

balazske created this revision.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.

Code to import "ctor initializers" at import of functions
is moved to be after the flags in the newly created function
are imported. This fixes an error when the already created but
incomplete (flags are not set) function declaration is accessed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65935

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  clang/test/Analysis/ctu-main.cpp


Index: clang/test/Analysis/ctu-main.cpp
===
--- clang/test/Analysis/ctu-main.cpp
+++ clang/test/Analysis/ctu-main.cpp
@@ -125,6 +125,8 @@
   static const int Test;
 };
 
+extern int testImportOfDelegateConstructor(int);
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
@@ -158,4 +160,6 @@
   // clang_analyzer_eval(extSCC.a == 7); // TODO
   clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}}
   clang_analyzer_eval(TestAnonUnionUSR::Test == 5); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(testImportOfDelegateConstructor(10) == 10); // 
expected-warning{{TRUE}}
 }
Index: clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
===
--- clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
+++ clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
@@ -26,3 +26,4 @@
 c:@extSCC ctu-other.cpp.ast
 c:@extU ctu-other.cpp.ast
 c:@S@TestAnonUnionUSR@Test ctu-other.cpp.ast
+c:@F@testImportOfDelegateConstructor#I# ctu-other.cpp.ast
Index: clang/test/Analysis/Inputs/ctu-other.cpp
===
--- clang/test/Analysis/Inputs/ctu-other.cpp
+++ clang/test/Analysis/Inputs/ctu-other.cpp
@@ -145,3 +145,14 @@
   static const int Test;
 };
 const int TestAnonUnionUSR::Test = 5;
+
+class TestDelegateConstructor {
+public:
+  TestDelegateConstructor() : TestDelegateConstructor(2) {}
+  TestDelegateConstructor(int) {}
+};
+
+int testImportOfDelegateConstructor(int i) {
+  TestDelegateConstructor TDC;
+  return i;
+}
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3265,23 +3265,6 @@
 // decl and its redeclarations may be required.
   }
 
-  // Import Ctor initializers.
-  if (auto *FromConstructor = dyn_cast(D)) {
-if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
-  SmallVector CtorInitializers(NumInitializers);
-  // Import first, then allocate memory and copy if there was no error.
-  if (Error Err = ImportContainerChecked(
-  FromConstructor->inits(), CtorInitializers))
-return std::move(Err);
-  auto **Memory =
-  new (Importer.getToContext()) CXXCtorInitializer *[NumInitializers];
-  std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory);
-  auto *ToCtor = cast(ToFunction);
-  ToCtor->setCtorInitializers(Memory);
-  ToCtor->setNumCtorInitializers(NumInitializers);
-}
-  }
-
   ToFunction->setQualifierInfo(ToQualifierLoc);
   ToFunction->setAccess(D->getAccess());
   ToFunction->setLexicalDeclContext(LexicalDC);
@@ -3307,6 +3290,23 @@
 }
   }
 
+  // Import Ctor initializers.
+  if (auto *FromConstructor = dyn_cast(D)) {
+if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
+  SmallVector CtorInitializers(NumInitializers);
+  // Import first, then allocate memory and copy if there was no error.
+  if (Error Err = ImportContainerChecked(
+  FromConstructor->inits(), CtorInitializers))
+return std::move(Err);
+  auto **Memory =
+  new (Importer.getToContext()) CXXCtorInitializer *[NumInitializers];
+  std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory);
+  auto *ToCtor = cast(ToFunction);
+  ToCtor->setCtorInitializers(Memory);
+  ToCtor->setNumCtorInitializers(NumInitializers);
+}
+  }
+
   if (usedDifferentExceptionSpec) {
 // Update FunctionProtoType::ExtProtoInfo.
 if (ExpectedType TyOrErr = import(D->getType()))


Index: clang/test/Analysis/ctu-main.cpp
===
--- clang/test/Analysis/ctu-main.cpp
+++ clang/test/Analysis/ctu-main.cpp
@@ -125,6 +125,8 @@
   static const int Test;
 };
 
+extern int testImportOfDelegateConstructor(int);
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
@@ -158,4 +160,6 @@
   /

[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, jfb, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

The API provided by refactoring lib doesn't provide enough flexibility
to get clangd's rename to behave as we expect. Instead, we replace it
with the low-level rename functions, which give us more control.

Bonus:

- performance, previously we visit the TU to find all occurrences, now we just 
visit top-level decls from main file;
- fix a bug where we wrongly filter out the main file replacement due to the 
different relative/absolute file path;


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65936

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/test/rename.test

Index: clang-tools-extra/clangd/test/rename.test
===
--- clang-tools-extra/clangd/test/rename.test
+++ clang-tools-extra/clangd/test/rename.test
@@ -23,7 +23,7 @@
 {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32001,
-# CHECK-NEXT:"message": "there is no symbol at the given location"
+# CHECK-NEXT:"message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0"
@@ -31,7 +31,7 @@
 {"jsonrpc":"2.0","id":4,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32001,
-# CHECK-NEXT:"message": "there is no symbol at the given location"
+# CHECK-NEXT:"message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 4,
 # CHECK-NEXT:  "jsonrpc": "2.0"
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -10,45 +10,15 @@
 #include "AST.h"
 #include "Logger.h"
 #include "index/SymbolCollector.h"
-#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
+#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
-class RefactoringResultCollector final
-: public tooling::RefactoringResultConsumer {
-public:
-  void handleError(llvm::Error Err) override {
-assert(!Result.hasValue());
-Result = std::move(Err);
-  }
-
-  // Using the handle(SymbolOccurrences) from parent class.
-  using tooling::RefactoringResultConsumer::handle;
-
-  void handle(tooling::AtomicChanges SourceReplacements) override {
-assert(!Result.hasValue());
-Result = std::move(SourceReplacements);
-  }
-
-  llvm::Optional> Result;
-};
-
-// Expand a DiagnosticError to make it print-friendly (print the detailed
-// message, rather than "clang diagnostic").
-llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
-  if (auto Diag = DiagnosticError::take(Err)) {
-llvm::cantFail(std::move(Err));
-SmallVector DiagMessage;
-Diag->second.EmitToString(DE, DiagMessage);
-return llvm::make_error(DiagMessage,
-   llvm::inconvertibleErrorCode());
-  }
-  return Err;
-}
-
 llvm::Optional filePath(const SymbolLocation &Loc,
  llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -89,6 +59,7 @@
 }
 
 enum ReasonToReject {
+  NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile,
@@ -138,6 +109,8 @@
 llvm::Error makeError(ReasonToReject Reason) {
   auto Message = [](ReasonToReject Reason) {
 switch (Reason) {
+case NoSymbolFound:
+  return "there is no symbol at the given location";
 case NoIndexProvided:
   return "symbol may be used in other files (no index available)";
 case UsedOutsideFile:
@@ -154,50 +127,57 @@
   llvm::inconvertibleErrorCode());
 }
 
+// Return all rename occurrences in the main file.
+tooling::SymbolOccurrences
+findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) {
+  const NamedDecl *CanonicalRenameDecl =
+  tooling::getCanonicalSymbolDeclaration(RenameDecl);
+  assert(CanonicalRenameDecl && "RenameDecl must be not null");
+  auto RenameUSRs =
+  tooling::getUSRsForDeclaration(CanonicalRenameDecl, AST.getASTContext());
+  std::string OldName = CanonicalRenameDecl->getNameAsString();
+  tooling::SymbolOccurrences Result;
+  for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
+auto RenameInDecl =
+tooling::getOccurrencesOfUSRs(Ren

[clang-tools-extra] r368277 - [clangd] Correct the documentation, NFC.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Aug  8 03:58:16 2019
New Revision: 368277

URL: http://llvm.org/viewvc/llvm-project?rev=368277&view=rev
Log:
[clangd] Correct the documentation, NFC.

Modified:
clang-tools-extra/trunk/clangd/unittests/TweakTesting.h

Modified: clang-tools-extra/trunk/clangd/unittests/TweakTesting.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/TweakTesting.h?rev=368277&r1=368276&r2=368277&view=diff
==
--- clang-tools-extra/trunk/clangd/unittests/TweakTesting.h (original)
+++ clang-tools-extra/trunk/clangd/unittests/TweakTesting.h Thu Aug  8 03:58:16 
2019
@@ -26,7 +26,7 @@ namespace clangd {
 // namespace foo { template class X{}; }
 // using namespace foo;
 //   cpp)";
-//   Context = Block;
+//   Context = Function;
 //   EXPECT_THAT(apply("[[auto]] X = foo();"),
 //   "foo X = foo();");


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

[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:141
+  for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
+auto RenameInDecl =
+tooling::getOccurrencesOfUSRs(RenameUSRs, OldName, TopLevelDecl);

nit: most of the new uses of `auto` in this patch don't have a type that's 
obvious from the RHS, nor one that's hard to read, and should probably be 
expanded



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+// Currently, we only support normal rename (one range) for C/C++.
+// FIXME: support multiple-range rename for objective-c methods.
+if (Rename.getNameRanges().size() > 1)

is this a regression in this patch, or did the limitation already exist?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:178
+  continue;
+cantFail(FilteredChanges.add(tooling::Replacement(
+AST.getASTContext().getSourceManager(),

why can't this fail?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936



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

[PATCH] D65938: [AST] No longer visiting CXXMethodDecl bodies created by compiler when method was default created.

[Johan Vikström via Phabricator via cfe-commits]

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

Clang generates function bodies and puts them in the AST for default methods if 
it is defaulted outside the class definition.

`
struct A {

  A &operator=(A &&O);

};

A &A::operator=(A &&O) = default;
`

This will generate a function body for the `A &A::operator=(A &&O)` and put it 
in the AST. This body should not be visited if implicit code is not visited as 
it is implicit.

This was causing SemanticHighlighting in clangd to generate duplicate tokens 
and putting them in weird places.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65938

Files:
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/RecursiveASTVisitorTests/CXXMethodDecl.cpp

Index: clang/unittests/Tooling/RecursiveASTVisitorTests/CXXMethodDecl.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/CXXMethodDecl.cpp
@@ -0,0 +1,58 @@
+//=-- unittest/Tooling/RecursiveASTVisitorTests/CXXMethodDecl.cpp --=//
+//
+// 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 "TestVisitor.h"
+#include "clang/AST/Expr.h"
+
+using namespace clang;
+
+namespace {
+
+class CXXMethodDeclVisitor
+: public ExpectedLocationVisitor {
+public:
+  CXXMethodDeclVisitor(bool VisitImplicitCode)
+  : VisitImplicitCode(VisitImplicitCode) {}
+
+  bool shouldVisitImplicitCode() const { return VisitImplicitCode; }
+
+  bool VisitDeclRefExpr(DeclRefExpr *D) {
+Match("declref", D->getLocation());
+return true;
+  }
+  bool VisitParmVarDecl(ParmVarDecl *P) {
+Match("parm", P->getLocation());
+return true;
+  }
+
+private:
+  bool VisitImplicitCode;
+};
+
+TEST(RecursiveASTVisitor, CXXMethodDeclNoDefaultBodyVisited) {
+  for (bool VisitImplCode : {false, true}) {
+CXXMethodDeclVisitor Visitor(VisitImplCode);
+if (VisitImplCode)
+  Visitor.ExpectMatch("declref", 8, 28);
+else
+  Visitor.DisallowMatch("declref", 8, 28);
+
+Visitor.ExpectMatch("parm", 8, 27);
+llvm::StringRef Code = R"cpp(
+  struct B {};
+  struct A {
+B BB;
+A &operator=(A &&O);
+  };
+
+  A &A::operator=(A &&O) = default;
+)cpp";
+EXPECT_TRUE(Visitor.runOver(Code, CXXMethodDeclVisitor::Lang_CXX11));
+  }
+}
+} // end anonymous namespace
Index: clang/unittests/Tooling/CMakeLists.txt
===
--- clang/unittests/Tooling/CMakeLists.txt
+++ clang/unittests/Tooling/CMakeLists.txt
@@ -28,6 +28,7 @@
   RecursiveASTVisitorTests/ConstructExpr.cpp
   RecursiveASTVisitorTests/CXXBoolLiteralExpr.cpp
   RecursiveASTVisitorTests/CXXMemberCall.cpp
+  RecursiveASTVisitorTests/CXXMethodDecl.cpp
   RecursiveASTVisitorTests/CXXOperatorCallExprTraverser.cpp
   RecursiveASTVisitorTests/DeclRefExpr.cpp
   RecursiveASTVisitorTests/ImplicitCtor.cpp
Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2027,7 +2027,13 @@
 }
   }
 
-  if (D->isThisDeclarationADefinition()) {
+  bool VisitBody = D->isThisDeclarationADefinition();
+  // If a method is set to default outside the class definition the compiler
+  // generates the method body and adds it to the AST.
+  if (const CXXMethodDecl *MD = dyn_cast(D))
+VisitBody &= !MD->isDefaulted() || getDerived().shouldVisitImplicitCode();
+
+  if (VisitBody) {
 TRY_TO(TraverseStmt(D->getBody())); // Function body.
   }
   return true;
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -54,7 +54,7 @@
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   std::vector ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test)) << Code;
 }
 
 // Any annotations in OldCode and NewCode are converted into their corresponding
@@ -255,6 +255,15 @@
   struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
   extern template struct $Class[[Tmpl]];
   template struct $Class[[Tmpl]];
+)cpp",
+R"cpp(
+  struct $Class[[B

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 214110.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.

Changed to do an isa check instead of checking if MemberLoc is 
invalid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928

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


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,11 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying
+  // to add a token with an invalid range.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,11 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying
+  // to add a token with an invalid range.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: Add llvm-prefer-register-over-unsigned to clang-tidy and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders created this revision.
dsanders added reviewers: arsenm, bogner.
Herald added subscribers: cfe-commits, s.egerton, Jim, asbirlea, 
Petar.Avramovic, jsji, jocewei, PkmX, tpr, the_o, brucehoult, MartinMosbeck, 
rogfer01, atanasyan, edward-jones, zzheng, MaskRay, jrtc27, niosHD, sabuasal, 
apazos, simoncook, johnrusso, rbar, asb, javed.absar, fedor.sergeev, kbarton, 
aheejin, hiraditya, kristof.beyls, jgravelle-google, sbc100, mgorny, nhaehnle, 
wdng, jvesely, nemanjai, sdardis, dylanmckay, jyknight, dschuff, qcolombet, 
MatzeB, jholewinski.
Herald added projects: clang, LLVM.

This clang-tidy check is looking for unsigned integer variables whose 
initializer
starts with an implicit cast from llvm::Register and changes the type of the
variable to llvm::Register (dropping the llvm:: where possible).

Partial reverts in:
X86FrameLowering.cpp - Some functions return unsigned and arguably should be 
MCRegister
X86FixupLEAs.cpp - Some functions return unsigned and arguably should be 
MCRegister
X86FrameLowering.cpp - Some functions return unsigned and arguably should be 
MCRegister
HexagonBitSimplify.cpp - Function takes BitTracker::RegisterRef which appears 
to be unsigned&
MachineVerifier.cpp - Ambiguous operator==() given MCRegister and const Register
PPCFastISel.cpp - No Register::operator-=()
PeepholeOptimizer.cpp - TargetInstrInfo::optimizeLoadInstr() takes an unsigned&
MachineTraceMetrics.cpp - MachineTraceMetrics lacks a suitable constructor

Manual fixups in:
AArch64InstrInfo.cpp - genFusedMultiply() now takes a Register* instead of 
unsigned*
ARMFastISel.cpp - ARMEmitLoad() now takes a Register& instead of unsigned&
AArch64LoadStoreOptimizer.cpp - Ternary operator was ambiguous between 
Register/MCRegister. Settled on Register
HexagonSplitDouble.cpp - Ternary operator was ambiguous between 
unsigned/Register
HexagonConstExtenders.cpp - Has a local class named Register, used 
llvm::Register instead of Register.
PPCFastISel.cpp - PPCEmitLoad() now takes a Register& instead of unsigned&


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/lib/CodeGen/BranchFolding.cpp
  llvm/lib/CodeGen/BreakFalseDeps.cpp
  llvm/lib/CodeGen/CalcSpillWeights.cpp
  llvm/lib/CodeGen/CriticalAntiDepBreaker.cpp
  llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
  llvm/lib/CodeGen/DetectDeadLanes.cpp
  llvm/lib/CodeGen/EarlyIfConversion.cpp
  llvm/lib/CodeGen/ExpandPostRAPseudos.cpp
  llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
  llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
  llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
  llvm/lib/CodeGen/GlobalISel/Localizer.cpp
  llvm/lib/CodeGen/GlobalISel/Utils.cpp
  llvm/lib/CodeGen/IfConversion.cpp
  llvm/lib/CodeGen/ImplicitNullChecks.cpp
  llvm/lib/CodeGen/InlineSpiller.cpp
  llvm/lib/CodeGen/LiveDebugValues.cpp
  llvm/lib/CodeGen/LiveDebugVariables.cpp
  llvm/lib/CodeGen/LiveIntervals.cpp
  llvm/lib/CodeGen/LivePhysRegs.cpp
  llvm/lib/CodeGen/LiveRangeEdit.cpp
  llvm/lib/CodeGen/LiveRangeShrink.cpp
  llvm/lib/CodeGen/LiveRegMatrix.cpp
  llvm/lib/CodeGen/LiveRegUnits.cpp
  llvm/lib/CodeGen/LiveVariables.cpp
  llvm/lib/CodeGen/MIRCanonicalizerPass.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineCSE.cpp
  llvm/lib/CodeGen/MachineCopyPropagation.cpp
  llvm/lib/CodeGen/MachineInstrBundle.cpp
  llvm/lib/CodeGen/MachineLICM.cpp
  llvm/lib/CodeGen/MachineOperand.cpp
  llvm/lib/CodeGen/MachinePipeliner.cpp
  llvm/lib/CodeGen/MachineSSAUpdater.cpp
  llvm/lib/CodeGen/MachineScheduler.cpp
  llvm/lib/CodeGen/MachineSink.cpp
  llvm/lib/CodeGen/MachineTraceMetrics.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/OptimizePHIs.cpp
  llvm/lib/CodeGen/PHIElimination.cpp
  llvm/lib/CodeGen/PeepholeOptimizer.cpp
  llvm/lib/CodeGen/ProcessImplicitDefs.cpp
  llvm/lib/CodeGen/RegAllocFast.cpp
  llvm/lib/CodeGen/RegAllocGreedy.cpp
  llvm/lib/CodeGen/RegisterCoalescer.cpp
  llvm/lib/CodeGen/RegisterPressure.cpp
  llvm/lib/CodeGen/RegisterScavenging.cpp
  llvm/lib/CodeGen/RenameIndependentSubregs.cpp
  llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
  llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
  llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

[PATCH] D65634: [RISCV] Default to ilp32d/lp64d in RISC-V Linux

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D65634#1618443 , @rogfer01 wrote:

> Thanks @asb @lenary for the review!
>
> I understand that, after this change, we will also want to make 
> `-march=rv{32,64}gc` the default in Linux as well. Otherwise there will be an 
> ABI mismatch with the default `-march=rv{32.64}i` in a default invocation.
>
> Does this make sense?


Yes, that makes sense to me.


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

https://reviews.llvm.org/D65634



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

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56
+if (isa(MD))
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying

Maybe just `The MemberLoc is invalid for C++ conversion operato , we don't 
attempt to add a token for an invalid location`?


Does the location is always invalid? or just for builtin types? e.g.
```
class Foo {};
struct Bar {
  explicit operator Foo*() const; // 1
  explicit operator int() const; // 2
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928



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

[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 214112.
hokein marked 4 inline comments as done.
hokein added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/test/rename.test

Index: clang-tools-extra/clangd/test/rename.test
===
--- clang-tools-extra/clangd/test/rename.test
+++ clang-tools-extra/clangd/test/rename.test
@@ -23,7 +23,7 @@
 {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32001,
-# CHECK-NEXT:"message": "there is no symbol at the given location"
+# CHECK-NEXT:"message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0"
@@ -31,7 +31,7 @@
 {"jsonrpc":"2.0","id":4,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32001,
-# CHECK-NEXT:"message": "there is no symbol at the given location"
+# CHECK-NEXT:"message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 4,
 # CHECK-NEXT:  "jsonrpc": "2.0"
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -10,45 +10,15 @@
 #include "AST.h"
 #include "Logger.h"
 #include "index/SymbolCollector.h"
-#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
+#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
-class RefactoringResultCollector final
-: public tooling::RefactoringResultConsumer {
-public:
-  void handleError(llvm::Error Err) override {
-assert(!Result.hasValue());
-Result = std::move(Err);
-  }
-
-  // Using the handle(SymbolOccurrences) from parent class.
-  using tooling::RefactoringResultConsumer::handle;
-
-  void handle(tooling::AtomicChanges SourceReplacements) override {
-assert(!Result.hasValue());
-Result = std::move(SourceReplacements);
-  }
-
-  llvm::Optional> Result;
-};
-
-// Expand a DiagnosticError to make it print-friendly (print the detailed
-// message, rather than "clang diagnostic").
-llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
-  if (auto Diag = DiagnosticError::take(Err)) {
-llvm::cantFail(std::move(Err));
-SmallVector DiagMessage;
-Diag->second.EmitToString(DE, DiagMessage);
-return llvm::make_error(DiagMessage,
-   llvm::inconvertibleErrorCode());
-  }
-  return Err;
-}
-
 llvm::Optional filePath(const SymbolLocation &Loc,
  llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -89,6 +59,7 @@
 }
 
 enum ReasonToReject {
+  NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile,
@@ -138,6 +109,8 @@
 llvm::Error makeError(ReasonToReject Reason) {
   auto Message = [](ReasonToReject Reason) {
 switch (Reason) {
+case NoSymbolFound:
+  return "there is no symbol at the given location";
 case NoIndexProvided:
   return "symbol may be used in other files (no index available)";
 case UsedOutsideFile:
@@ -154,50 +127,59 @@
   llvm::inconvertibleErrorCode());
 }
 
+// Return all rename occurrences in the main file.
+tooling::SymbolOccurrences
+findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) {
+  const NamedDecl *CanonicalRenameDecl =
+  tooling::getCanonicalSymbolDeclaration(RenameDecl);
+  assert(CanonicalRenameDecl && "RenameDecl must be not null");
+  std::vector RenameUSRs =
+  tooling::getUSRsForDeclaration(CanonicalRenameDecl, AST.getASTContext());
+  std::string OldName = CanonicalRenameDecl->getNameAsString();
+  tooling::SymbolOccurrences Result;
+  for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
+tooling::SymbolOccurrences RenameInDecl =
+tooling::getOccurrencesOfUSRs(RenameUSRs, OldName, TopLevelDecl);
+Result.insert(Result.end(), std::make_move_iterator(RenameInDecl.begin()),
+  std::make_move_iterator(RenameInDecl.end()));
+  }
+  return Result;
+}
+
 } // namespace
 
 llvm::Expected
 renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
  llvm::StringRef NewName, const SymbolIndex *Index) {
-  RefactoringResultCollecto

[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+// Currently, we only support normal rename (one range) for C/C++.
+// FIXME: support multiple-range rename for objective-c methods.
+if (Rename.getNameRanges().size() > 1)

sammccall wrote:
> is this a regression in this patch, or did the limitation already exist?
this is not a regression, this is a limitation in clangd.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:178
+  continue;
+cantFail(FilteredChanges.add(tooling::Replacement(
+AST.getASTContext().getSourceManager(),

sammccall wrote:
> why can't this fail?
I just kept the previous behavior. Made the error emit to the client rather 
than crashing here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936



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

[PATCH] D65940: [clang-format] fix crash involving invalid preprocessor line

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This (invalid) fragment is crashing clang-format:

  #if 1
  int x;
  #elif
  int y;
  #endif

The reason being that the parser expects a token after `#elif`, and the
subsequent parsing of the next line does not check if `CurrentToken` is null.


Repository:
  rC Clang

https://reviews.llvm.org/D65940

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3087,6 +3087,15 @@
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is nothing following #elif.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1099,6 +1099,8 @@
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3087,6 +3087,15 @@
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is nothing following #elif.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1099,6 +1099,8 @@
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65940: [clang-format] fix crash involving invalid preprocessor line

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: unittests/Format/FormatTest.cpp:3090
Style);
+  // Don't crash if there is nothing following #elif.
+  verifyFormat("#if 1\n"

`// #elif directive without a condition.` -- to follow the existing comment 
format in other tests.



Repository:
  rC Clang

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

https://reviews.llvm.org/D65940



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

[PATCH] D65941: [OpenCL] Fix lang mode predefined macros for C++ mode

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: svenvh.
Herald added subscribers: ebevhan, yaxunl.

We should only avoid adding `__OPENCL_C_VERSION__`, all other predefined macros 
about the language mode are still valid.

This change also fixes the language version check in the headers.


https://reviews.llvm.org/D65941

Files:
  lib/Frontend/InitPreprocessor.cpp
  lib/Headers/opencl-c-base.h
  lib/Headers/opencl-c.h

Index: lib/Headers/opencl-c.h
===
--- lib/Headers/opencl-c.h
+++ lib/Headers/opencl-c.h
@@ -11,11 +11,11 @@
 
 #include "opencl-c-base.h"
 
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 #ifndef cl_khr_depth_images
 #define cl_khr_depth_images
 #endif //cl_khr_depth_images
-#endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#endif //defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 #if __OPENCL_C_VERSION__ < CL_VERSION_2_0
 #ifdef cl_khr_3d_image_writes
@@ -23,10 +23,10 @@
 #endif //cl_khr_3d_image_writes
 #endif //__OPENCL_C_VERSION__ < CL_VERSION_2_0
 
-#if __OPENCL_C_VERSION__ >= CL_VERSION_1_2
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_1_2)
 #pragma OPENCL EXTENSION cl_intel_planar_yuv : begin
 #pragma OPENCL EXTENSION cl_intel_planar_yuv : end
-#endif // __OPENCL_C_VERSION__ >= CL_VERSION_1_2
+#endif // defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_1_2)
 
 #define __ovld __attribute__((overloadable))
 #define __conv __attribute__((convergent))
@@ -6517,11 +6517,11 @@
  */
 size_t __ovld __cnfn get_global_offset(uint dimindx);
 
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 size_t __ovld get_enqueued_local_size(uint dimindx);
 size_t __ovld get_global_linear_id(void);
 size_t __ovld get_local_linear_id(void);
-#endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#endif //defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 // OpenCL v1.1 s6.11.2, v1.2 s6.12.2, v2.0 s6.13.2 - Math functions
 
@@ -7352,7 +7352,7 @@
  * Returns fmin(x - floor (x), 0x1.fep-1f ).
  * floor(x) is returned in iptr.
  */
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 float __ovld fract(float x, float *iptr);
 float2 __ovld fract(float2 x, float2 *iptr);
 float3 __ovld fract(float3 x, float3 *iptr);
@@ -7434,7 +7434,7 @@
 half8 __ovld fract(half8 x, __private half8 *iptr);
 half16 __ovld fract(half16 x, __private half16 *iptr);
 #endif //cl_khr_fp16
-#endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#endif //defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 /**
  * Extract mantissa and exponent from x. For each
@@ -7442,7 +7442,7 @@
  * magnitude in the interval [1/2, 1) or 0. Each
  * component of x equals mantissa returned * 2^exp.
  */
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 float __ovld frexp(float x, int *exp);
 float2 __ovld frexp(float2 x, int2 *exp);
 float3 __ovld frexp(float3 x, int3 *exp);
@@ -7524,7 +7524,7 @@
 half8 __ovld frexp(half8 x, __private int8 *exp);
 half16 __ovld frexp(half16 x, __private int16 *exp);
 #endif //cl_khr_fp16
-#endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#endif //defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 /**
  * Compute the value of the square root of x^2 + y^2
@@ -7649,7 +7649,7 @@
 half16 __ovld __cnfn lgamma(half16 x);
 #endif //cl_khr_fp16
 
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 float __ovld lgamma_r(float x, int *signp);
 float2 __ovld lgamma_r(float2 x, int2 *signp);
 float3 __ovld lgamma_r(float3 x, int3 *signp);
@@ -7731,7 +7731,7 @@
 half8 __ovld lgamma_r(half8 x, __private int8 *signp);
 half16 __ovld lgamma_r(half16 x, __private int16 *signp);
 #endif //cl_khr_fp16
-#endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#endif //defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 /**
  * Compute natural logarithm.
@@ -7955,7 +7955,7 @@
  * the argument. It stores the integral part in the object
  * pointed to by iptr.
  */
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 float __ovld modf(float x, float *iptr);
 float2 __ovld modf(float2 x, float2 *iptr);
 float3 __ovld modf(float3 x, float3 *iptr);
@@ -8037,7 +8037,7 @@
 half8 __ovld modf(half8 x, __private half8 *iptr);
 half16 __ovld modf(half16 x, __private half16 *iptr);
 #endif //cl_khr_fp16
-#endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#endif //defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 
 /**
  * Returns a quiet NaN. T

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> hfinkel wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > > > Telling the compiler `ps` is a device pointer only to 
> > > > > > > > > > create a local uninitialized shadowing variable seems like 
> > > > > > > > > > an error to me.
> > > > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > > > created in the context of the parallel region, not the target 
> > > > > > > > > region. So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > It means, that for OpenMP 5.0 we should emit a warning/error 
> > > > > > > here. It is allowed according to the standard, we should allow it 
> > > > > > > too.
> > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > 
> > > > > > The last answer contradicts what you said earlier. I expect there 
> > > > > > is a *not* missing, correct?
> > > > > > 
> > > > > > 
> > > > > > Assuming you do not want an error, which is fine, I still think a 
> > > > > > warning is appropriate as it seems to me there is never a reason to 
> > > > > > have a `is_device_ptr` clause for a private value. I mean, it is 
> > > > > > either a bug by the programmer, e.g., 5 letters of `firstprivate` 
> > > > > > went missing, or simply nonsensical code for which we warn in other 
> > > > > > situations as well.
> > > > > Missed `not`.
> > > > > These kind of construct are explicitly allowed in OpenMP. And we 
> > > > > should obey the standard unconditionally.
> > > > > Plus, there might be situations where user may require it explicitly. 
> > > > > For example, the device pointer is dereferenced in one of the clauses 
> > > > > for the subregions but in the deeper subregion it might be used as a 
> > > > > private pointer. Why we should emit a warning here?
> > > > If you have a different situation, e.g., the one you describe, you 
> > > > should not have a warning. Though, that is not the point. If you have 
> > > > the situation above (single directive), as per my reasoning, there 
> > > > doesn't seem to be a sensible use case. If you have one and we should 
> > > > create an explicit test for it.
> > > > 
> > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > And we should obey the standard unconditionally.
> > > > Nobody says we should not. We warn people all the time even if it is 
> > > > valid code.
> > > Warnings may prevent successful compilation in some cases, e.g. when 
> > > warnings are treated as errors. Why we should emit a warning if the 
> > > construct is allowed by the standard? Ask to change the standard if you 
> > > did not agree with it.
> > Warnings are specifically for constructs which are legal, but likely wrong 
> > (i.e., will not do what the user likely intended). Treating warnings as 
> > errors is not a conforming compilation mode - by design (specifically 
> > because that will reject conforming programs). Thus...
> > 
> > > Why we should emit a warning if the construct is allowed by the standard? 
> > > Ask to change the standard if you did not agree with it.
> > 
> > This is the wrong way to approach this. Warnings are specifically for legal 
> > code. They help users prevent errors, however, in cases where that legal 
> > code is likely problematic or won't do what the user intends.
> > 
> Ok, we could emit wqrnings in some cases. But better to do it in the separate 
> patches. Each particular case requires additional analysis.
> 
> > This is the wrong way to approach this.
> 
> I don't think so. If some cases are really meaningless, better to ask to 
> prohibit them in the standard. It is always a good idea to change the 
> requirements first, if you think that some scenarios are not described 
> correctly rather than do the changes in the code. It leads to different 
> behavior of different compilers in the same situation and it is not good for 
> the users.
> I don't think so. If some cases are really meaningless, better to ask to 
> prohibit them in the standard. It is always a good idea to change the 
> requirements first, if you think

[PATCH] D65940: [clang-format] fix crash involving invalid preprocessor line

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir updated this revision to Diff 214115.
krasimir marked an inline comment as done.
krasimir added a comment.

- Apply review comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D65940

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3087,6 +3087,15 @@
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is an #elif directive without a condition.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1099,6 +1099,8 @@
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3087,6 +3087,15 @@
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is an #elif directive without a condition.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1099,6 +1099,8 @@
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65940: [clang-format] fix crash involving invalid preprocessor line

[Krasimir Georgiev via Phabricator via cfe-commits]

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



Comment at: unittests/Format/FormatTest.cpp:3090
Style);
+  // Don't crash if there is nothing following #elif.
+  verifyFormat("#if 1\n"

gribozavr wrote:
> `// #elif directive without a condition.` -- to follow the existing comment 
> format in other tests.
> 
done


Repository:
  rC Clang

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

https://reviews.llvm.org/D65940



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

r368280 - [clang-format] fix crash involving invalid preprocessor line

[Krasimir Georgiev via cfe-commits]

Author: krasimir
Date: Thu Aug  8 04:56:18 2019
New Revision: 368280

URL: http://llvm.org/viewvc/llvm-project?rev=368280&view=rev
Log:
[clang-format] fix crash involving invalid preprocessor line

Summary:
This (invalid) fragment is crashing clang-format:
```
#if 1
int x;
#elif
int y;
#endif
```

The reason being that the parser expects a token after `#elif`, and the
subsequent parsing of the next line does not check if `CurrentToken` is null.

Reviewers: gribozavr

Reviewed By: gribozavr

Subscribers: cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/unittests/Format/FormatTest.cpp

Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=368280&r1=368279&r2=368280&view=diff
==
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Thu Aug  8 04:56:18 2019
@@ -1099,6 +1099,8 @@ private:
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();

Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=368280&r1=368279&r2=368280&view=diff
==
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Aug  8 04:56:18 2019
@@ -3087,6 +3087,15 @@ TEST_F(FormatTest, IndentPreprocessorDir
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is an #elif directive without a condition.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the


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

[PATCH] D65940: [clang-format] fix crash involving invalid preprocessor line

[Krasimir Georgiev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368280: [clang-format] fix crash involving invalid 
preprocessor line (authored by krasimir, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65940

Files:
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -1099,6 +1099,8 @@
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -3087,6 +3087,15 @@
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is an #elif directive without a condition.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -1099,6 +1099,8 @@
 
 public:
   LineType parseLine() {
+if (!CurrentToken)
+  return LT_Invalid;
 NonTemplateLess.clear();
 if (CurrentToken->is(tok::hash))
   return parsePreprocessorDirective();
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -3087,6 +3087,15 @@
"#endif\n"
"#endif\n",
Style);
+  // Don't crash if there is an #elif directive without a condition.
+  verifyFormat("#if 1\n"
+   "int x;\n"
+   "#elif\n"
+   "int y;\n"
+   "#else\n"
+   "int z;\n"
+   "#endif",
+   Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 214117.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.

Added test for conversion operators. Also changed comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,20 @@
   struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
   extern template struct $Class[[Tmpl]];
   template struct $Class[[Tmpl]];
+)cpp",
+R"cpp(
+  class $Class[[Foo]] {};
+  struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+  };
+  void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // The MemberLoc is invalid for C++ conversion operators. We do not
+  // attempt to add tokens with invalid locations.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,20 @@
   struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
   extern template struct $Class[[Tmpl]];
   template struct $Class[[Tmpl]];
+)cpp",
+R"cpp(
+  class $Class[[Foo]] {};
+  struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+  };
+  void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // The MemberLoc is invalid for C++ conversion operators. We do not
+  // attempt to add tokens with invalid locations.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Johan Vikström via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56
+if (isa(MD))
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying

hokein wrote:
> Maybe just `The MemberLoc is invalid for C++ conversion operato , we don't 
> attempt to add a token for an invalid location`?
> 
> 
> Does the location is always invalid? or just for builtin types? e.g.
> ```
> class Foo {};
> struct Bar {
>   explicit operator Foo*() const; // 1
>   explicit operator int() const; // 2
> };
> ```
Builtin types has nothing to do with it. It's invalid for every conversion 
operator. Will actually add a test just to make sure that everything else is 
still highlighted correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928



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

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[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/SemanticHighlighting.cpp:56
+if (isa(MD))
+  // If the member is a conversion operator the loc will be invalid. This
+  // causes the addToken function to emit a lot of error logs about trying

jvikstrom wrote:
> hokein wrote:
> > Maybe just `The MemberLoc is invalid for C++ conversion operato , we don't 
> > attempt to add a token for an invalid location`?
> > 
> > 
> > Does the location is always invalid? or just for builtin types? e.g.
> > ```
> > class Foo {};
> > struct Bar {
> >   explicit operator Foo*() const; // 1
> >   explicit operator int() const; // 2
> > };
> > ```
> Builtin types has nothing to do with it. It's invalid for every conversion 
> operator. Will actually add a test just to make sure that everything else is 
> still highlighted correctly.
ah, I thought it is the operator declaration, but here we mean expression.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:268
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];

nit: could you add a comment describing the purpose of this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928



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

[PATCH] D65941: [OpenCL] Fix lang mode predefined macros for C++ mode

[Sven van Haastregt via Phabricator via cfe-commits]

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

LGTM!


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

https://reviews.llvm.org/D65941



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

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

please update the patch description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65928



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

[clang-tools-extra] r368282 - [clangd] Remove a function accidently being added in rL368261.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Aug  8 05:19:01 2019
New Revision: 368282

URL: http://llvm.org/viewvc/llvm-project?rev=368282&view=rev
Log:
[clangd] Remove a function accidently being added in rL368261.

Modified:
clang-tools-extra/trunk/clangd/AST.cpp

Modified: clang-tools-extra/trunk/clangd/AST.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/AST.cpp?rev=368282&r1=368281&r2=368282&view=diff
==
--- clang-tools-extra/trunk/clangd/AST.cpp (original)
+++ clang-tools-extra/trunk/clangd/AST.cpp Thu Aug  8 05:19:01 2019
@@ -73,26 +73,6 @@ bool isImplementationDetail(const Decl *
 D->getASTContext().getSourceManager());
 }
 
-// Returns true if the complete name of decl \p D is spelled in the source 
code.
-// This is not the case for:
-//   * symbols formed via macro concatenation, the spelling location will
-// be ""
-//   * symbols controlled and defined by a compile command-line option
-// `-DName=foo`, the spelling location will be "".
-bool isSpelledInSourceCode(const Decl *D) {
-  const auto &SM = D->getASTContext().getSourceManager();
-  auto Loc = D->getLocation();
-  // FIXME: Revisit the strategy, the heuristic is limitted when handling
-  // macros, we should use the location where the whole definition occurs.
-  if (Loc.isMacroID()) {
-std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
-if (llvm::StringRef(PrintLoc).startswith(""))
-  return false;
-  }
-  return true;
-}
-
 SourceLocation findName(const clang::Decl *D) {
   return D->getLocation();
 }


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

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

hfinkel wrote:
> ABataev wrote:
> > hfinkel wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > > > > Telling the compiler `ps` is a device pointer only to 
> > > > > > > > > > > create a local uninitialized shadowing variable seems 
> > > > > > > > > > > like an error to me.
> > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > > > > created in the context of the parallel region, not the 
> > > > > > > > > > target region. So, for OpenMP 5.0 we should not emit error 
> > > > > > > > > > here.
> > > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > > It means, that for OpenMP 5.0 we should emit a warning/error 
> > > > > > > > here. It is allowed according to the standard, we should allow 
> > > > > > > > it too.
> > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > 
> > > > > > > The last answer contradicts what you said earlier. I expect there 
> > > > > > > is a *not* missing, correct?
> > > > > > > 
> > > > > > > 
> > > > > > > Assuming you do not want an error, which is fine, I still think a 
> > > > > > > warning is appropriate as it seems to me there is never a reason 
> > > > > > > to have a `is_device_ptr` clause for a private value. I mean, it 
> > > > > > > is either a bug by the programmer, e.g., 5 letters of 
> > > > > > > `firstprivate` went missing, or simply nonsensical code for which 
> > > > > > > we warn in other situations as well.
> > > > > > Missed `not`.
> > > > > > These kind of construct are explicitly allowed in OpenMP. And we 
> > > > > > should obey the standard unconditionally.
> > > > > > Plus, there might be situations where user may require it 
> > > > > > explicitly. For example, the device pointer is dereferenced in one 
> > > > > > of the clauses for the subregions but in the deeper subregion it 
> > > > > > might be used as a private pointer. Why we should emit a warning 
> > > > > > here?
> > > > > If you have a different situation, e.g., the one you describe, you 
> > > > > should not have a warning. Though, that is not the point. If you have 
> > > > > the situation above (single directive), as per my reasoning, there 
> > > > > doesn't seem to be a sensible use case. If you have one and we should 
> > > > > create an explicit test for it.
> > > > > 
> > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > And we should obey the standard unconditionally.
> > > > > Nobody says we should not. We warn people all the time even if it is 
> > > > > valid code.
> > > > Warnings may prevent successful compilation in some cases, e.g. when 
> > > > warnings are treated as errors. Why we should emit a warning if the 
> > > > construct is allowed by the standard? Ask to change the standard if you 
> > > > did not agree with it.
> > > Warnings are specifically for constructs which are legal, but likely 
> > > wrong (i.e., will not do what the user likely intended). Treating 
> > > warnings as errors is not a conforming compilation mode - by design 
> > > (specifically because that will reject conforming programs). Thus...
> > > 
> > > > Why we should emit a warning if the construct is allowed by the 
> > > > standard? Ask to change the standard if you did not agree with it.
> > > 
> > > This is the wrong way to approach this. Warnings are specifically for 
> > > legal code. They help users prevent errors, however, in cases where that 
> > > legal code is likely problematic or won't do what the user intends.
> > > 
> > Ok, we could emit wqrnings in some cases. But better to do it in the 
> > separate patches. Each particular case requires additional analysis.
> > 
> > > This is the wrong way to approach this.
> > 
> > I don't think so. If some cases are really meaningless, better to ask to 
> > prohibit them in the standard. It is always a good idea to change the 
> > requirements first, if you think that some scenarios are not described 
> > correctly rather than do the changes in the code. It leads to different 
> > behavior of different compilers in the same situation and it is not g

[PATCH] D65926: [clangd] Fixed printTemplateSpecializationArgs not printing partial variable specialization arguments.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:40
 
+TEST(PrintTemplateSpecializationArgs, PrintsTemplateArgs) {
+  TestTU TU;

looks like the related tests are in `PrintASTTests.cpp`, could you move the 
test there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65926



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

[clang-tools-extra] r368283 - [clangd] Don't include internal gtest header.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Aug  8 05:33:12 2019
New Revision: 368283

URL: http://llvm.org/viewvc/llvm-project?rev=368283&view=rev
Log:
[clangd] Don't include internal gtest header.

Modified:
clang-tools-extra/trunk/clangd/unittests/PrintASTTests.cpp

Modified: clang-tools-extra/trunk/clangd/unittests/PrintASTTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/PrintASTTests.cpp?rev=368283&r1=368282&r2=368283&view=diff
==
--- clang-tools-extra/trunk/clangd/unittests/PrintASTTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/PrintASTTests.cpp Thu Aug  8 
05:33:12 2019
@@ -15,7 +15,6 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest-param-test.h"
 #include "gtest/gtest.h"
-#include "gtest/internal/gtest-param-util-generated.h"
 
 namespace clang {
 namespace clangd {


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

[PATCH] D65943: [clangd] Added semantic highlighting support for primitives.

[Johan Vikström via Phabricator via cfe-commits]

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

Adds a new HighlightingKind "Primitive". Adds a special case for TypeLocs that 
have an underlying TypePtr that is are builtin types, adding them as primitives.
The primary reason for this change is because otherwise typedefs that typedef 
primitives `typedef int A` would not get highlighted (so in the example `A` 
would not get any highlightings.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65943

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  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
@@ -488,7 +488,7 @@
   checkAvailable(ID, "^vo^id^ ^f(^) {^}^"); // available everywhere.
   checkAvailable(ID, "[[int a; int b;]]");
   const char *Input = "void ^f() {}";
-  const char *Output = "void /* entity.name.function.cpp */f() {}";
+  const char *Output = "/* storage.type.primitive.cpp */void /* entity.name.function.cpp */f() {}";
   checkTransform(ID, Input, Output);
 
   checkTransform(ID,
@@ -497,8 +497,8 @@
 void f2();]]
 )cpp",
   R"cpp(
-void /* entity.name.function.cpp */f1();
-void /* entity.name.function.cpp */f2();
+/* storage.type.primitive.cpp */void /* entity.name.function.cpp */f1();
+/* storage.type.primitive.cpp */void /* entity.name.function.cpp */f2();
 )cpp");
 
checkTransform(ID,
@@ -509,7 +509,7 @@
 
   R"cpp(
 void f1();
-void /* entity.name.function.cpp */f2() {};
+/* storage.type.primitive.cpp */void /* entity.name.function.cpp */f2() {};
 )cpp");
 }
 
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -39,7 +39,8 @@
   {HighlightingKind::EnumConstant, "EnumConstant"},
   {HighlightingKind::Field, "Field"},
   {HighlightingKind::Method, "Method"},
-  {HighlightingKind::TemplateParameter, "TemplateParameter"}};
+  {HighlightingKind::TemplateParameter, "TemplateParameter"},
+  {HighlightingKind::Primitive, "Primitive"}};
   std::vector ExpectedTokens;
   for (const auto &KindString : KindToString) {
 std::vector Toks = makeHighlightingTokens(
@@ -93,26 +94,26 @@
   const char *TestCases[] = {
 R"cpp(
   struct $Class[[AS]] {
-double $Field[[SomeMember]];
+$Primitive[[double]] $Field[[SomeMember]];
   };
   struct {
   } $Variable[[S]];
-  void $Function[[foo]](int $Variable[[A]], $Class[[AS]] $Variable[[As]]) {
+  $Primitive[[void]] $Function[[foo]]($Primitive[[int]] $Variable[[A]], $Class[[AS]] $Variable[[As]]) {
 auto $Variable[[VeryLongVariableName]] = 12312;
 $Class[[AS]] $Variable[[AA]];
 auto $Variable[[L]] = $Variable[[AA]].$Field[[SomeMember]] + $Variable[[A]];
-auto $Variable[[FN]] = [ $Variable[[AA]]](int $Variable[[A]]) -> void {};
+auto $Variable[[FN]] = [ $Variable[[AA]]]($Primitive[[int]] $Variable[[A]]) -> $Primitive[[void]] {};
 $Variable[[FN]](12312);
   }
 )cpp",
 R"cpp(
-  void $Function[[foo]](int);
-  void $Function[[Gah]]();
-  void $Function[[foo]]() {
+  $Primitive[[void]] $Function[[foo]]($Primitive[[int]]);
+  $Primitive[[void]] $Function[[Gah]]();
+  $Primitive[[void]] $Function[[foo]]() {
 auto $Variable[[Bou]] = $Function[[Gah]];
   }
   struct $Class[[A]] {
-void $Method[[abc]]();
+$Primitive[[void]] $Method[[abc]]();
   };
 )cpp",
 R"cpp(
@@ -126,17 +127,17 @@
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
 typename $TemplateParameter[[T]]::A* $Field[[D]];
   };
-  $Namespace[[abc]]::$Class[[A]] $Variable[[AA]];
-  typedef $Namespace[[abc]]::$Class[[A]] $Class[[AAA]];
+  $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
+  typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
   struct $Class[[B]] {
 $Class[[B]]();
 ~$Class[[B]]();
-void operator<<($Class[[B]]);
+$Primitive[[void]] operator<<($Class[[B]]);
 $Class[[AAA]] $Field[[AA]];
   };
   $Class[[B]]::$Class[[B]]() {}
   $Class[[B]]::~$Class[[B]]() {}
-  void $Function[[f]] () {
+  $Primitive[[void]] $Function[[f]] () {
 $Class[[B]] $Variable[[BB]] = $Class[[B]]();
   

[clang-tools-extra] r368287 - [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if underlying MemberDecl is a CXXConversionDecl.

[Johan Vikstrom via cfe-commits]

Author: jvikstrom
Date: Thu Aug  8 05:43:55 2019
New Revision: 368287

URL: http://llvm.org/viewvc/llvm-project?rev=368287&view=rev
Log:
[clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if 
underlying MemberDecl is a CXXConversionDecl.

Summary:
Conversion operators contain invalid MemberLocs which caused 
SemanticHighlighting
to emit a lot of error logs in large files as they can occur fairly
often (for example converting StringRef to std string).
As the only thing happening was a lot of error logs being
emited there doesn't really seem to be any way to test this
(no erroneous tokens are added). But emiting as many logs as
were being emited is not wanted.

This also adds a test to guard against regressions for highlightings
disapearing from places where the conversion operators are used as their
behaviour differ from the other CXXMethodDecls.

Reviewers: hokein, ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

Modified: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp?rev=368287&r1=368286&r2=368287&view=diff
==
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp (original)
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp Thu Aug  8 05:43:55 
2019
@@ -52,6 +52,10 @@ public:
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // The MemberLoc is invalid for C++ conversion operators. We do not
+  // attempt to add tokens with invalid locations.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }

Modified: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp?rev=368287&r1=368286&r2=368287&view=diff
==
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp 
(original)
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp Thu 
Aug  8 05:43:55 2019
@@ -255,6 +255,23 @@ TEST(SemanticHighlighting, GetsCorrectTo
   struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
   extern template struct $Class[[Tmpl]];
   template struct $Class[[Tmpl]];
+)cpp",
+// This test is to guard against highlightings disappearing when using
+// conversion operators as their behaviour in the clang AST differ from
+// other CXXMethodDecls.
+R"cpp(
+  class $Class[[Foo]] {};
+  struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+  };
+  void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);


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

[PATCH] D65938: [AST] No longer visiting CXXMethodDecl bodies created by compiler when method was default created.

[Haojian Wu via Phabricator via cfe-commits]

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

looks good from my side.




Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:262
+  struct $Class[[A]] {
+  $Class[[B]] $Field[[BB]];
+  $Class[[A]] &operator=($Class[[A]] &&$Variable[[O]]);

nit: the indent should be 2 spaces.



Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2033
+  // generates the method body and adds it to the AST.
+  if (const CXXMethodDecl *MD = dyn_cast(D))
+VisitBody &= !MD->isDefaulted() || getDerived().shouldVisitImplicitCode();

nit: use auto, as type in RHS is obvious. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65938



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

[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.

[Johan Vikström via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
jvikstrom marked 2 inline comments as done.
Closed by commit rL368287: [clangd] Added an early return from VisitMemberExpr 
in SemanticHighlighting if… (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65928?vs=214117&id=214122#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65928

Files:
  clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
  clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // The MemberLoc is invalid for C++ conversion operators. We do not
+  // attempt to add tokens with invalid locations.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,23 @@
   struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
   extern template struct $Class[[Tmpl]];
   template struct $Class[[Tmpl]];
+)cpp",
+// This test is to guard against highlightings disappearing when using
+// conversion operators as their behaviour in the clang AST differ from
+// other CXXMethodDecls.
+R"cpp(
+  class $Class[[Foo]] {};
+  struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+  };
+  void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);


Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
   // When calling the destructor manually like: AAA::~A(); The ~ is a
   // MemberExpr. Other methods should still be highlighted though.
   return true;
+if (isa(MD))
+  // The MemberLoc is invalid for C++ conversion operators. We do not
+  // attempt to add tokens with invalid locations.
+  return true;
 addToken(ME->getMemberLoc(), MD);
 return true;
   }
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,23 @@
   struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
   extern template struct $Class[[Tmpl]];
   template struct $Class[[Tmpl]];
+)cpp",
+// This test is to guard against highlightings disappearing when using
+// conversion operators as their behaviour in the clang AST differ from
+// other CXXMethodDecls.
+R"cpp(
+  class $Class[[Foo]] {};
+  struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+  };
+  void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a project: clang.

The `ExprWithCleanups` node is added to the AST along with the elidable
CXXConstructExpr.  If it is the outermost node of the node being matched, ignore
it as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65944

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -6533,14 +6533,15 @@
 }
 
 /// Matches expressions that match InnerMatcher that are possibly wrapped in an
-/// elidable constructor.
+/// elidable constructor and other corresponding bookkeeping nodes.
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used 
to
-/// skip those elidable constructors.
+/// In C++17 copy elidable constructors are no longer being generated in the 
AST
+/// as it is not permitted by the standard. They are, however, part of the AST
+/// in C++14 and earlier. So, a matcher must elide these differences to work in
+/// all language modes. This matcher supports such elision by skipping elidable
+/// constructor-call AST nodes, `ExprWithCleanups` nodes wrapping elidable
+/// constructor-calls and various implicit nodes inside the constructor calls,
+/// all of which will not appear in the C++17 AST.
 ///
 /// Given
 ///
@@ -6552,13 +6553,20 @@
 /// }
 /// \endcode
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).
 AST_MATCHER_P(Expr, ignoringElidableConstructorCall,
   ast_matchers::internal::Matcher, InnerMatcher) {
-  if (const auto *CtorExpr = dyn_cast(&Node)) {
+  // E tracks the node that we are examining.
+  const Expr *E = &Node;
+  // If present, remove an outer `ExprWithCleanups` corresponding to the
+  // underlying `CXXConstructExpr`. This check won't cover all cases of added
+  // `ExprWithCleanups` corresponding to `CXXConstructExpr` nodes (because the
+  // EWC is placed on the outermost node of the expression, which this may not
+  // be), but, it still improves the coverage of this matcher.
+  if (const auto *CleanupsExpr = dyn_cast(&Node))
+E = CleanupsExpr->getSubExpr();
+  if (const auto *CtorExpr = dyn_cast(E)) {
 if (CtorExpr->isElidable()) {
   if (const auto *MaterializeTemp =
   dyn_cast(CtorExpr->getArg(0))) {
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -5805,14 +5805,15 @@
 
 MatcherExpr>ignoringElidableConstructorCallast_matchers::MatcherExpr> 
InnerMatcher
 Matches expressions that match 
InnerMatcher that are possibly wrapped in an
-elidable constructor.
+elidable constructor and other corresponding bookkeeping nodes.
 
-In C++17 copy elidable constructors are no longer being
-generated in the AST as it is not permitted by the standard. They are
-however part of the AST in C++14 and earlier. Therefore, to write a matcher
-that works in all language modes, the matcher has to skip elidable
-constructor AST nodes if they appear in the AST. This matcher can be used to
-skip those elidable constructors.
+In C++17 copy elidable constructors are no longer being generated in the AST
+as it is not permitted by the standard. They are, however, part of the AST
+in C++14 and earlier. So, a matcher must elide these differences to work in
+all language modes. This matcher supports such elision by skipping elidable
+constructor-call AST nodes, `ExprWithCleanups` nodes wrapping elidable
+constructor-calls and various implicit nodes inside the constructor calls,
+all of which will not appear in the C++17 AST.
 
 Given
 
@@ -5822,10 +5823,8 @@
   H D = G();
 }
 
-``varDecl(hasInitializer(any(
-  ignoringElidableConstructorCall(callExpr()),
-  exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-matches ``H D = G()``
+``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+matches ``H D = G()`` in C++11 through C++17 (and beyond).
 
 
 


Index: clang/include/clang/ASTMatchers/AS

[PATCH] D65943: [clangd] Added semantic highlighting support for primitives.

[Haojian Wu via Phabricator via cfe-commits]

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

nice, just a nit.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+// Builtins must be special cased as they do not have a TagDecl.
+if (const auto *T = TL.getTypePtr())
+  if (T->isBuiltinType())

nit: we could merge this to the `if` below. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65943



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

[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+// Currently, we only support normal rename (one range) for C/C++.
+// FIXME: support multiple-range rename for objective-c methods.
+if (Rename.getNameRanges().size() > 1)

hokein wrote:
> sammccall wrote:
> > is this a regression in this patch, or did the limitation already exist?
> this is not a regression, this is a limitation in clangd.
can you point me to where this behavior (bailing out when there's more than one 
name range) occurred in the old version of the code?

(by regression I would mean e.g. if objective-c selector renames worked before 
this change, but don't work afterwards. But either way, it'd be useful to 
understand why)



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:178
+  continue;
+cantFail(FilteredChanges.add(tooling::Replacement(
+AST.getASTContext().getSourceManager(),

hokein wrote:
> sammccall wrote:
> > why can't this fail?
> I just kept the previous behavior. Made the error emit to the client rather 
> than crashing here.
I'm not sure what you meant by "kept the previous behavior".
Yes, before this change there was a call `cantFail`, but the implementation was 
different, so it's not clear the set of errors it is handling are the same.

Previously, I guess it couldn't fail because we assumed that the underlying 
layer (which returned a set of edits) wouldn't produce conflicting ones.

Here, we're producing the set of edits ourselves, so any global property (e.g. 
"no pair of these conflict") needs to be established by us. So we should 
document whether/under what circumstances it can fail.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:182
+CharSourceRange::getCharRange(Rename.getNameRanges()[0]), 
NewName)))
+  return std::move(Err);
   }

for actual error handling behavior (if this can actually fail): is this a 
deliberate choice to refuse to perform any of the edits if there are conflicts? 
Is this better than applying some non-conflicting set?

Unlike most error-propagation, this is a nontrivial policy choice and needs a 
comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Diogo N. Sampaio via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368288: [ARM] Set default alignment to 64bits (authored by 
dnsampaio, committed by ).

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65000

Files:
  cfe/trunk/lib/Basic/Targets/ARM.cpp
  cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp
  cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp


Index: cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
===
--- cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
+++ cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
@@ -2,11 +2,12 @@
 // RUN: %clang_cc1 -triple arm64-apple-ios10 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos10 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos4 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
+// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions  -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-ios12 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos12 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos5 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
-// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
+// RUN: %clang_cc1 -triple arm-linux-androideabi -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple aarch64-linux-gnueabi -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mipsel-linux-gnu -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mips64el-linux-gnu -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
Index: cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp
===
--- cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp
+++ cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp
@@ -0,0 +1,21 @@
+// Bug: https://bugs.llvm.org/show_bug.cgi?id=42668
+// REQUIRES: arm-registered-target
+
+// RUN: %clang_cc1 -triple armv8-arm-none-eabi -emit-llvm -target-cpu generic 
-Os -fcxx-exceptions -o - -x c++ %s | FileCheck --check-prefixes=CHECK,A8 %s
+// RUN: %clang_cc1 -triple armv8-unknown-linux-android -emit-llvm -target-cpu 
generic -Os -fcxx-exceptions -o - -x c++ %s | FileCheck 
--check-prefixes=CHECK,A16 %s
+
+// CHECK: [[E:%[A-z0-9]+]] = tail call i8* @__cxa_allocate_exception
+// CHECK-NEXT: [[BC:%[A-z0-9]+]] = bitcast i8* [[E]] to <2 x i64>*
+// A8-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 8
+// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16
+#include 
+
+int main(void) {
+  try {
+throw vld1q_u64(((const uint64_t[2]){1, 2}));
+  } catch (uint64x2_t exc) {
+return 0;
+  }
+  return 1;
+}
+
Index: cfe/trunk/lib/Basic/Targets/ARM.cpp
===
--- cfe/trunk/lib/Basic/Targets/ARM.cpp
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp
@@ -309,8 +309,9 @@
   setAtomic();
 
   // Maximum alignment for ARM NEON data types should be 64-bits (AAPCS)
+  // as well the default alignment
   if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
-MaxVectorAlign = 64;
+DefaultAlignForAttributeAligned = MaxVectorAlign = 64;
 
   // Do force alignment of members that follow zero length bitfields.  If
   // the alignment of the zero-length bitfield is greater than the member


Index: cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
===
--- cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
+++ cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
@@ -2,11 +2,12 @@
 // RUN: %clang_cc1 -triple arm64-apple-ios10 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos10 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos4 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
+// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions  -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-ios12 -verify -fsyntax-only -

r368288 - [ARM] Set default alignment to 64bits

[Diogo N. Sampaio via cfe-commits]

Author: dnsampaio
Date: Thu Aug  8 05:50:36 2019
New Revision: 368288

URL: http://llvm.org/viewvc/llvm-project?rev=368288&view=rev
Log:
[ARM] Set default alignment to 64bits

Summary:
The maximum alignment used by ARM arch
is 64bits, not 128.

This could cause overaligned memory
access for 128 bit neon vector that
have unpredictable behaviour.

This fixes: https://bugs.llvm.org/show_bug.cgi?id=42668

Reviewers: ostannard, dmgreen, srhines, danalbert, pirama, peter.smith

Reviewed By: pirama, peter.smith

Subscribers: phosek, thegameg, thakis, llvm-commits, carwil, peter.smith, 
javed.absar, kristof.beyls, cfe-commits

Tags: #clang, #llvm

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

Added:
cfe/trunk/test/CodeGenCXX/ARM/
cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp
Modified:
cfe/trunk/lib/Basic/Targets/ARM.cpp
cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp

Modified: cfe/trunk/lib/Basic/Targets/ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/ARM.cpp?rev=368288&r1=368287&r2=368288&view=diff
==
--- cfe/trunk/lib/Basic/Targets/ARM.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp Thu Aug  8 05:50:36 2019
@@ -309,8 +309,9 @@ ARMTargetInfo::ARMTargetInfo(const llvm:
   setAtomic();
 
   // Maximum alignment for ARM NEON data types should be 64-bits (AAPCS)
+  // as well the default alignment
   if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
-MaxVectorAlign = 64;
+DefaultAlignForAttributeAligned = MaxVectorAlign = 64;
 
   // Do force alignment of members that follow zero length bitfields.  If
   // the alignment of the zero-length bitfield is greater than the member

Added: cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp?rev=368288&view=auto
==
--- cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp (added)
+++ cfe/trunk/test/CodeGenCXX/ARM/exception-alignment.cpp Thu Aug  8 05:50:36 
2019
@@ -0,0 +1,21 @@
+// Bug: https://bugs.llvm.org/show_bug.cgi?id=42668
+// REQUIRES: arm-registered-target
+
+// RUN: %clang_cc1 -triple armv8-arm-none-eabi -emit-llvm -target-cpu generic 
-Os -fcxx-exceptions -o - -x c++ %s | FileCheck --check-prefixes=CHECK,A8 %s
+// RUN: %clang_cc1 -triple armv8-unknown-linux-android -emit-llvm -target-cpu 
generic -Os -fcxx-exceptions -o - -x c++ %s | FileCheck 
--check-prefixes=CHECK,A16 %s
+
+// CHECK: [[E:%[A-z0-9]+]] = tail call i8* @__cxa_allocate_exception
+// CHECK-NEXT: [[BC:%[A-z0-9]+]] = bitcast i8* [[E]] to <2 x i64>*
+// A8-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 8
+// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16
+#include 
+
+int main(void) {
+  try {
+throw vld1q_u64(((const uint64_t[2]){1, 2}));
+  } catch (uint64x2_t exc) {
+return 0;
+  }
+  return 1;
+}
+

Modified: cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp?rev=368288&r1=368287&r2=368288&view=diff
==
--- cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-overaligned-type-thrown.cpp Thu Aug  8 05:50:36 
2019
@@ -2,11 +2,12 @@
 // RUN: %clang_cc1 -triple arm64-apple-ios10 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos10 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos4 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
+// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions  -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-ios12 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos12 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos5 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
-// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
+// RUN: %clang_cc1 -triple arm-linux-androideabi -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple aarch64-linux-gnueabi -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mipsel-linux-gnu -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mips64el-linux-gnu -verify -fsyntax-only -s

[clang-tools-extra] r368291 - [clangd] Added semantic highlighting support for primitives.

[Johan Vikstrom via cfe-commits]

Author: jvikstrom
Date: Thu Aug  8 06:10:30 2019
New Revision: 368291

URL: http://llvm.org/viewvc/llvm-project?rev=368291&view=rev
Log:
[clangd] Added semantic highlighting support for primitives.

Summary:
Adds a new HighlightingKind "Primitive". Adds a special case for TypeLocs that 
have an underlying TypePtr that is are builtin types, adding them as primitives.
The primary reason for this change is because otherwise typedefs that typedef 
primitives `typedef int A` would not get highlighted (so in the example `A` 
would not get any highlightings.)

Reviewers: hokein, ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

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

Modified: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp?rev=368291&r1=368290&r2=368291&view=diff
==
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp (original)
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp Thu Aug  8 06:10:30 
2019
@@ -138,9 +138,13 @@ public:
 
 private:
   void addTypeLoc(SourceLocation Loc, const TypeLoc &TL) {
-if (const Type *TP = TL.getTypePtr())
+if (const Type *TP = TL.getTypePtr()) {
   if (const TagDecl *TD = TP->getAsTagDecl())
 addToken(Loc, TD);
+  if (TP->isBuiltinType())
+// Builtins must be special cased as they do not have a TagDecl.
+addToken(Loc, HighlightingKind::Primitive);
+}
   }
 
   void addToken(SourceLocation Loc, const NamedDecl *D) {
@@ -386,6 +390,8 @@ llvm::StringRef toTextMateScope(Highligh
 return "entity.name.namespace.cpp";
   case HighlightingKind::TemplateParameter:
 return "entity.name.type.template.cpp";
+  case HighlightingKind::Primitive:
+return "storage.type.primitive.cpp";
   case HighlightingKind::NumKinds:
 llvm_unreachable("must not pass NumKinds to the function");
   }

Modified: clang-tools-extra/trunk/clangd/SemanticHighlighting.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/SemanticHighlighting.h?rev=368291&r1=368290&r2=368291&view=diff
==
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.h (original)
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.h Thu Aug  8 06:10:30 
2019
@@ -33,6 +33,7 @@ enum class HighlightingKind {
   EnumConstant,
   Namespace,
   TemplateParameter,
+  Primitive,
 
   NumKinds,
 };

Modified: clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/test/semantic-highlighting.test?rev=368291&r1=368290&r2=368291&view=diff
==
--- clang-tools-extra/trunk/clangd/test/semantic-highlighting.test (original)
+++ clang-tools-extra/trunk/clangd/test/semantic-highlighting.test Thu Aug  8 
06:10:30 2019
@@ -30,6 +30,9 @@
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.type.template.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"storage.type.primitive.cpp"
 # CHECK-NEXT:  ]
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
@@ -40,7 +43,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:"tokens": "AAADAAkEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -55,11 +58,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:"tokens": "AAADAAkEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:"tokens": "AAADAAkEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -74,7 +77,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:"tokens": "AAADAAkEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:   ],
 # CHECK-NEXT:"textDocument": {

Modified: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp?rev=368291&r1=368290&r2=368291&vie

[PATCH] D65943: [clangd] Added semantic highlighting support for primitives.

[Johan Vikström via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
jvikstrom marked an inline comment as done.
Closed by commit rL368291: [clangd] Added semantic highlighting support for 
primitives. (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65943?vs=214121&id=214130#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65943

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

Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -138,9 +138,13 @@
 
 private:
   void addTypeLoc(SourceLocation Loc, const TypeLoc &TL) {
-if (const Type *TP = TL.getTypePtr())
+if (const Type *TP = TL.getTypePtr()) {
   if (const TagDecl *TD = TP->getAsTagDecl())
 addToken(Loc, TD);
+  if (TP->isBuiltinType())
+// Builtins must be special cased as they do not have a TagDecl.
+addToken(Loc, HighlightingKind::Primitive);
+}
   }
 
   void addToken(SourceLocation Loc, const NamedDecl *D) {
@@ -386,6 +390,8 @@
 return "entity.name.namespace.cpp";
   case HighlightingKind::TemplateParameter:
 return "entity.name.type.template.cpp";
+  case HighlightingKind::Primitive:
+return "storage.type.primitive.cpp";
   case HighlightingKind::NumKinds:
 llvm_unreachable("must not pass NumKinds to the function");
   }
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -39,7 +39,8 @@
   {HighlightingKind::EnumConstant, "EnumConstant"},
   {HighlightingKind::Field, "Field"},
   {HighlightingKind::Method, "Method"},
-  {HighlightingKind::TemplateParameter, "TemplateParameter"}};
+  {HighlightingKind::TemplateParameter, "TemplateParameter"},
+  {HighlightingKind::Primitive, "Primitive"}};
   std::vector ExpectedTokens;
   for (const auto &KindString : KindToString) {
 std::vector Toks = makeHighlightingTokens(
@@ -93,26 +94,26 @@
   const char *TestCases[] = {
 R"cpp(
   struct $Class[[AS]] {
-double $Field[[SomeMember]];
+$Primitive[[double]] $Field[[SomeMember]];
   };
   struct {
   } $Variable[[S]];
-  void $Function[[foo]](int $Variable[[A]], $Class[[AS]] $Variable[[As]]) {
+  $Primitive[[void]] $Function[[foo]]($Primitive[[int]] $Variable[[A]], $Class[[AS]] $Variable[[As]]) {
 auto $Variable[[VeryLongVariableName]] = 12312;
 $Class[[AS]] $Variable[[AA]];
 auto $Variable[[L]] = $Variable[[AA]].$Field[[SomeMember]] + $Variable[[A]];
-auto $Variable[[FN]] = [ $Variable[[AA]]](int $Variable[[A]]) -> void {};
+auto $Variable[[FN]] = [ $Variable[[AA]]]($Primitive[[int]] $Variable[[A]]) -> $Primitive[[void]] {};
 $Variable[[FN]](12312);
   }
 )cpp",
 R"cpp(
-  void $Function[[foo]](int);
-  void $Function[[Gah]]();
-  void $Function[[foo]]() {
+  $Primitive[[void]] $Function[[foo]]($Primitive[[int]]);
+  $Primitive[[void]] $Function[[Gah]]();
+  $Primitive[[void]] $Function[[foo]]() {
 auto $Variable[[Bou]] = $Function[[Gah]];
   }
   struct $Class[[A]] {
-void $Method[[abc]]();
+$Primitive[[void]] $Method[[abc]]();
   };
 )cpp",
 R"cpp(
@@ -126,17 +127,17 @@
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
 typename $TemplateParameter[[T]]::A* $Field[[D]];
   };
-  $Namespace[[abc]]::$Class[[A]] $Variable[[AA]];
-  typedef $Namespace[[abc]]::$Class[[A]] $Class[[AAA]];
+  $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
+  typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
   struct $Class[[B]] {
 $Class[[B]]();
 ~$Class[[B]]();
-void operator<<($Class[[B]]);
+$Primitive[[void]] operator<<($Class[[B]]);
 $Class[[AAA]] $Field[[AA]];
   };
   $Class[[B]]::$Class[[B]]() {}
   $Class[[B]]::~$Class[[B]]() {}
-  void $Function[[f]] () {
+  $Primitive[[void]] $Function[[f]] () {
 $Class[[B]] $Variable[[BB]] = $Class[[B]]();
 $Variable[[BB]].~$Class[[B]]();
 $Class[[B]]();
@@ -154,7 +155,7 @@
 $Enum[[E]] $Fi

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> hfinkel wrote:
> > ABataev wrote:
> > > hfinkel wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > I think this should cause an error or at least a 
> > > > > > > > > > > > warning. Telling the compiler `ps` is a device pointer 
> > > > > > > > > > > > only to create a local uninitialized shadowing variable 
> > > > > > > > > > > > seems like an error to me.
> > > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy must 
> > > > > > > > > > > be created in the context of the parallel region, not the 
> > > > > > > > > > > target region. So, for OpenMP 5.0 we should not emit 
> > > > > > > > > > > error here.
> > > > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > > > It means, that for OpenMP 5.0 we should emit a warning/error 
> > > > > > > > > here. It is allowed according to the standard, we should 
> > > > > > > > > allow it too.
> > > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > > 
> > > > > > > > The last answer contradicts what you said earlier. I expect 
> > > > > > > > there is a *not* missing, correct?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Assuming you do not want an error, which is fine, I still think 
> > > > > > > > a warning is appropriate as it seems to me there is never a 
> > > > > > > > reason to have a `is_device_ptr` clause for a private value. I 
> > > > > > > > mean, it is either a bug by the programmer, e.g., 5 letters of 
> > > > > > > > `firstprivate` went missing, or simply nonsensical code for 
> > > > > > > > which we warn in other situations as well.
> > > > > > > Missed `not`.
> > > > > > > These kind of construct are explicitly allowed in OpenMP. And we 
> > > > > > > should obey the standard unconditionally.
> > > > > > > Plus, there might be situations where user may require it 
> > > > > > > explicitly. For example, the device pointer is dereferenced in 
> > > > > > > one of the clauses for the subregions but in the deeper subregion 
> > > > > > > it might be used as a private pointer. Why we should emit a 
> > > > > > > warning here?
> > > > > > If you have a different situation, e.g., the one you describe, you 
> > > > > > should not have a warning. Though, that is not the point. If you 
> > > > > > have the situation above (single directive), as per my reasoning, 
> > > > > > there doesn't seem to be a sensible use case. If you have one and 
> > > > > > we should create an explicit test for it.
> > > > > > 
> > > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > > And we should obey the standard unconditionally.
> > > > > > Nobody says we should not. We warn people all the time even if it 
> > > > > > is valid code.
> > > > > Warnings may prevent successful compilation in some cases, e.g. when 
> > > > > warnings are treated as errors. Why we should emit a warning if the 
> > > > > construct is allowed by the standard? Ask to change the standard if 
> > > > > you did not agree with it.
> > > > Warnings are specifically for constructs which are legal, but likely 
> > > > wrong (i.e., will not do what the user likely intended). Treating 
> > > > warnings as errors is not a conforming compilation mode - by design 
> > > > (specifically because that will reject conforming programs). Thus...
> > > > 
> > > > > Why we should emit a warning if the construct is allowed by the 
> > > > > standard? Ask to change the standard if you did not agree with it.
> > > > 
> > > > This is the wrong way to approach this. Warnings are specifically for 
> > > > legal code. They help users prevent errors, however, in cases where 
> > > > that legal code is likely problematic or won't do what the user intends.
> > > > 
> > > Ok, we could emit wqrnings in some cases. But better to do it in the 
> > > separate patches. Each particular case requires additional analysis.
> > > 
> > > > This is the wrong way to approach this.
> > > 
> > > I don't think so. If some cases are really meaningless, better to ask to 
> > > prohibit them in the standard. It is always a good idea to change the 
> > > requirements first, if you think that some scenar

[PATCH] D65926: [clangd] Fixed printTemplateSpecializationArgs not printing partial variable specialization arguments.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 214132.
jvikstrom marked 2 inline comments as done.
jvikstrom added a comment.

Merged test with PrintASTTest.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65926

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


Index: clang-tools-extra/clangd/unittests/PrintASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/PrintASTTests.cpp
+++ clang-tools-extra/clangd/unittests/PrintASTTests.cpp
@@ -96,6 +96,15 @@
   struct Bar { friend class Foo; };
   template <> struct ^Foo {};)cpp",
 {""}},
+{
+R"cpp(
+  template
+  T S = T(10);
+  template 
+  int ^S = 0;
+  template <>
+  int ^S = 0;)cpp",
+{"", ""}},
 })),);
 } // namespace
 } // namespace clangd
Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -188,7 +188,7 @@
 AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("i"), WithTemplateArgs("")),
 AllOf(DeclNamed("d"), WithTemplateArgs("")),
-AllOf(DeclNamed("foo"), WithTemplateArgs("<>")),
+AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("foo"), WithTemplateArgs(""))}));
 }
 
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -36,6 +36,10 @@
  llvm::dyn_cast(&ND)) {
 if (auto *Args = Cls->getTemplateArgsAsWritten())
   return Args->arguments();
+  } else if (auto *Var =
+ llvm::dyn_cast(&ND)) {
+if (auto *Args = Var->getTemplateArgsAsWritten())
+  return Args->arguments();
   } else if (auto *Var = llvm::dyn_cast(&ND))
 return Var->getTemplateArgsInfo().arguments();
   // We return None for ClassTemplateSpecializationDecls because it does not


Index: clang-tools-extra/clangd/unittests/PrintASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/PrintASTTests.cpp
+++ clang-tools-extra/clangd/unittests/PrintASTTests.cpp
@@ -96,6 +96,15 @@
   struct Bar { friend class Foo; };
   template <> struct ^Foo {};)cpp",
 {""}},
+{
+R"cpp(
+  template
+  T S = T(10);
+  template 
+  int ^S = 0;
+  template <>
+  int ^S = 0;)cpp",
+{"", ""}},
 })),);
 } // namespace
 } // namespace clangd
Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -188,7 +188,7 @@
 AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("i"), WithTemplateArgs("")),
 AllOf(DeclNamed("d"), WithTemplateArgs("")),
-AllOf(DeclNamed("foo"), WithTemplateArgs("<>")),
+AllOf(DeclNamed("foo"), WithTemplateArgs("")),
 AllOf(DeclNamed("foo"), WithTemplateArgs(""))}));
 }
 
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -36,6 +36,10 @@
  llvm::dyn_cast(&ND)) {
 if (auto *Args = Cls->getTemplateArgsAsWritten())
   return Args->arguments();
+  } else if (auto *Var =
+ llvm::dyn_cast(&ND)) {
+if (auto *Args = Var->getTemplateArgsAsWritten())
+  return Args->arguments();
   } else if (auto *Var = llvm::dyn_cast(&ND))
 return Var->getTemplateArgsInfo().arguments();
   // We return None for ClassTemplateSpecializationDecls because it d

[PATCH] D65926: [clangd] Fixed printTemplateSpecializationArgs not printing partial variable specialization arguments.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:40
 
+TEST(PrintTemplateSpecializationArgs, PrintsTemplateArgs) {
+  TestTU TU;

hokein wrote:
> looks like the related tests are in `PrintASTTests.cpp`, could you move the 
> test there?
Oh I didn't see that test file. Seems like a lot of this is already tested, so 
just removing everything that isn't related to template variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65926



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

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6538
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used 
to
-/// skip those elidable constructors.
+/// In C++17 copy elidable constructors are no longer being generated in the 
AST
+/// as it is not permitted by the standard. They are, however, part of the AST

while you're editing it: "elidable copy constructors"



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6540
+/// as it is not permitted by the standard. They are, however, part of the AST
+/// in C++14 and earlier. So, a matcher must elide these differences to work in
+/// all language modes. This matcher supports such elision by skipping elidable

"elide these differences" => "abstract over these differences"?



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6541
+/// in C++14 and earlier. So, a matcher must elide these differences to work in
+/// all language modes. This matcher supports such elision by skipping elidable
+/// constructor-call AST nodes, `ExprWithCleanups` nodes wrapping elidable

"This matcher skips elidable constructor call AST nodes, ... "?



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6556
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).

Could you add this matcher as a test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65944



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

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Would it be an alternative solution if we imported the function parameters 
after we created the FunctionDecl? I guess it would be. Maybe that would result 
in a smaller change, what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577



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

r368295 - [OPENMP]Add support for analysis of linear variables and step.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Thu Aug  8 06:42:45 2019
New Revision: 368295

URL: http://llvm.org/viewvc/llvm-project?rev=368295&view=rev
Log:
[OPENMP]Add support for analysis of linear variables and step.

Summary:
Added support for basic analysis of the linear variables and linear step
expression. Linear loop iteration variables must be excluded from this
analysis, only non-loop iteration variables must be analyzed.

Reviewers: NoQ

Subscribers: guansong, cfe-commits, caomhin, kkwli0

Tags: #clang

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

Modified:
cfe/trunk/include/clang/AST/OpenMPClause.h
cfe/trunk/lib/AST/OpenMPClause.cpp
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Analysis/cfg-openmp.cpp
cfe/trunk/test/OpenMP/distribute_parallel_for_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/distribute_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/for_linear_messages.cpp
cfe/trunk/test/OpenMP/for_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/parallel_for_linear_messages.cpp
cfe/trunk/test/OpenMP/parallel_for_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/simd_linear_messages.cpp
cfe/trunk/test/OpenMP/target_parallel_for_linear_messages.cpp
cfe/trunk/test/OpenMP/target_parallel_for_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/target_simd_linear_messages.cpp

cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/target_teams_distribute_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/taskloop_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/teams_distribute_parallel_for_simd_linear_messages.cpp
cfe/trunk/test/OpenMP/teams_distribute_simd_linear_messages.cpp

Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=368295&r1=368294&r2=368295&view=diff
==
--- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
+++ cfe/trunk/include/clang/AST/OpenMPClause.h Thu Aug  8 06:42:45 2019
@@ -3216,6 +3216,14 @@ class OMPLinearClause final
 return llvm::makeArrayRef(getUpdates().end(), varlist_size());
   }
 
+  /// Gets the list of used expressions for linear variables.
+  MutableArrayRef getUsedExprs() {
+return MutableArrayRef(getFinals().end() + 2, varlist_size() + 1);
+  }
+  ArrayRef getUsedExprs() const {
+return llvm::makeArrayRef(getFinals().end() + 2, varlist_size() + 1);
+  }
+
   /// Sets the list of the copies of original linear variables.
   /// \param PL List of expressions.
   void setPrivates(ArrayRef PL);
@@ -3295,6 +3303,9 @@ public:
   /// \param FL List of expressions.
   void setFinals(ArrayRef FL);
 
+  /// Sets the list of used expressions for the linear clause.
+  void setUsedExprs(ArrayRef UE);
+
   using privates_iterator = MutableArrayRef::iterator;
   using privates_const_iterator = ArrayRef::iterator;
   using privates_range = llvm::iterator_range;
@@ -3347,6 +3358,21 @@ public:
 return finals_const_range(getFinals().begin(), getFinals().end());
   }
 
+  using used_expressions_iterator = MutableArrayRef::iterator;
+  using used_expressions_const_iterator = ArrayRef::iterator;
+  using used_expressions_range =
+  llvm::iterator_range;
+  using used_expressions_const_range =
+  llvm::iterator_range;
+
+  used_expressions_range used_expressions() {
+return finals_range(getUsedExprs().begin(), getUsedExprs().end());
+  }
+
+  used_expressions_const_range used_expressions() const {
+return finals_const_range(getUsedExprs().begin(), getUsedExprs().end());
+  }
+
   child_range children() {
 return child_range(reinterpret_cast(varlist_begin()),
reinterpret_cast(varlist_end()));
@@ -3357,11 +3383,11 @@ public:
 return const_child_range(Children.begin(), Children.end());
   }
 
-  child_range used_children() {
-return child_range(child_iterator(), child_iterator());
-  }
+  child_range used_children();
+
   const_child_range used_children() const {
-return const_child_range(const_child_iterator(), const_child_iterator());
+auto Children = const_cast(this)->used_children();
+return const_child_range(Children.begin(), Children.end());
   }
 
   static bool classof(const OMPClause *T) {

Modified: cfe/trunk/lib/AST/OpenMPClause.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/OpenMPClause.cpp?rev=368295&r1=368294&r2=368295&view=diff
==
--- cfe/trunk/lib/AST/OpenMPClause.cpp (original)
+++ cfe/trunk/lib/AST/OpenMPClause.cpp Thu Aug  8 06:42:45 2019
@@ -429,15 +429,23 @@ void OMPLinearClause::setFinals(ArrayRef
   std::copy(FL.begin(), FL.end(), getUpdates().end());
 }
 
+void OMPLinearClause::setUsedExprs(ArrayRef UE) {
+  assert(
+  UE.size() == va

[PATCH] D65461: [OPENMP]Add support for analysis of linear variables and step.

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368295: [OPENMP]Add support for analysis of linear variables 
and step. (authored by ABataev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65461

Files:
  cfe/trunk/include/clang/AST/OpenMPClause.h
  cfe/trunk/lib/AST/OpenMPClause.cpp
  cfe/trunk/lib/Sema/SemaOpenMP.cpp
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/Analysis/cfg-openmp.cpp
  cfe/trunk/test/OpenMP/distribute_parallel_for_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/distribute_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/for_linear_messages.cpp
  cfe/trunk/test/OpenMP/for_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/parallel_for_linear_messages.cpp
  cfe/trunk/test/OpenMP/parallel_for_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_for_linear_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_for_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/target_simd_linear_messages.cpp
  
cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/taskloop_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/teams_distribute_parallel_for_simd_linear_messages.cpp
  cfe/trunk/test/OpenMP/teams_distribute_simd_linear_messages.cpp

Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -6846,6 +6846,8 @@
   }
   Record.AddStmt(C->getStep());
   Record.AddStmt(C->getCalcStep());
+  for (auto *VE : C->used_expressions())
+Record.AddStmt(VE);
 }
 
 void OMPClauseWriter::VisitOMPAlignedClause(OMPAlignedClause *C) {
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -12736,6 +12736,10 @@
   C->setFinals(Vars);
   C->setStep(Record.readSubExpr());
   C->setCalcStep(Record.readSubExpr());
+  Vars.clear();
+  for (unsigned I = 0; I != NumVars + 1; ++I)
+Vars.push_back(Record.readSubExpr());
+  C->setUsedExprs(Vars);
 }
 
 void OMPClauseReader::VisitOMPAlignedClause(OMPAlignedClause *C) {
Index: cfe/trunk/lib/AST/OpenMPClause.cpp
===
--- cfe/trunk/lib/AST/OpenMPClause.cpp
+++ cfe/trunk/lib/AST/OpenMPClause.cpp
@@ -429,15 +429,23 @@
   std::copy(FL.begin(), FL.end(), getUpdates().end());
 }
 
+void OMPLinearClause::setUsedExprs(ArrayRef UE) {
+  assert(
+  UE.size() == varlist_size() + 1 &&
+  "Number of used expressions is not the same as the preallocated buffer");
+  std::copy(UE.begin(), UE.end(), getFinals().end() + 2);
+}
+
 OMPLinearClause *OMPLinearClause::Create(
 const ASTContext &C, SourceLocation StartLoc, SourceLocation LParenLoc,
 OpenMPLinearClauseKind Modifier, SourceLocation ModifierLoc,
 SourceLocation ColonLoc, SourceLocation EndLoc, ArrayRef VL,
 ArrayRef PL, ArrayRef IL, Expr *Step, Expr *CalcStep,
 Stmt *PreInit, Expr *PostUpdate) {
-  // Allocate space for 4 lists (Vars, Inits, Updates, Finals) and 2 expressions
-  // (Step and CalcStep).
-  void *Mem = C.Allocate(totalSizeToAlloc(5 * VL.size() + 2));
+  // Allocate space for 5 lists (Vars, Inits, Updates, Finals), 2 expressions
+  // (Step and CalcStep), list of used expression + step.
+  void *Mem =
+  C.Allocate(totalSizeToAlloc(5 * VL.size() + 2 + VL.size() + 1));
   OMPLinearClause *Clause = new (Mem) OMPLinearClause(
   StartLoc, LParenLoc, Modifier, ModifierLoc, ColonLoc, EndLoc, VL.size());
   Clause->setVarRefs(VL);
@@ -449,6 +457,8 @@
 nullptr);
   std::fill(Clause->getUpdates().end(), Clause->getUpdates().end() + VL.size(),
 nullptr);
+  std::fill(Clause->getUsedExprs().begin(), Clause->getUsedExprs().end(),
+nullptr);
   Clause->setStep(Step);
   Clause->setCalcStep(CalcStep);
   Clause->setPreInitStmt(PreInit);
@@ -458,12 +468,19 @@
 
 OMPLinearClause *OMPLinearClause::CreateEmpty(const ASTContext &C,
   unsigned NumVars) {
-  // Allocate space for 4 lists (Vars, Inits, Updates, Finals) and 2 expressions
-  // (Step and CalcStep).
-  void *Mem = C.Allocate(totalSizeToAlloc(5 * NumVars + 2));
+  // Allocate space for 5 lists (Vars, Inits, Updates, Finals), 2 expressions
+  // (Step and CalcStep), list of used expression + step.
+  void *Mem = C.Allocate(totalSizeToAlloc(5 * NumVars + 2 + NumVars  +1));
   return new (Mem) OMPLinearClause(NumVars);
 }
 
+OMPClause::child_range OMPLinearClau

[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Haojian Wu via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+// Currently, we only support normal rename (one range) for C/C++.
+// FIXME: support multiple-range rename for objective-c methods.
+if (Rename.getNameRanges().size() > 1)

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > is this a regression in this patch, or did the limitation already exist?
> > this is not a regression, this is a limitation in clangd.
> can you point me to where this behavior (bailing out when there's more than 
> one name range) occurred in the old version of the code?
> 
> (by regression I would mean e.g. if objective-c selector renames worked 
> before this change, but don't work afterwards. But either way, it'd be useful 
> to understand why)
There is a 
[FIXME](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L219)
 states that. 

> can you point me to where this behavior (bailing out when there's more than 
> one name range) occurred in the old version of the code?

I'm pretty sure that  the old version of the code will trigger the 
[assertion](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L151)
 when doing multiple-piece rename, as the name pieces of the `NewName` is 
always 1 according to the 
[code](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L221).



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:178
+  continue;
+cantFail(FilteredChanges.add(tooling::Replacement(
+AST.getASTContext().getSourceManager(),

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > why can't this fail?
> > I just kept the previous behavior. Made the error emit to the client rather 
> > than crashing here.
> I'm not sure what you meant by "kept the previous behavior".
> Yes, before this change there was a call `cantFail`, but the implementation 
> was different, so it's not clear the set of errors it is handling are the 
> same.
> 
> Previously, I guess it couldn't fail because we assumed that the underlying 
> layer (which returned a set of edits) wouldn't produce conflicting ones.
> 
> Here, we're producing the set of edits ourselves, so any global property 
> (e.g. "no pair of these conflict") needs to be established by us. So we 
> should document whether/under what circumstances it can fail.
> Previously, I guess it couldn't fail because we assumed that the underlying 
> layer (which returned a set of edits) wouldn't produce conflicting ones.

This is true, at this point we shouldn't see conflicts, because if we have 
conflicts, we have caught an 
[error](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L155)
  (in the RefactoringResultConsumer::handleError) when invoking the 
`RenameOccurrences`.

To sum up, in the previous version, we will report the error and refuse to 
apply any edits if there are conflicts, which is the same behavior we keep in 
current patch.
The only difference I see is that we traversal main file decls (rather than 
TUDecl), if we have bugs in the main file decls, we may generate conflicting 
edits, but I wouldn't too worry about this (as top level decls are a small 
subset of decls inside TUDecl).







Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:182
+CharSourceRange::getCharRange(Rename.getNameRanges()[0]), 
NewName)))
+  return std::move(Err);
   }

sammccall wrote:
> for actual error handling behavior (if this can actually fail): is this a 
> deliberate choice to refuse to perform any of the edits if there are 
> conflicts? Is this better than applying some non-conflicting set?
> 
> Unlike most error-propagation, this is a nontrivial policy choice and needs a 
> comment.
I think if there are conflicts, it is a signal that indicates we have bugs in 
the code (either in clangd, or rename library), applying parts of them may be 
not valuable (like generating uncompilable code)

I'd prefer to refuse to perform renaming if we have conflicts, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936



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

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

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

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



Comment at: clang/test/Analysis/Inputs/ctu-other.cpp:135
+
+struct DefParmContext {
+  static const int I;

martong wrote:
> shafik wrote:
> > martong wrote:
> > > Perhaps we could write `Default` instead of `Def`.
> > +1 to this and the name suggestion below.
> Please do not forget to rename.
I did not get response to the name of `testDefParmIncompleteImport`, I think 
`testImportOfIncompleteDefaultParm` is not good, probably 
`testImportOfIncompleteDefaultParmDuringImport` is better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577



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

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

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

balazske added a comment.

Yes importing the parameters should be doable after the create of the function. 
I am not sure if it fixes this problem, the parameter that has the default 
expression can still be encountered without the default expression set in it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577



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

r368301 - [FIX][NFC] Update clang sema test

[Diogo N. Sampaio via cfe-commits]

Author: dnsampaio
Date: Thu Aug  8 07:45:42 2019
New Revision: 368301

URL: http://llvm.org/viewvc/llvm-project?rev=368301&view=rev
Log:
[FIX][NFC] Update clang sema test

Try to fix Sema test for default alignment for when
compiling to ARM, but not to android, due
r9427aa2d543b


Modified:
cfe/trunk/test/Sema/struct-packed-align.c

Modified: cfe/trunk/test/Sema/struct-packed-align.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/struct-packed-align.c?rev=368301&r1=368300&r2=368301&view=diff
==
--- cfe/trunk/test/Sema/struct-packed-align.c (original)
+++ cfe/trunk/test/Sema/struct-packed-align.c Thu Aug  8 07:45:42 2019
@@ -59,7 +59,7 @@ extern int e2[__alignof(struct as1) == 8
 struct __attribute__((aligned)) as1_2 {
 char c;
 };
-#ifdef __s390x__
+#if ( defined(__s390x__) || ( defined (__ARM_32BIT_STATE) && ! 
defined(__ANDROID__) ) )
 extern int e1_2[sizeof(struct as1_2) == 8 ? 1 : -1];
 extern int e2_2[__alignof(struct as1_2) == 8 ? 1 : -1];
 #else


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

[PATCH] D65630: [PowerPC] Port SSE3, SSSE3 and SSE4 intrinsics to PowerPC.

[Jinsong Ji via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65630



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

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

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

In D65835#1619560 , @ABataev wrote:

> In D65835#1619549 , @jdenny wrote:
>
> > In D65835#1619526 , @ABataev wrote:
> >
> > > > Maybe, but I haven't found any statement in either version that states 
> > > > that map restrictions apply to is_device_ptr.
> > >
> > > `is_device_ptr` is a kind of mapping clause. I assume we can extend the 
> > > restrictions for `map` clause to this clause too.
> >
> >
> > I'd like to understand this better.  Is there something from the spec we 
> > can quote in the code?
>
>
> See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives


I looked again.  I'm still not finding any text in that section that implies 
is_device_ptr follows the same restrictions as map.  Can you please cite 
specific lines of text instead of an entire section?  Thanks for your help.




Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

hfinkel wrote:
> ABataev wrote:
> > hfinkel wrote:
> > > ABataev wrote:
> > > > hfinkel wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > I think this should cause an error or at least a 
> > > > > > > > > > > > > warning. Telling the compiler `ps` is a device 
> > > > > > > > > > > > > pointer only to create a local uninitialized 
> > > > > > > > > > > > > shadowing variable seems like an error to me.
> > > > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy 
> > > > > > > > > > > > must be created in the context of the parallel region, 
> > > > > > > > > > > > not the target region. So, for OpenMP 5.0 we should not 
> > > > > > > > > > > > emit error here.
> > > > > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > > > > It means, that for OpenMP 5.0 we should emit a 
> > > > > > > > > > warning/error here. It is allowed according to the 
> > > > > > > > > > standard, we should allow it too.
> > > > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > > > 
> > > > > > > > > The last answer contradicts what you said earlier. I expect 
> > > > > > > > > there is a *not* missing, correct?
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Assuming you do not want an error, which is fine, I still 
> > > > > > > > > think a warning is appropriate as it seems to me there is 
> > > > > > > > > never a reason to have a `is_device_ptr` clause for a private 
> > > > > > > > > value. I mean, it is either a bug by the programmer, e.g., 5 
> > > > > > > > > letters of `firstprivate` went missing, or simply nonsensical 
> > > > > > > > > code for which we warn in other situations as well.
> > > > > > > > Missed `not`.
> > > > > > > > These kind of construct are explicitly allowed in OpenMP. And 
> > > > > > > > we should obey the standard unconditionally.
> > > > > > > > Plus, there might be situations where user may require it 
> > > > > > > > explicitly. For example, the device pointer is dereferenced in 
> > > > > > > > one of the clauses for the subregions but in the deeper 
> > > > > > > > subregion it might be used as a private pointer. Why we should 
> > > > > > > > emit a warning here?
> > > > > > > If you have a different situation, e.g., the one you describe, 
> > > > > > > you should not have a warning. Though, that is not the point. If 
> > > > > > > you have the situation above (single directive), as per my 
> > > > > > > reasoning, there doesn't seem to be a sensible use case. If you 
> > > > > > > have one and we should create an explicit test for it.
> > > > > > > 
> > > > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > > > And we should obey the standard unconditionally.
> > > > > > > Nobody says we should not. We warn people all the time even if it 
> > > > > > > is valid code.
> > > > > > Warnings may prevent successful compilation in some cases, e.g. 
> > > > > > when warnings are treated as errors. Why we should emit a warning 
> > > > > > if the construct is allowed by the standard? Ask to change the 
> > > > > > standard if you did not agree with it.
> > > > > Warnings are specifically for const

[PATCH] D65556: Phabricator D49466

[Dan McGregor via Phabricator via cfe-commits]

dankm added a comment.

In D65556#1610001 , @lebedev.ri wrote:

> Please fix patch title and description


Sorry, I'm going to delete this. It was supposed to be a new patchset on D49466 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65556



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

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 214159.
ymandel marked an inline comment as done.
ymandel added a comment.

Added test; adjusted comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65944

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -671,6 +671,23 @@
   LanguageMode::Cxx11OrLater));
 }
 
+TEST(Matcher, IgnoresElidableInVarInit) {
+  auto matcher =
+  varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(;
+  EXPECT_TRUE(matches("struct H {};"
+  "H G();"
+  "void f(H D = G()) {"
+  "  return;"
+  "}",
+  matcher, LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(matches("struct H {};"
+  "H G();"
+  "void f() {"
+  "  H D = G();"
+  "}",
+  matcher, LanguageMode::Cxx11OrLater));
+}
+
 TEST(Matcher, BindTheSameNameInAlternatives) {
   StatementMatcher matcher = anyOf(
 binaryOperator(hasOperatorName("+"),
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -6533,14 +6533,15 @@
 }
 
 /// Matches expressions that match InnerMatcher that are possibly wrapped in an
-/// elidable constructor.
+/// elidable constructor and other corresponding bookkeeping nodes.
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used to
-/// skip those elidable constructors.
+/// In C++17, elidable copy constructors are no longer being generated in the
+/// AST as it is not permitted by the standard. They are, however, part of the
+/// AST in C++14 and earlier. So, a matcher must abstract over these differences
+/// to work in all language modes. This matcher skips elidable constructor-call
+/// AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+/// various implicit nodes inside the constructor calls, all of which will not
+/// appear in the C++17 AST.
 ///
 /// Given
 ///
@@ -6552,13 +6553,20 @@
 /// }
 /// \endcode
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).
 AST_MATCHER_P(Expr, ignoringElidableConstructorCall,
   ast_matchers::internal::Matcher, InnerMatcher) {
-  if (const auto *CtorExpr = dyn_cast(&Node)) {
+  // E tracks the node that we are examining.
+  const Expr *E = &Node;
+  // If present, remove an outer `ExprWithCleanups` corresponding to the
+  // underlying `CXXConstructExpr`. This check won't cover all cases of added
+  // `ExprWithCleanups` corresponding to `CXXConstructExpr` nodes (because the
+  // EWC is placed on the outermost node of the expression, which this may not
+  // be), but, it still improves the coverage of this matcher.
+  if (const auto *CleanupsExpr = dyn_cast(&Node))
+E = CleanupsExpr->getSubExpr();
+  if (const auto *CtorExpr = dyn_cast(E)) {
 if (CtorExpr->isElidable()) {
   if (const auto *MaterializeTemp =
   dyn_cast(CtorExpr->getArg(0))) {
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -5805,14 +5805,15 @@
 
 MatcherExpr>ignoringElidableConstructorCallast_matchers::MatcherExpr> InnerMatcher
 Matches expressions that match InnerMatcher that are possibly wrapped in an
-elidable constructor.
+elidable constructor and other corresponding bookkeeping nodes.
 
-In C++17 copy elidable constructors are no longer being
-generated in the AST as it is not permitted by the standard. They are
-however part of the AST in C++14 and earlier. Therefore, to write a matcher
-that works in all language modes, the mat

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

Thanks for the comments. I struggled with the wording so your edits are 
appreciated.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6556
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).

gribozavr wrote:
> Could you add this matcher as a test?
Done

Also, I noticed that the matcher is classified as "Traversal" yet its tests are 
in ASTMatchersNarrowingTest.cpp.  Any objection to my moving the tests to 
ASTMatchersTraversalTest.cpp as a followup patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65944



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

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1621168 , @jdenny wrote:

> In D65835#1619560 , @ABataev wrote:
>
> > In D65835#1619549 , @jdenny wrote:
> >
> > > In D65835#1619526 , @ABataev 
> > > wrote:
> > >
> > > > > Maybe, but I haven't found any statement in either version that 
> > > > > states that map restrictions apply to is_device_ptr.
> > > >
> > > > `is_device_ptr` is a kind of mapping clause. I assume we can extend the 
> > > > restrictions for `map` clause to this clause too.
> > >
> > >
> > > I'd like to understand this better.  Is there something from the spec we 
> > > can quote in the code?
> >
> >
> > See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives
>
>
> I looked again.  I'm still not finding any text in that section that implies 
> is_device_ptr follows the same restrictions as map.  Can you please cite 
> specific lines of text instead of an entire section?  Thanks for your help.


Ah, it is only in OpenMP 5.0 anв restrictions for the map clause are for map 
clause only. Then we should allow `is_device_ptr` with the private clauses for 
OpenMP 4.5.
Better to do this in a separate patch only for `is_device_ptr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65835



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

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.
Herald added a project: clang.

A few weeks later, I realize that since the test doesn't have a ".test" 
extension, it doesn't run on any bots. Could you rename it to leakfiles.test 
(or any other fitting extension listed in clang/test/lit.cfg.py's 
config.suffixes list)?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57165



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

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6556
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).

ymandel wrote:
> gribozavr wrote:
> > Could you add this matcher as a test?
> Done
> 
> Also, I noticed that the matcher is classified as "Traversal" yet its tests 
> are in ASTMatchersNarrowingTest.cpp.  Any objection to my moving the tests to 
> ASTMatchersTraversalTest.cpp as a followup patch?
That would make a lot of sense, thank you!



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65944



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 214168.
modocache marked 3 inline comments as done.
modocache added a comment.

Thanks for the review, @Quuxplusone! I addressed your test comments, but the 
'co_yield' test is something I think I'll need to deal with separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65043

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -3803,10 +3803,13 @@
   "if (aa.aa(\n"
   "aa) == 5) {\n"
   "}");
+
+  FormatStyle Cpp2a = getLLVMStyle();
+  Cpp2a.Standard = FormatStyle::LS_Cpp2a;
   verifyFormat(
   "if (a(\n"
   "aa) <=> 5) {\n"
-  "}");
+  "}", Cpp2a);
   // Even explicit parentheses stress the precedence enough to make the
   // additional break unnecessary.
   verifyFormat("if ((a +\n"
@@ -3829,7 +3832,7 @@
   verifyFormat("if (a +\n"
"aa <=>\n"
"5) {\n"
-   "}");
+   "}", Cpp2a);
 
   FormatStyle OnePerLine = getLLVMStyle();
   OnePerLine.BinPackParameters = false;
@@ -11839,8 +11842,14 @@
   Style.Standard = FormatStyle::LS_Auto;
   CHECK_PARSE("Standard: Cpp03", Standard, FormatStyle::LS_Cpp03);
   CHECK_PARSE("Standard: Cpp11", Standard, FormatStyle::LS_Cpp11);
+  CHECK_PARSE("Standard: Cpp14", Standard, FormatStyle::LS_Cpp14);
+  CHECK_PARSE("Standard: Cpp17", Standard, FormatStyle::LS_Cpp17);
+  CHECK_PARSE("Standard: Cpp2a", Standard, FormatStyle::LS_Cpp2a);
   CHECK_PARSE("Standard: C++03", Standard, FormatStyle::LS_Cpp03);
   CHECK_PARSE("Standard: C++11", Standard, FormatStyle::LS_Cpp11);
+  CHECK_PARSE("Standard: C++14", Standard, FormatStyle::LS_Cpp14);
+  CHECK_PARSE("Standard: C++17", Standard, FormatStyle::LS_Cpp17);
+  CHECK_PARSE("Standard: C++2a", Standard, FormatStyle::LS_Cpp2a);
   CHECK_PARSE("Standard: Auto", Standard, FormatStyle::LS_Auto);
 
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
@@ -13770,6 +13779,18 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
+TEST_F(FormatTest, Coroutines) {
+  FormatStyle Cpp2a = getLLVMStyle();
+  Cpp2a.Standard = FormatStyle::LS_Cpp2a;
+
+  // 'co_yield' is treated as an identifier in standards below C++2a, and so
+  // the increment is interpreted as a postfix on that identifier.
+  // In C++2a, it is interpreted as a prefix increment on 'it'.
+  verifyFormat("f(co_yield ++it);", Cpp2a);
+
+  verifyFormat("co_await []() -> g { co_return; }();", Cpp2a);
+}
+
 TEST_F(FormatTest, FileAndCode) {
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.cc", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2862,7 +2862,7 @@
 (Style.Language == FormatStyle::LK_Proto && Left.is(TT_DictLiteral)))
   return !Style.Cpp11BracedListStyle;
 return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-   (Style.Standard < FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+   (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }
   if (Right.isOneOf(tok::arrow, tok::arrowstar, tok::periodstar) ||
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
@@ -2881,7 +2881,7 @@
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
   if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
 return (Left.is(TT_TemplateOpener) &&
-Style.Standard < FormatStyle::LS_Cpp11) ||
+Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
   TT_TemplateOpener)) ||
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -71,6 +71,12 @@
 IO.enumCase(Value, "C++03", FormatStyle::LS_Cpp03);
 IO.enumCase(Value, "Cpp11", FormatStyle::LS_Cpp11);
 IO.enumCase(Value, "C++11", FormatStyle::LS_Cpp11);
+IO.enumCase(Value, "Cpp14", FormatStyle::L

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:13780
+  // In C++2a, it is interpreted as a prefix increment on 'i'.
+  verifyFormat("co_yield++ i;");
+  verifyFormat("co_yield ++i;", Cpp2a);

Quuxplusone wrote:
> My comment https://reviews.llvm.org/D65043#inline-582865 still stands. Please 
> just write
> 
> verifyFormat("f(co_yield - 1);");
> verifyFormat("f(co_yield -1);", Cpp2a);
> 
Unfortunately I think there's a separate issue here, that `co_yield -1` is 
formatted as `co_yield - 1` no matter which C++ standard is used. I'd like to 
address that in a separate commit, if that's alright. For now, I removed the 
test verifying the C++17 behavior, and instead only test the correct C++20 
formatting of `co_yield ++it`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65043



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

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

jdenny wrote:
> hfinkel wrote:
> > ABataev wrote:
> > > hfinkel wrote:
> > > > ABataev wrote:
> > > > > hfinkel wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > I think this should cause an error or at least a 
> > > > > > > > > > > > > > warning. Telling the compiler `ps` is a device 
> > > > > > > > > > > > > > pointer only to create a local uninitialized 
> > > > > > > > > > > > > > shadowing variable seems like an error to me.
> > > > > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy 
> > > > > > > > > > > > > must be created in the context of the parallel 
> > > > > > > > > > > > > region, not the target region. So, for OpenMP 5.0 we 
> > > > > > > > > > > > > should not emit error here.
> > > > > > > > > > > > What does that mean and how does that affect my 
> > > > > > > > > > > > reasoning?
> > > > > > > > > > > It means, that for OpenMP 5.0 we should emit a 
> > > > > > > > > > > warning/error here. It is allowed according to the 
> > > > > > > > > > > standard, we should allow it too.
> > > > > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > > > > 
> > > > > > > > > > The last answer contradicts what you said earlier. I expect 
> > > > > > > > > > there is a *not* missing, correct?
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Assuming you do not want an error, which is fine, I still 
> > > > > > > > > > think a warning is appropriate as it seems to me there is 
> > > > > > > > > > never a reason to have a `is_device_ptr` clause for a 
> > > > > > > > > > private value. I mean, it is either a bug by the 
> > > > > > > > > > programmer, e.g., 5 letters of `firstprivate` went missing, 
> > > > > > > > > > or simply nonsensical code for which we warn in other 
> > > > > > > > > > situations as well.
> > > > > > > > > Missed `not`.
> > > > > > > > > These kind of construct are explicitly allowed in OpenMP. And 
> > > > > > > > > we should obey the standard unconditionally.
> > > > > > > > > Plus, there might be situations where user may require it 
> > > > > > > > > explicitly. For example, the device pointer is dereferenced 
> > > > > > > > > in one of the clauses for the subregions but in the deeper 
> > > > > > > > > subregion it might be used as a private pointer. Why we 
> > > > > > > > > should emit a warning here?
> > > > > > > > If you have a different situation, e.g., the one you describe, 
> > > > > > > > you should not have a warning. Though, that is not the point. 
> > > > > > > > If you have the situation above (single directive), as per my 
> > > > > > > > reasoning, there doesn't seem to be a sensible use case. If you 
> > > > > > > > have one and we should create an explicit test for it.
> > > > > > > > 
> > > > > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > > > > And we should obey the standard unconditionally.
> > > > > > > > Nobody says we should not. We warn people all the time even if 
> > > > > > > > it is valid code.
> > > > > > > Warnings may prevent successful compilation in some cases, e.g. 
> > > > > > > when warnings are treated as errors. Why we should emit a warning 
> > > > > > > if the construct is allowed by the standard? Ask to change the 
> > > > > > > standard if you did not agree with it.
> > > > > > Warnings are specifically for constructs which are legal, but 
> > > > > > likely wrong (i.e., will not do what the user likely intended). 
> > > > > > Treating warnings as errors is not a conforming compilation mode - 
> > > > > > by design (specifically because that will reject conforming 
> > > > > > programs). Thus...
> > > > > > 
> > > > > > > Why we should emit a warning if the construct is allowed by the 
> > > > > > > standard? Ask to change the standard if you did not agree with it.
> > > > > > 
> > > > > > This is the wrong way to approach this. Warnings are specifically 
> > > > > > for legal code. They help users prevent errors, however, in cases 
> > > > > > where that legal code is likely problematic or won't do what the 
> > > > > > user intends.
> > > > > > 
> > > > > Ok, we could emit wqrn

[PATCH] D65956: clang: Diag running out of file handles while looking for files

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: rnk.

clang would only print "file not found" when it's unable to find a
header file.  If the reason for that is a file handle leak, that's not a
very useful error message.  For errors that aren't in a small whitelist
("file not found", "file is directory"), print an error with the
strerror() output.

This changes behavior in corner cases: If clang was out of file handles
while looking in one -I dir but then suddenly wasn't when looking in the
next -I dir, and both directories contained a file with the desired
name, previously we'd silently return the file from the second
directory. For this reason, it's important to ignore "is a directory"
for this new diag: if a file foo/foo exists and -I -Ifoo are passed, an
include of "foo" should successfully open file "foo" in directory "foo/"
instead of complaining that "./foo" is a directory.

No test since we mostly hit this when there's a handle leak somewhere,
and currently there isn't one. I manually tested this with the repro
steps in comment 2 on the bug below.

Fixes PR42524.


https://reviews.llvm.org/D65956

Files:
  clang/lib/Lex/HeaderSearch.cpp


Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -309,9 +309,18 @@
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  auto File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
-  if (!File)
+  llvm::ErrorOr File =
+  getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  if (!File) {
+// For rare, surprising errors (e.g. "out of file handles"), diag the EC
+// message.
+std::error_code EC = File.getError();
+if (EC != std::errc::no_such_file_or_directory &&
+EC != std::errc::is_a_directory) {
+  Diags.Report(IncludeLoc, diag::err_cannot_open_file) << EC.message();
+}
 return nullptr;
+  }
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(*File, Dir ? Dir : (*File)->getDir(),


Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -309,9 +309,18 @@
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  auto File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
-  if (!File)
+  llvm::ErrorOr File =
+  getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  if (!File) {
+// For rare, surprising errors (e.g. "out of file handles"), diag the EC
+// message.
+std::error_code EC = File.getError();
+if (EC != std::errc::no_such_file_or_directory &&
+EC != std::errc::is_a_directory) {
+  Diags.Report(IncludeLoc, diag::err_cannot_open_file) << EC.message();
+}
 return nullptr;
+  }
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(*File, Dir ? Dir : (*File)->getDir(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65456: [OpenCL] Add generic type handling for builtin functions

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 214169.
svenvh added a comment.

- Move checking of GenType compatibility from SemaLookup to TableGen emitter.
- Provide more elaborate explanation about combining GenTypes in a declaration.
- Add `max`/`min` definitions to cover "sgentype" behavior and add test for 
`max`.
- Minor scattered comment improvements.

Combining GenTypes in an incorrect way will now produce an error from TableGen, 
e.g.:

  OpenCLBuiltins.td:1601:1: error: number of vector sizes should be equal or 1 
for all gentypes in a declaration
  def : Builtin<"crashme", [GenTypeFloatVecNoScalar, GenTypeFloatVecAndScalar]>
  ^
  
  OpenCLBuiltins.td:1602:1: error: number of types should be equal or 1 for all 
gentypes in a declaration
  def : Builtin<"crashme2", [IGenTypeN, FGenTypeN]>;
  ^


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

https://reviews.llvm.org/D65456

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -15,12 +15,39 @@
 //
 // For a successful lookup of e.g. the "cos" builtin, isOpenCLBuiltin("cos")
 // returns a pair .
-// OpenCLBuiltins[Index] to OpenCLBuiltins[Index + Len] contains the pairs
+// BuiltinTable[Index] to BuiltinTable[Index + Len] contains the pairs
 //  of the overloads of "cos".
-// OpenCLSignature[SigIndex] to OpenCLSignature[SigIndex + SigLen] contains
-// one of the signatures of "cos". The OpenCLSignature entry can be
-// referenced by other functions, i.e. "sin", since multiple OpenCL builtins
-// share the same signature.
+// SignatureTable[SigIndex] to SignatureTable[SigIndex + SigLen] contains
+// one of the signatures of "cos". The SignatureTable entry can be
+// referenced by other functions, e.g. "sin", to exploit the fact that
+// many OpenCL builtins share the same signature.
+//
+// The file generated by this TableGen emitter contains the following:
+//
+//  * Structs and enums to represent types and function signatures.
+//
+//  * OpenCLTypeStruct TypeTable[]
+//Type information for return types and arguments.
+//
+//  * unsigned SignatureTable[]
+//A list of types representing function signatures.  Each entry is an index
+//into the above TypeTable.  Multiple entries following each other form a
+//signature, where the first entry is the return type and subsequent
+//entries are the argument types.
+//
+//  * OpenCLBuiltinStruct BuiltinTable[]
+//Each entry represents one overload of an OpenCL builtin function and
+//consists of an index into the SignatureTable and the number of arguments.
+//
+//  * std::pair isOpenCLBuiltin(llvm::StringRef Name)
+//Find out whether a string matches an existing OpenCL builtin function
+//name and return an index into BuiltinTable and the number of overloads.
+//
+//  * void OCL2Qual(ASTContext&, OpenCLTypeStruct, std::vector&)
+//Convert an OpenCLTypeStruct type to a list of QualType instances.
+//One OpenCLTypeStruct can represent multiple types, primarily when using
+//GenTypes.
+//
 //===--===//
 
 #include "llvm/ADT/MapVector.h"
@@ -57,34 +84,47 @@
   // The output file.
   raw_ostream &OS;
 
-  // Emit the enums and structs.
+  // Helper function for BuiltinNameEmitter::EmitDeclarations.  Generate enum
+  // definitions in the Output string parameter, and save their Record instances
+  // in the List parameter.
+  // \param Types (in) List containing the Types to extract.
+  // \param TypesSeen (inout) List containing the Types already extracted.
+  // \param Output (out) String containing the enums to emit in the output file.
+  // \param List (out) List containing the extracted Types, except the Types in
+  //TypesSeen.
+  void ExtractEnumTypes(std::vector &Types,
+StringMap &TypesSeen, std::string &Output,
+std::vector &List);
+
+  // Emit the enum or struct used in the generated file.
+  // Populate the TypeList at the same time.
   void EmitDeclarations();
 
-  // Parse the Records generated by TableGen and populate OverloadInfo and
-  // SignatureSet.
+  // Parse the Records generated by TableGen to populate the SignaturesList,
+  // FctOverloadMap and TypeMap.
   void GetOverloads();
 
-  // Emit the OpenCLSignature table. This table contains all possible
-  // signatures, and is a struct OpenCLType. A signature is composed of a
-  // return type (mandatory), followed by zero or more argument types.
+  // Emit the TypeTable containing all types used by OpenCL builtins.
+  void EmitTypeTable();
+
+  // Emit the SignatureTable. This table contains all the pos

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:820
+  llvm::DenseMap>
+  CompatibleAliases;

`unordered_set`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[PATCH] D65456: [OpenCL] Add generic type handling for builtin functions

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh marked 6 inline comments as done.
svenvh added inline comments.



Comment at: clang/lib/Sema/OpenCLBuiltins.td:116
+// combination of Types and vector sizes.
+//
+// E.g.: If TypeListField =  and VectorList = <1, 2, 4>, then

Pierre wrote:
> Maybe it would be worth adding more information on how to use GenTypes. I was 
> thinking of something like this : 
> 
> When using multiple generic types :
>   * The maximal cardinal of the used  GenTypes must be the PGCM of all the 
> cardinals.
>   * The generic types combine as if it was an array like 
> GenType[Type][VecSize].
> I.e: With GT1 = [half, <1, 2>] and GT2 = [float, int, <1, 2>], they will 
> combine as
> , , , 
> The maximal cardinal of GT1 and GT2 is 4 (= 2 types * 2 vector sizes).
> 4 is the PGCM of GT1 (=2) and GT2 (=4).
I've added some rules about combining GenTypes in the comment below based on 
your example.



Comment at: clang/lib/Sema/SemaLookup.cpp:708
+  }
+  for (unsigned Index = 0; Index < ArgTypes.size(); Index++) {
+unsigned TypeCntAtIndex = ArgTypes[Index].size();

svenvh wrote:
> Anastasia wrote:
> > I don't get this logic?
> While trying to clarify this, I realized this checking should probably be 
> moved to the TableGen emitter, as this is checking validity of the .td input 
> so ideally we should check that at compiler compile time.  @Pierre mentioned 
> that this might not be trivial, but I'll have a look at it.
The checking has been moved into `ClangOpenCLBuiltinEmitter.cpp` now, see 
`VerifySignature`.



Comment at: clang/lib/Sema/SemaLookup.cpp:817
   }
-  New->setParams(Params);
+  NewOpenCLBuiltin->addAttr(OverloadableAttr::CreateImplicit(Context));
+  LR.addDecl(NewOpenCLBuiltin);

Anastasia wrote:
> I guess this should be done conditionally for C++ mode. But perhaps we don't 
> have to do this now. It might be worth adding a FIXME.
Done.


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

https://reviews.llvm.org/D65456



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

[PATCH] D62960: Add SVE opaque built-in types

[John McCall via Phabricator via cfe-commits]

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

LGTM.




Comment at: lib/AST/ASTContext.cpp:1974
+  break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
 }

rsandifo-arm wrote:
> rjmccall wrote:
> > Why do SVE predicates have 16-bit alignment?  Should this be 128-bit 
> > (16-*byte*)?
> > 
> > I guess these alignments are reasonable to hard-code here since they're 
> > target-specific for now.  That might be worth including in the comment.
> Yeah, in retrospect this is sorely lacking a comment.  The reason for using 
> 16 is that there is one predicate bit for each vector byte, so the predicate 
> size is a runtime multiple of 16 bits.  I've added a comment to say that and 
> added a reference to the ABI that defines the alignments.
Thanks!

I'm surprised predicates don't require a higher alignment, even if it'd be 
excessive for very short vectors, but if that's the ABI rule, so be it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960



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

[PATCH] D65956: clang: Diag running out of file handles while looking for files

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm


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

https://reviews.llvm.org/D65956



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

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Denis Nikitin via Phabricator via cfe-commits]

denik commandeered this revision.
denik added a reviewer: yunlian.
denik added a comment.
Herald added a project: clang.

Taking ownership of the change as Yunlian is no longer working on this CL.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52524



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

[PATCH] D65853: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

In D65853#1619801 , @plotfi wrote:

> I tested this out. Seems to work fine, and print the Expected's Error. I am 
> fine with it if @compnerd and @lhames and @jkorous are cool with it.


@compnerd @lhames @jkorous ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65853



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

[clang-tools-extra] r368313 - [clang-doc] Protect Index with mutex during reducing and generation stage

[Diego Astiazaran via cfe-commits]

Author: diegoastiazaran
Date: Thu Aug  8 10:14:17 2019
New Revision: 368313

URL: http://llvm.org/viewvc/llvm-project?rev=368313&view=rev
Log:
[clang-doc] Protect Index with mutex during reducing and generation stage

Idx in ClangDocContext instance was being modified by multiple threads
causing a seg fault.
A mutex is added to avoid this.

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

Modified:
clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp

Modified: clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp?rev=368313&r1=368312&r2=368313&view=diff
==
--- clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp (original)
+++ clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp Thu Aug  8 10:14:17 
2019
@@ -36,6 +36,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
@@ -246,6 +247,7 @@ int main(int argc, const char **argv) {
   llvm::outs() << "Reducing " << USRToBitcode.size() << " infos...\n";
   std::atomic Error;
   Error = false;
+  llvm::sys::Mutex IndexMutex;
   // ExecutorConcurrency is a flag exposed by AllTUsExecution.h
   llvm::ThreadPool Pool(ExecutorConcurrency == 0 ? llvm::hardware_concurrency()
  : ExecutorConcurrency);
@@ -289,8 +291,10 @@ int main(int argc, const char **argv) {
 return;
   }
 
+  IndexMutex.lock();
   // Add a reference to this Info in the Index
   clang::doc::Generator::addInfoToIndex(CDCtx.Idx, I);
+  IndexMutex.unlock();
 
   if (auto Err = G->get()->generateDocForInfo(I, InfoOS, CDCtx))
 llvm::errs() << toString(std::move(Err)) << "\n";


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

[PATCH] D65915: [clang-doc] Protect Index with mutex during reducing and generation stage

[Diego Astiazarán via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368313: [clang-doc] Protect Index with mutex during reducing 
and generation stage (authored by DiegoAstiazaran, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65915?vs=214047&id=214178#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65915

Files:
  clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp


Index: clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
===
--- clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
+++ clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
@@ -36,6 +36,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
@@ -246,6 +247,7 @@
   llvm::outs() << "Reducing " << USRToBitcode.size() << " infos...\n";
   std::atomic Error;
   Error = false;
+  llvm::sys::Mutex IndexMutex;
   // ExecutorConcurrency is a flag exposed by AllTUsExecution.h
   llvm::ThreadPool Pool(ExecutorConcurrency == 0 ? llvm::hardware_concurrency()
  : ExecutorConcurrency);
@@ -289,8 +291,10 @@
 return;
   }
 
+  IndexMutex.lock();
   // Add a reference to this Info in the Index
   clang::doc::Generator::addInfoToIndex(CDCtx.Idx, I);
+  IndexMutex.unlock();
 
   if (auto Err = G->get()->generateDocForInfo(I, InfoOS, CDCtx))
 llvm::errs() << toString(std::move(Err)) << "\n";


Index: clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
===
--- clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
+++ clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
@@ -36,6 +36,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
@@ -246,6 +247,7 @@
   llvm::outs() << "Reducing " << USRToBitcode.size() << " infos...\n";
   std::atomic Error;
   Error = false;
+  llvm::sys::Mutex IndexMutex;
   // ExecutorConcurrency is a flag exposed by AllTUsExecution.h
   llvm::ThreadPool Pool(ExecutorConcurrency == 0 ? llvm::hardware_concurrency()
  : ExecutorConcurrency);
@@ -289,8 +291,10 @@
 return;
   }
 
+  IndexMutex.lock();
   // Add a reference to this Info in the Index
   clang::doc::Generator::addInfoToIndex(CDCtx.Idx, I);
+  IndexMutex.unlock();
 
   if (auto Err = G->get()->generateDocForInfo(I, InfoOS, CDCtx))
 llvm::errs() << toString(std::move(Err)) << "\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61879: WIP: Prototype of DSE optimizations for -ftrivial-auto-var-init

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka updated this revision to Diff 214183.
vitalybuka added a comment.

Fix https://godbolt.org/z/-PinQP


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61879

Files:
  clang/test/CodeGenCXX/member-function-pointer-calls.cpp
  clang/test/CodeGenCXX/union-tbaa2.cpp
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/LinkAllPasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Scalar/DeadStoreEliminationExp.h
  llvm/lib/Analysis/Analysis.cpp
  llvm/lib/IR/AsmWriter.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Scalar/CMakeLists.txt
  llvm/lib/Transforms/Scalar/DeadStoreEliminationExp.cpp
  llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.cpp
  llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobal.h
  llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobalArgInfoGen.h
  llvm/lib/Transforms/Scalar/DeadStoreEliminationExpGlobalGUIDListGen.h
  llvm/lib/Transforms/Scalar/Scalar.cpp
  llvm/test/ObjectYAML/MachO/DWARF-debug_line.yaml
  llvm/test/Other/dsae.ll
  llvm/test/Other/dse-exp-struct.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Other/pass-pipelines.ll
  llvm/test/Transforms/Coroutines/ArgAddr.ll
  llvm/test/Transforms/Coroutines/coro-split-01.ll
  llvm/test/Transforms/Coroutines/ex0.ll
  llvm/test/Transforms/Coroutines/ex1.ll
  llvm/test/Transforms/Coroutines/ex2.ll
  llvm/test/Transforms/Coroutines/ex3.ll
  llvm/test/Transforms/Coroutines/ex4.ll
  llvm/test/Transforms/Coroutines/ex5.ll
  llvm/test/Transforms/Coroutines/phi-coro-end.ll
  llvm/test/Transforms/Coroutines/restart-trigger.ll
  llvm/utils/gn/build/BUILD.gn
  llvm/utils/gn/secondary/llvm/lib/Transforms/Scalar/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Scalar/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Scalar/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Scalar/BUILD.gn
@@ -19,6 +19,8 @@
 "CorrelatedValuePropagation.cpp",
 "DCE.cpp",
 "DeadStoreElimination.cpp",
+"DeadStoreEliminationExp.cpp",
+"DeadStoreEliminationExpGlobal.cpp",
 "DivRemPairs.cpp",
 "EarlyCSE.cpp",
 "FlattenCFGPass.cpp",
Index: llvm/utils/gn/build/BUILD.gn
===
--- llvm/utils/gn/build/BUILD.gn
+++ llvm/utils/gn/build/BUILD.gn
@@ -39,7 +39,7 @@
   cflags += [ "-g" ]
 }
 if (is_optimized) {
-  cflags += [ "-Oz" ]
+  cflags += [ "-O3" ]
 }
 if (is_new_pm) {
   cflags += [ 
Index: llvm/test/Transforms/Coroutines/restart-trigger.ll
===
--- llvm/test/Transforms/Coroutines/restart-trigger.ll
+++ llvm/test/Transforms/Coroutines/restart-trigger.ll
@@ -7,6 +7,8 @@
 ; CHECK:  CoroSplit: Processing coroutine 'f' state: 0
 ; CHECK-NEXT: CoroSplit: Processing coroutine 'f' state: 1
 
+; REQUIRES: abcd
+
 define void @f() {
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
   %size = call i32 @llvm.coro.size.i32()
Index: llvm/test/Transforms/Coroutines/phi-coro-end.ll
===
--- llvm/test/Transforms/Coroutines/phi-coro-end.ll
+++ llvm/test/Transforms/Coroutines/phi-coro-end.ll
@@ -1,6 +1,8 @@
 ; Verify that we correctly handle suspend when the coro.end block contains phi
 ; RUN: opt < %s -O2 -enable-coroutines -S | FileCheck %s
 
+; REQUIRES: abcd
+
 define i8* @f(i32 %n) {
 entry:
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
Index: llvm/test/Transforms/Coroutines/ex5.ll
===
--- llvm/test/Transforms/Coroutines/ex5.ll
+++ llvm/test/Transforms/Coroutines/ex5.ll
@@ -1,6 +1,8 @@
 ; Fifth example from Doc/Coroutines.rst (final suspend)
 ; RUN: opt < %s -O2 -enable-coroutines -S | FileCheck %s
 
+; REQUIRES: abcd
+
 define i8* @f(i32 %n) {
 entry:
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
Index: llvm/test/Transforms/Coroutines/ex4.ll
===
--- llvm/test/Transforms/Coroutines/ex4.ll
+++ llvm/test/Transforms/Coroutines/ex4.ll
@@ -1,6 +1,8 @@
 ; Fourth example from Doc/Coroutines.rst (coroutine promise)
 ; RUN: opt < %s -O2 -enable-coroutines -S | FileCheck %s
 
+; REQUIRES: abcd
+
 define i8* @f(i32 %n) {
 entry:
   %promise = alloca i32
Index: llvm/test/Transforms/Coroutines/ex3.ll
===
--- llvm/test/Transforms/Coroutines/ex3.ll
+++ llvm/test/Transforms/Coroutines/ex3.ll
@@ -1,6 +1,8 @@
 ; Th

[PATCH] D65575: [analyzer] Mention whether an event is about a condition in a bug report part 1

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D65575#1611013 , @NoQ wrote:

> Fantastic! Let's open the wording bikeshed season?
>
> I suspect that a simple "(The) Value -> Condition value" change would have 
> worked better.
>
> Another variant: "Value ..., which participates in a condition later".


Yea, I kinda prefer a more uniform indication as to whether we're explaining a 
condition or "THE value". While I personally took a unique approach in 
evaluating analysis results (my eye was hunting for the changes I made 
specifically), I did find each function call in the bug report super easy to 
understand:
F9758175: image.png 
See how this function call screams what it is about? Now, condition tracking is 
inherently imperfect (like bug report construction as a whole), and whenever I 
feel like the notes added by it provide little value, simply glancing at the 
notes can tell whether I should observe that function call or not.

I think it isn't crucial of getting rid of the "The" prefix, if we append ", 
which participates in a condition later" (which sounds so much better than what 
I added in this patch), so maybe changing `WillBeUsedForACondition` to that 
would be good enough. However, as I said, I realize that the way I looked at 
these results was a lot different than how the average user will do so, so I'm 
totally open on this topic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65575



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

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/docs/OpenMPSupport.rst:222-226
+| device extension | clause: reverse_offload   
   | :good:`done` | D52780  
   |
++--+--+--++
+| device extension | clause: atomic_default_mem_order  
   | :good:`done` | D53513  
   |
++--+--+--++
+| device extension | clause: dynamic_allocators
   | :good:`done` | D53079  
   |

Hahnfeld wrote:
> Do we have CodeGen for this? I only remember Sema support...
> 
> Also, `reverse_offload` is listed again below as `unclaimed`.
We don't have codegen for this. Same for the reverse_offload and 
atomic_default_mem_order


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64375



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

r368319 - [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Yitzhak Mandelbaum via cfe-commits]

Author: ymandel
Date: Thu Aug  8 10:41:44 2019
New Revision: 368319

URL: http://llvm.org/viewvc/llvm-project?rev=368319&view=rev
Log:
[clang] Update `ignoringElidableConstructorCall` matcher to ignore 
`ExprWithCleanups`.

Summary:
The `ExprWithCleanups` node is added to the AST along with the elidable
CXXConstructExpr.  If it is the outermost node of the node being matched, ignore
it as well.

Reviewers: gribozavr

Subscribers: cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/docs/LibASTMatchersReference.html
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=368319&r1=368318&r2=368319&view=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Thu Aug  8 10:41:44 2019
@@ -5805,14 +5805,15 @@ Example matches x (matcher = expr(hasTyp
 
 MatcherExpr>ignoringElidableConstructorCallast_matchers::MatcherExpr> 
InnerMatcher
 Matches expressions that match 
InnerMatcher that are possibly wrapped in an
-elidable constructor.
+elidable constructor and other corresponding bookkeeping nodes.
 
-In C++17 copy elidable constructors are no longer being
-generated in the AST as it is not permitted by the standard. They are
-however part of the AST in C++14 and earlier. Therefore, to write a matcher
-that works in all language modes, the matcher has to skip elidable
-constructor AST nodes if they appear in the AST. This matcher can be used to
-skip those elidable constructors.
+In C++17, elidable copy constructors are no longer being generated in the
+AST as it is not permitted by the standard. They are, however, part of the
+AST in C++14 and earlier. So, a matcher must abstract over these differences
+to work in all language modes. This matcher skips elidable constructor-call
+AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+various implicit nodes inside the constructor calls, all of which will not
+appear in the C++17 AST.
 
 Given
 
@@ -5822,10 +5823,8 @@ void f() {
   H D = G();
 }
 
-``varDecl(hasInitializer(any(
-  ignoringElidableConstructorCall(callExpr()),
-  exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-matches ``H D = G()``
+``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+matches ``H D = G()`` in C++11 through C++17 (and beyond).
 
 
 

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=368319&r1=368318&r2=368319&view=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Thu Aug  8 10:41:44 2019
@@ -6533,14 +6533,15 @@ AST_MATCHER(FunctionDecl, hasTrailingRet
 }
 
 /// Matches expressions that match InnerMatcher that are possibly wrapped in an
-/// elidable constructor.
+/// elidable constructor and other corresponding bookkeeping nodes.
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used 
to
-/// skip those elidable constructors.
+/// In C++17, elidable copy constructors are no longer being generated in the
+/// AST as it is not permitted by the standard. They are, however, part of the
+/// AST in C++14 and earlier. So, a matcher must abstract over these 
differences
+/// to work in all language modes. This matcher skips elidable constructor-call
+/// AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+/// various implicit nodes inside the constructor calls, all of which will not
+/// appear in the C++17 AST.
 ///
 /// Given
 ///
@@ -6552,13 +6553,20 @@ AST_MATCHER(FunctionDecl, hasTrailingRet
 /// }
 /// \endcode
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).
 AST_MATCHER_P(Expr, ignoringElidableConstructorCall,
   ast_matchers::internal::Matcher, InnerMatcher) {
-  if (const auto *CtorExpr = dyn_cast(&Node)) {
+  // E trac

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 214188.
ymandel marked an inline comment as done.
ymandel added a comment.

updated HTML docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65944

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -671,6 +671,23 @@
   LanguageMode::Cxx11OrLater));
 }
 
+TEST(Matcher, IgnoresElidableInVarInit) {
+  auto matcher =
+  varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(;
+  EXPECT_TRUE(matches("struct H {};"
+  "H G();"
+  "void f(H D = G()) {"
+  "  return;"
+  "}",
+  matcher, LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(matches("struct H {};"
+  "H G();"
+  "void f() {"
+  "  H D = G();"
+  "}",
+  matcher, LanguageMode::Cxx11OrLater));
+}
+
 TEST(Matcher, BindTheSameNameInAlternatives) {
   StatementMatcher matcher = anyOf(
 binaryOperator(hasOperatorName("+"),
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -6533,14 +6533,15 @@
 }
 
 /// Matches expressions that match InnerMatcher that are possibly wrapped in an
-/// elidable constructor.
+/// elidable constructor and other corresponding bookkeeping nodes.
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used to
-/// skip those elidable constructors.
+/// In C++17, elidable copy constructors are no longer being generated in the
+/// AST as it is not permitted by the standard. They are, however, part of the
+/// AST in C++14 and earlier. So, a matcher must abstract over these differences
+/// to work in all language modes. This matcher skips elidable constructor-call
+/// AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+/// various implicit nodes inside the constructor calls, all of which will not
+/// appear in the C++17 AST.
 ///
 /// Given
 ///
@@ -6552,13 +6553,20 @@
 /// }
 /// \endcode
 ///
-/// ``varDecl(hasInitializer(any(
-///   ignoringElidableConstructorCall(callExpr()),
-///   exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).
 AST_MATCHER_P(Expr, ignoringElidableConstructorCall,
   ast_matchers::internal::Matcher, InnerMatcher) {
-  if (const auto *CtorExpr = dyn_cast(&Node)) {
+  // E tracks the node that we are examining.
+  const Expr *E = &Node;
+  // If present, remove an outer `ExprWithCleanups` corresponding to the
+  // underlying `CXXConstructExpr`. This check won't cover all cases of added
+  // `ExprWithCleanups` corresponding to `CXXConstructExpr` nodes (because the
+  // EWC is placed on the outermost node of the expression, which this may not
+  // be), but, it still improves the coverage of this matcher.
+  if (const auto *CleanupsExpr = dyn_cast(&Node))
+E = CleanupsExpr->getSubExpr();
+  if (const auto *CtorExpr = dyn_cast(E)) {
 if (CtorExpr->isElidable()) {
   if (const auto *MaterializeTemp =
   dyn_cast(CtorExpr->getArg(0))) {
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -5805,14 +5805,15 @@
 
 MatcherExpr>ignoringElidableConstructorCallast_matchers::MatcherExpr> InnerMatcher
 Matches expressions that match InnerMatcher that are possibly wrapped in an
-elidable constructor.
+elidable constructor and other corresponding bookkeeping nodes.
 
-In C++17 copy elidable constructors are no longer being
-generated in the AST as it is not permitted by the standard. They are
-however part of the AST in C++14 and earlier. Therefore, to write a matcher
-that works in all language modes, the matcher has to 

[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

[Kristóf Umann via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:469-471
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > I'm pretty sure you can define this function only once.
> > > Note that it is in a namespace!
> > I mean, why is it in a namespace? Why not just define it once for the whole 
> > file? It's not like you're gonna be trying out different variants of 
> > `__assert_fail`.
> Yea, good point.
Actually, I kinda like how a single namespace is a single test case, an 
all-in-one package. Do you insist?


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

https://reviews.llvm.org/D65287



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

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368319: [clang] Update `ignoringElidableConstructorCall` 
matcher to ignore… (authored by ymandel, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65944?vs=214188&id=214189#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65944

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -671,6 +671,23 @@
   LanguageMode::Cxx11OrLater));
 }
 
+TEST(Matcher, IgnoresElidableInVarInit) {
+  auto matcher =
+  varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(;
+  EXPECT_TRUE(matches("struct H {};"
+  "H G();"
+  "void f(H D = G()) {"
+  "  return;"
+  "}",
+  matcher, LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(matches("struct H {};"
+  "H G();"
+  "void f() {"
+  "  H D = G();"
+  "}",
+  matcher, LanguageMode::Cxx11OrLater));
+}
+
 TEST(Matcher, BindTheSameNameInAlternatives) {
   StatementMatcher matcher = anyOf(
 binaryOperator(hasOperatorName("+"),
Index: cfe/trunk/docs/LibASTMatchersReference.html
===
--- cfe/trunk/docs/LibASTMatchersReference.html
+++ cfe/trunk/docs/LibASTMatchersReference.html
@@ -5805,14 +5805,15 @@
 
 MatcherExpr>ignoringElidableConstructorCallast_matchers::MatcherExpr> InnerMatcher
 Matches expressions that match InnerMatcher that are possibly wrapped in an
-elidable constructor.
+elidable constructor and other corresponding bookkeeping nodes.
 
-In C++17 copy elidable constructors are no longer being
-generated in the AST as it is not permitted by the standard. They are
-however part of the AST in C++14 and earlier. Therefore, to write a matcher
-that works in all language modes, the matcher has to skip elidable
-constructor AST nodes if they appear in the AST. This matcher can be used to
-skip those elidable constructors.
+In C++17, elidable copy constructors are no longer being generated in the
+AST as it is not permitted by the standard. They are, however, part of the
+AST in C++14 and earlier. So, a matcher must abstract over these differences
+to work in all language modes. This matcher skips elidable constructor-call
+AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+various implicit nodes inside the constructor calls, all of which will not
+appear in the C++17 AST.
 
 Given
 
@@ -5822,10 +5823,8 @@
   H D = G();
 }
 
-``varDecl(hasInitializer(any(
-  ignoringElidableConstructorCall(callExpr()),
-  exprWithCleanups(ignoringElidableConstructorCall(callExpr()``
-matches ``H D = G()``
+``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr(``
+matches ``H D = G()`` in C++11 through C++17 (and beyond).
 
 
 
Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -6533,14 +6533,15 @@
 }
 
 /// Matches expressions that match InnerMatcher that are possibly wrapped in an
-/// elidable constructor.
+/// elidable constructor and other corresponding bookkeeping nodes.
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used to
-/// skip those elidable constructors.
+/// In C++17, elidable copy constructors are no longer being generated in the
+/// AST as it is not permitted by the standard. They are, however, part of the
+/// AST in C++14 and earlier. So, a matcher must abstract over these differences
+/// to work in all language modes. This matcher skips elidable constructor-call
+/// AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+/// various implicit nodes inside the constructor calls, all of which will not
+/// appear in the C++17 AST.
 ///
 /// Given
 ///
@@ -6552,13 +655

[PATCH] D65853: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit

[Lang Hames via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65853



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

[PATCH] D52524: Add -Wno-poison-system-directories flag

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Couldn't cross build users just pass -nostdsysteminc to tell clang to not look 
in system header locations?


Repository:
  rC Clang

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

https://reviews.llvm.org/D52524



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

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

[Amy Huang via Phabricator via cfe-commits]

akhuang added a comment.

The llvm-dev discussion is here 
http://lists.llvm.org/pipermail/llvm-dev/2019-July/134035.html
I think the consensus is that it should be fine to change the data layout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64931



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

r368322 - clang: Diag running out of file handles while looking for files

[Nico Weber via cfe-commits]

Author: nico
Date: Thu Aug  8 10:58:32 2019
New Revision: 368322

URL: http://llvm.org/viewvc/llvm-project?rev=368322&view=rev
Log:
clang: Diag running out of file handles while looking for files

clang would only print "file not found" when it's unable to find a
header file.  If the reason for that is a file handle leak, that's not a
very useful error message.  For errors that aren't in a small whitelist
("file not found", "file is directory"), print an error with the
strerror() output.

This changes behavior in corner cases: If clang was out of file handles
while looking in one -I dir but then suddenly wasn't when looking in the
next -I dir, and both directories contained a file with the desired
name, previously we'd silently return the file from the second
directory. For this reason, it's important to ignore "is a directory"
for this new diag: if a file foo/foo exists and -I -Ifoo are passed, an
include of "foo" should successfully open file "foo" in directory "foo/"
instead of complaining that "./foo" is a directory.

No test since we mostly hit this when there's a handle leak somewhere,
and currently there isn't one. I manually tested this with the repro
steps in comment 2 on the bug below.

Fixes PR42524.

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

Modified:
cfe/trunk/lib/Lex/HeaderSearch.cpp

Modified: cfe/trunk/lib/Lex/HeaderSearch.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/HeaderSearch.cpp?rev=368322&r1=368321&r2=368322&view=diff
==
--- cfe/trunk/lib/Lex/HeaderSearch.cpp (original)
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp Thu Aug  8 10:58:32 2019
@@ -309,9 +309,18 @@ const FileEntry *HeaderSearch::getFileAn
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  auto File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
-  if (!File)
+  llvm::ErrorOr File =
+  getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  if (!File) {
+// For rare, surprising errors (e.g. "out of file handles"), diag the EC
+// message.
+std::error_code EC = File.getError();
+if (EC != std::errc::no_such_file_or_directory &&
+EC != std::errc::is_a_directory) {
+  Diags.Report(IncludeLoc, diag::err_cannot_open_file) << EC.message();
+}
 return nullptr;
+  }
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(*File, Dir ? Dir : (*File)->getDir(),


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

[PATCH] D65956: clang: Diag running out of file handles while looking for files

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368322: clang: Diag running out of file handles while 
looking for files (authored by nico, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65956?vs=214166&id=214192#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65956

Files:
  cfe/trunk/lib/Lex/HeaderSearch.cpp


Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -309,9 +309,18 @@
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  auto File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
-  if (!File)
+  llvm::ErrorOr File =
+  getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  if (!File) {
+// For rare, surprising errors (e.g. "out of file handles"), diag the EC
+// message.
+std::error_code EC = File.getError();
+if (EC != std::errc::no_such_file_or_directory &&
+EC != std::errc::is_a_directory) {
+  Diags.Report(IncludeLoc, diag::err_cannot_open_file) << EC.message();
+}
 return nullptr;
+  }
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(*File, Dir ? Dir : (*File)->getDir(),


Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -309,9 +309,18 @@
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  auto File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
-  if (!File)
+  llvm::ErrorOr File =
+  getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  if (!File) {
+// For rare, surprising errors (e.g. "out of file handles"), diag the EC
+// message.
+std::error_code EC = File.getError();
+if (EC != std::errc::no_such_file_or_directory &&
+EC != std::errc::is_a_directory) {
+  Diags.Report(IncludeLoc, diag::err_cannot_open_file) << EC.message();
+}
 return nullptr;
+  }
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(*File, Dir ? Dir : (*File)->getDir(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r368323 - Recommit Devirtualize destructor of final class.

[Hiroshi Yamauchi via cfe-commits]

Author: yamauchi
Date: Thu Aug  8 11:00:49 2019
New Revision: 368323

URL: http://llvm.org/viewvc/llvm-project?rev=368323&view=rev
Log:
Recommit Devirtualize destructor of final class.

Original patch commited as r364100, reverted as r364359, recommitted as r365509,
reverted as r365850.


Added:
cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
Modified:
cfe/trunk/lib/CodeGen/CGExprCXX.cpp

Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=368323&r1=368322&r2=368323&view=diff
==
--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Thu Aug  8 11:00:49 2019
@@ -1882,9 +1882,33 @@ static void EmitObjectDelete(CodeGenFunc
   Dtor = RD->getDestructor();
 
   if (Dtor->isVirtual()) {
-CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,
-Dtor);
-return;
+bool UseVirtualCall = true;
+const Expr *Base = DE->getArgument();
+if (auto *DevirtualizedDtor =
+dyn_cast_or_null(
+Dtor->getDevirtualizedMethod(
+Base, CGF.CGM.getLangOpts().AppleKext))) {
+  UseVirtualCall = false;
+  const CXXRecordDecl *DevirtualizedClass =
+  DevirtualizedDtor->getParent();
+  if (declaresSameEntity(getCXXRecord(Base), DevirtualizedClass)) {
+// Devirtualized to the class of the base type (the type of the
+// whole expression).
+Dtor = DevirtualizedDtor;
+  } else {
+// Devirtualized to some other type. Would need to cast the this
+// pointer to that type but we don't have support for that yet, so
+// do a virtual call. FIXME: handle the case where it is
+// devirtualized to the derived type (the type of the inner
+// expression) as in EmitCXXMemberOrOperatorMemberCallExpr.
+UseVirtualCall = true;
+  }
+}
+if (UseVirtualCall) {
+  CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, 
ElementType,
+  Dtor);
+  return;
+}
   }
 }
   }

Added: cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp?rev=368323&view=auto
==
--- cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (added)
+++ cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp Thu Aug  8 11:00:49 
2019
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 %s -emit-llvm -o - 
| FileCheck %s
+
+namespace Test1 {
+  struct A { virtual ~A() {} };
+  struct B final : A {};
+  struct C : A { virtual ~C() final {} };
+  struct D { virtual ~D() final = 0; };
+  // CHECK-LABEL: define void @_ZN5Test13fooEPNS_1BE
+  void foo(B *b) {
+// CHECK: call void @_ZN5Test11BD1Ev
+delete b;
+  }
+  // CHECK-LABEL: define void @_ZN5Test14foo2EPNS_1CE
+  void foo2(C *c) {
+// CHECK: call void @_ZN5Test11CD1Ev
+delete c;
+  }
+  // CHECK-LABEL: define void @_ZN5Test14evilEPNS_1DE
+  void evil(D *p) {
+// CHECK-NOT: call void @_ZN5Test11DD1Ev
+delete p;
+  }
+}


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

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Why do you need to change (update) the datalayout in every single test?
That looks extremely noisy and hides the actually-needed changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64931



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