[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

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

Ignore lambdas in FunctionSizeCheck (functionDecl() now matches their 
operator()).

I think they should probably be included, but don't want to change the check
semantics in this patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444

Files:
  include/clang/AST/RecursiveASTVisitor.h
  lib/CodeGen/CodeGenPGO.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  unittests/AST/ASTContextParentMapTest.cpp
  unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp

Index: unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
+++ unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
@@ -17,25 +17,34 @@
 class LambdaExprVisitor : public ExpectedLocationVisitor {
 public:
   bool VisitLambdaExpr(LambdaExpr *Lambda) {
-PendingBodies.push(Lambda);
+llvm::errs() << "visitlambdaexpr\n";
+PendingBodies.push(Lambda->getBody());
+PendingClasses.push(Lambda->getLambdaClass());
 Match("", Lambda->getIntroducerRange().getBegin());
 return true;
   }
-  /// For each call to VisitLambdaExpr, we expect a subsequent call (with
-  /// proper nesting) to TraverseLambdaBody.
-  bool TraverseLambdaBody(LambdaExpr *Lambda) {
-EXPECT_FALSE(PendingBodies.empty());
-EXPECT_EQ(PendingBodies.top(), Lambda);
-PendingBodies.pop();
-return TraverseStmt(Lambda->getBody());
+  /// For each call to VisitLambdaExpr, we expect a subsequent call to visit
+  /// the body (and maybe the lambda class, which is implicit).
+  bool VisitStmt(Stmt *S) {
+if (!PendingBodies.empty() && S == PendingBodies.top())
+  PendingBodies.pop();
+return true;
   }
-  /// Determine whether TraverseLambdaBody has been called for every call to
-  /// VisitLambdaExpr.
-  bool allBodiesHaveBeenTraversed() const {
-return PendingBodies.empty();
+  bool VisitDecl(Decl *D) {
+if (!PendingClasses.empty() && D == PendingClasses.top())
+  PendingClasses.pop();
+return true;
   }
+  /// Determine whether parts of lambdas (VisitLambdaExpr) were later traversed.
+  bool allBodiesHaveBeenTraversed() const { return PendingBodies.empty(); }
+  bool allClassesHaveBeenTraversed() const { return PendingClasses.empty(); }
+
+  bool VisitImplicitCode = false;
+  bool shouldVisitImplicitCode() const { return VisitImplicitCode; }
+
 private:
-  std::stack PendingBodies;
+  std::stack PendingBodies;
+  std::stack PendingClasses;
 };
 
 TEST(RecursiveASTVisitor, VisitsLambdaExpr) {
@@ -43,13 +52,18 @@
   Visitor.ExpectMatch("", 1, 12);
   EXPECT_TRUE(Visitor.runOver("void f() { []{ return; }(); }",
   LambdaExprVisitor::Lang_CXX11));
+  EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_FALSE(Visitor.allClassesHaveBeenTraversed());
 }
 
-TEST(RecursiveASTVisitor, TraverseLambdaBodyCanBeOverridden) {
+TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   LambdaExprVisitor Visitor;
+  Visitor.VisitImplicitCode = true;
+  Visitor.ExpectMatch("", 1, 12);
   EXPECT_TRUE(Visitor.runOver("void f() { []{ return; }(); }",
   LambdaExprVisitor::Lang_CXX11));
   EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
 }
 
 TEST(RecursiveASTVisitor, VisitsAttributedLambdaExpr) {
Index: unittests/AST/ASTContextParentMapTest.cpp
===
--- unittests/AST/ASTContextParentMapTest.cpp
+++ unittests/AST/ASTContextParentMapTest.cpp
@@ -106,5 +106,16 @@
   EXPECT_THAT(Ctx.getParents(Foo), ElementsAre(DynTypedNode::create(TU)));
 }
 
+TEST(GetParents, ImplicitLambdaNodes) {
+  MatchVerifier LambdaVerifier;
+  EXPECT_TRUE(LambdaVerifier.match(
+  "auto x = []{int y;};",
+  varDecl(hasName("y"), hasAncestor(functionDecl(
+hasOverloadedOperatorName("()"),
+hasParent(cxxRecordDecl(
+isImplicit(), hasParent(lambdaExpr())),
+  Lang_CXX11));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -1153,7 +1153,12 @@
 bool TraverseDecl(Decl *D) { return true; }
 
 // We analyze lambda bodies separately. Skip them here.
-bool TraverseLambdaBody(LambdaExpr *LE) { return true; }
+bool TraverseLambdaExpr(LambdaExpr *LE) {
+  // Traverse the captures, but not the body.
+  for (const auto &C : zip(LE->captures(), LE->capture_inits()))
+TraverseLambdaCapture(LE, &std::get<0>(C), std::get<1>(C));
+  return true;
+}
 
   private:
 
Index: lib/CodeGen/Code

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: klimek.
sammccall added a comment.

In D56444#1350252 , @JonasToth wrote:

> I still see the unit-test crashing for `ExprMutAnalyzer` (just apply the last 
> two tests 
> https://github.com/JonasToth/clang/blob/fix_crash/unittests/Analysis/ExprMutationAnalyzerTest.cpp
>  and run `check-clang-unit`)


The `ReproduceFailure11` test passes for me, and the `ReproduceFailureMinimal` 
fails to compile (`a&&`).

> The clang-tidy check does not crash anymore, but `readability-function-size` 
> regresses. It probably doesn't properly count for lambdas anymore.

Rather, it previously **didn't** count lambda bodies as functions (because 
`functionDecl()` didn't include them), and now it does :-)
I think counting them is better behavior, but I've modified the matcher to 
preserve the behavior for now.

@klimek: would it be better to preserve the odd behavior of the 
`functionDecl()` matcher, and add a new `functionOrLambdaDecl()`? It seems too 
surprising to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:27
+static bool isGoogletestTestMacro(StringRef MacroName) {
+  static const llvm::StringSet<> MacroNames = {"TEST", "TEST_F", "TEST_P",
+   "TYPED_TEST", "TYPED_TEST_P"};

karepker wrote:
> MyDeveloperDay wrote:
> > Is there value in making the list of macros and option?, I've worked with 
> > other unit testing packages (some inhouse) that use the same format as 
> > google test, but don't use TEST() itself
> > 
> > e.g.  (to name a few)
> > 
> > ```
> > TEST_CASE(testclass, testname)
> > BOOST_TEST(statement, floating_point_comparison_manipulation);
> > BOOST_DATA_TEST_CASE(test_case_name, dataset)
> > BOOST_FIXTURE_TEST_CASE( test_case2, F )
> > BOOST_AUTO_TEST_CASE( test_case3 )
> > 
> > ```
> > 
> > too many for you to capture in the checker...but a nice addition for those 
> > who use alternative frameworks and would like to benefit from similar  "no 
> > underscore" naming conventions
> > 
> I'm not familiar with, e.g. the boost testing framework, so I don't know how 
> closely it mirrors Googletest, but I think my preference would be to have a 
> separate check for other testing frameworks.
> 
> While the testing frameworks might share some high-level similarities, there 
> could be some different corner cases which would make having a separate check 
> worth it as opposed to making this code more complex by trying to generalize 
> it for several cases. At the very least, a different diagnostic message would 
> be required. Factoring out similar functionality into some utility functions 
> might reduce some of the boilerplate from implementing separate checks.
Maybe, but a shame as the code for a different check would be almost identical 
and unlikely to be able to support seperate in house systems that work on the 
same principle, of combining the testcase and testname as a macro and the 
presense of _ at the start or in the middle generating invalid symbols or 
ambiguities.

So perhaps even the

> "according to Googletest FAQ"

might be unnecessary text anyway for the warning, its easily something that can 
go in the documentation for the justification for the checker, I'm not sure 
everyone needs to see this every time they look at a warning..

most checks just say what to do, not why its a bad idea or where to find the 
justification.

e.g.


```
diag(Loc, "do not use unnamed namespaces in header files");
```





Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+  Check->diag(TestCaseNameToken->getLocation(),
+  "avoid using \"_\" in test case name \"%0\" according to "
+  "Googletest FAQ")

karepker wrote:
> JonasToth wrote:
> > Duplicated message text, you can store it in a local 
> > variable/StringRef/StringLiteral,
> > 
> > ellide braces
> The message text is not quite the same. The first says "test case name" and 
> the second says "test name" per the naming conventions for the two name 
> arguments to the test macros in Googletest. I preferred having these as two 
> separate strings instead of trying to add the word "case" conditionally to 
> one, which I thought would be too complex.
> 
> Please let me know if you feel differently or have other suggestions 
> regarding the message.
> 
> Braces have been removed.
I think it would be enough to have one diag message saying 

```
diag(Loc,"don't use "_" in %s() ",testMacro) 
```

simply elide the name of the parameter, the user will be drawn to the line and 
will see they are using an "_" in either testname or testcase, I don't think it 
necessary to tell them which parameter is incorrect..

And so whilst I see why you might prefer to generate 2 different messages for 
either testcase and testname, isn't it also true that these messages will be 
wrong if your using one of the other macros likes TEST_F() shouldn't the second 
argument now be testfixture? and if I'm using TEST_P() shouldn't it be pattern 
or parameterized?

This is why I think the test macros used would be useful as an option, by 
generalizing the checker by removing the "googletest" specifics makes the 
checker much more useful.

I know for one, I don't use gtest, but a similar framework (with the same 
issues), and I'd use your checker if I could customize it.






Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D56483: [clangd] Add a test for SignatureHelp on dynamic index.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric.

This would catch regressions caused by future changes of the index.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56483

Files:
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1793,6 +1793,37 @@
 SigDoc("Doc from sema";
 }
 
+TEST(SignatureHelpTest, DynamicIndexDocumentation) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.BuildDynamicSymbolIndex = true;
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+
+  FS.Files[testPath("foo.h")] = R"cpp(
+struct Foo {
+   // Member doc
+   int foo();
+};
+  )cpp";
+  Annotations FileContent(R"cpp(
+#include "foo.h"
+void test() {
+  Foo f;
+  f.foo(^);
+}
+  )cpp");
+  auto File = testPath("test.cpp");
+  Server.addDocument(File, FileContent.code());
+  // Wait for the dynamic index being built.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(
+  llvm::cantFail(runSignatureHelp(Server, File, FileContent.point()))
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Member doc";
+}
+
 TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1793,6 +1793,37 @@
 SigDoc("Doc from sema";
 }
 
+TEST(SignatureHelpTest, DynamicIndexDocumentation) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.BuildDynamicSymbolIndex = true;
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+
+  FS.Files[testPath("foo.h")] = R"cpp(
+struct Foo {
+   // Member doc
+   int foo();
+};
+  )cpp";
+  Annotations FileContent(R"cpp(
+#include "foo.h"
+void test() {
+  Foo f;
+  f.foo(^);
+}
+  )cpp");
+  auto File = testPath("test.cpp");
+  Server.addDocument(File, FileContent.code());
+  // Wait for the dynamic index being built.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(
+  llvm::cantFail(runSignatureHelp(Server, File, FileContent.point()))
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Member doc";
+}
+
 TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+  Check->diag(TestCaseNameToken->getLocation(),
+  "avoid using \"_\" in test case name \"%0\" according to "
+  "Googletest FAQ")

MyDeveloperDay wrote:
> karepker wrote:
> > JonasToth wrote:
> > > Duplicated message text, you can store it in a local 
> > > variable/StringRef/StringLiteral,
> > > 
> > > ellide braces
> > The message text is not quite the same. The first says "test case name" and 
> > the second says "test name" per the naming conventions for the two name 
> > arguments to the test macros in Googletest. I preferred having these as two 
> > separate strings instead of trying to add the word "case" conditionally to 
> > one, which I thought would be too complex.
> > 
> > Please let me know if you feel differently or have other suggestions 
> > regarding the message.
> > 
> > Braces have been removed.
> I think it would be enough to have one diag message saying 
> 
> ```
> diag(Loc,"don't use "_" in %s() ",testMacro) 
> ```
> 
> simply elide the name of the parameter, the user will be drawn to the line 
> and will see they are using an "_" in either testname or testcase, I don't 
> think it necessary to tell them which parameter is incorrect..
> 
> And so whilst I see why you might prefer to generate 2 different messages for 
> either testcase and testname, isn't it also true that these messages will be 
> wrong if your using one of the other macros likes TEST_F() shouldn't the 
> second argument now be testfixture? and if I'm using TEST_P() shouldn't it be 
> pattern or parameterized?
> 
> This is why I think the test macros used would be useful as an option, by 
> generalizing the checker by removing the "googletest" specifics makes the 
> checker much more useful.
> 
> I know for one, I don't use gtest, but a similar framework (with the same 
> issues), and I'd use your checker if I could customize it.
> 
> 
> 
> 
minor correction. to my comment

```
diag(Loc,"avoid using \"_\" in %s() ",testMacro)
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D56444#1350746 , @sammccall wrote:

> In D56444#1350252 , @JonasToth wrote:
>
> > I still see the unit-test crashing for `ExprMutAnalyzer` (just apply the 
> > last two tests 
> > https://github.com/JonasToth/clang/blob/fix_crash/unittests/Analysis/ExprMutationAnalyzerTest.cpp
> >  and run `check-clang-unit`)
>
>
> The `ReproduceFailure11` test passes for me, and the 
> `ReproduceFailureMinimal` fails to compile (`a&&`).


Wupsi, thats odd. I am currently not at my PC and can't fix, but it seems that 
it should be `"template  T forward(T & A) { return 
static_cast(A); }"` instead of `a&&`. If you have the time to try again, 
great! Otherwise I will check this evening (8:00-9:00 evening berlin timezone).

>> The clang-tidy check does not crash anymore, but `readability-function-size` 
>> regresses. It probably doesn't properly count for lambdas anymore.
> 
> Rather, it previously **didn't** count lambda bodies as functions (because 
> `functionDecl()` didn't include them), and now it does :-)
>  I think counting them is better behavior, but I've modified the matcher to 
> preserve the behavior for now.
> 
> @klimek: would it be better to preserve the odd behavior of the 
> `functionDecl()` matcher, and add a new `functionOrLambdaDecl()`? It seems 
> too surprising to me.

We can change the clang-tidy check as well. I think lambdas should be 
considered functionDecls, shouldnt they? WDYT @aaron.ballman ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:259
+  // if this thread sees the older version but finishes later. This should
+  // be rare in practice.
+  DigestIt.first->second = Hash;

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > ilya-biryukov wrote:
> > > > > "should be rare in practice"
> > > > And yet, can we avoid this altogether?
> > > > 
> > > > Also, I believe it won't be rare. When processing multiple different 
> > > > TUs, we can easily run into the same header multiple times, possibly 
> > > > with the different contents.
> > > > E.g. imagine the index for the whole of clang is being built and the 
> > > > user is editing `Sema.h` at the same time. We'll definitely be running 
> > > > into this case over and over again.
> > > Well I am open to ideas, but currently there is no way of knowing whether 
> > > this is the newer version for the file. Because only information we have 
> > > is the digest is different than what we currently have and this doesn't 
> > > yield any chronological information.
> > Do we know which request is "newer" when scheduling it?
> > If so, we could keep the map of the **latest** hashes of the files we've 
> > seen (in addition to the `IndexedFileDigests`, which correspond to the 
> > digest for which we built the index IIUC).
> > 
> > This would give us a simple invariant of the final state we want to bring 
> > the index to: `IndexedFileDigests` should correspond to the latest hashes 
> > seen so far. If not, we have to rebuild the index for the corresponding 
> > files. That, in turn, gives us a way to resolve conflicts: we should never 
> > replace with symbols built for the latest version (hash) of the file we've 
> > seen.
> > 
> > Would that work?
> I am not sure if it would work for non-main files. We update the Hash for the 
> main files at each indexing action, so we can safely keep track of the latest 
> versions. But we collect hashes for dependencies while performing the 
> indexing which happens in parallel. Therefore an indexing action triggered 
> earlier might get an up-to-date version of a dependency than an action 
> triggered later-on.
If updates for each TU are atomic (i.e. all files included in the TU are 
updated in a single go) we would get the up-to-date index eventually, assuming 
we rebuild the index when TU dependencies change (we don't schedule rebuilds on 
changes to included header now, but we're planning to do so at some point).



Comment at: clangd/index/Background.cpp:468
+if (!Shard || !Shard->Sources) {
+  vlog("Failed to load shard: {0}", CurDependencyPath);
+  continue;

kadircet wrote:
> ilya-biryukov wrote:
> > Should we mark the file as requiring re-indexing at this point?
> > Not sure how that might happen in practice, but still...
> All files are marked as requiring re-indexing by default 
> `Dependencies.push_back({AbsolutePath, true})`. The second element is always 
> true, and we only mark it as false if we are sure file is up-to-date.
My confusion is coming from the fact that I'm constantly forgetting that we 
return the queue we're processing afterwards.
Could you put a small comment that file will be returned to the caller as 
requiring a reindex here?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224



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

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/SourceCode.h:107
+// "/tmp/build/foo.h"
+std::string makeCanonicalPath(llvm::StringRef AbsolutePath,
+  const SourceManager &SM);

This changes should go away after the rebase, right?
Could you please run the rebase to make sure the patch is in its final state.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224



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

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D51641#1338004 , @labath wrote:

> Might I ask what was the motivation for this change? Performance 
> optimalization?


Yes, this was for performance.
IIRC the problem was something like calling `VFS->makeAbsolute(P)` for every 
filename in a large PCH.

This cache indeed breaks two related assumptions that used to hold:

1. `VFS->getCurrentWorkingDirectory()` is consistent with how paths are 
interpreted by `VFS->openFileForRead()` etc
2. `VFS->getCurrentWorkingDirectory()` is consistent with the process's CWD: 
`::getcwd`, `llvm::sys::fs::current_path`, etc

The first should clearly hold, so this cache certainly introduced a bug. And 
probably a hard one to track down, sorry :-(
However, I don't think the second should actually be guaranteed here (see 
below...)

> I am asking this because this makes the RealFileSystem return bogus values 
> for the CWD if it changes through means other than the 
> VFS::setCurrentWorkingDirectory (which is something that, as a library, we 
> can never rule out). We ran into problems with this in lldb where our tests 
> use lldb as a library, and they are driven by a python scripts (which does 
> some CWD changing as a part of test discovery).
> 
> If I understand the long scary comment in the `setCurrentWorkingDirectory` 
> function correctly, the RealFileSystem wants to be independent of the OS 
> notion of CWD. However, right now it's not doing either job very well 
> (sharing the CWD with the OS nor being independent of it), because all other 
> RFS functions will use the real OS CWD (as it is at the moment of the call), 
> only `getCurrentWorkingDirectory` will return whatever was the last CWD set 
> through the appropriate setter.

Right. In order to write correct multithreaded programs that use the 
`FileSystem` abstraction, the VFS's working directory needs to be a local 
attribute of that VFS, not something global.
However the current implementation of `RealFileSystem` doesn't satisfy this. It 
is fixable on most OSes (on recent unix, store a FD for the working dir and use 
`openat()`) but nobody was burned badly enough to fix it yet I think. With this 
fix, clearly no cache would be required.

This would imply that RealFileSystem would always return the working directory 
from when it was created, and all other function would use that working 
directory too.
Does that seem reasonable? (Does it make it easier to fix the code that got 
broken by this cache?)

I can try to get an `openat()`-based implementation ready.


Repository:
  rC Clang

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

https://reviews.llvm.org/D51641



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

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 180808.
hokein marked 2 inline comments as done.
hokein added a comment.

Address comments, store docs.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314

Files:
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -655,10 +655,15 @@
 void Foo::ssf() {}
   )";
   runSymbolCollector(Header, Main);
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("Foo"), QName("Foo::f"),
-   QName("Foo::g"), QName("Foo::sf"),
-   QName("Foo::ssf"), QName("Foo::x")));
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  QName("Foo"),
+  AllOf(QName("Foo::f"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::g"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::sf"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::ssf"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::x"), ReturnType(""), ForCodeCompletion(false;
 }
 
 TEST_F(SymbolCollectorTest, Scopes) {
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -531,6 +531,15 @@
   getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
 S.CanonicalDeclaration = *DeclLoc;
 
+  S.Origin = Opts.Origin;
+  if (ND.getAvailability() == AR_Deprecated)
+S.Flags |= Symbol::Deprecated;
+
+  auto Insert = [this](const Symbol& S) {
+Symbols.insert(S);
+return Symbols.find(S.ID);
+  };
+
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
@@ -540,13 +549,26 @@
   *ASTCtx, *PP, CodeCompletionContext::CCC_Symbol, *CompletionAllocator,
   *CompletionTUInfo,
   /*IncludeBriefComments*/ false);
-  std::string Signature;
-  std::string SnippetSuffix;
-  getSignature(*CCS, &Signature, &SnippetSuffix);
   std::string Documentation =
   formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
   /*CommentsFromHeaders=*/true));
+  // For symbols not indexed for completion (class members), we also store their
+  // docs in the index, because Sema doesn't load the docs from the preamble, we
+  // rely on the index to get the docs.
+  // FIXME: this can be optimized by only storing the docs in dynamic index --
+  // dynamic index should index these symbols when Sema completes a member
+  // completion.
+  S.Documentation = Documentation;
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion))
+return Insert(S);
+
+  std::string Signature;
+  std::string SnippetSuffix;
+  getSignature(*CCS, &Signature, &SnippetSuffix);
+  S.Signature = Signature;
+  S.CompletionSnippetSuffix = SnippetSuffix;
   std::string ReturnType = getReturnType(*CCS);
+  S.ReturnType = ReturnType;
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
@@ -556,10 +578,6 @@
 QName, SM, SM.getExpansionLoc(ND.getLocation()), Opts))
   Include = std::move(*Header);
   }
-  S.Signature = Signature;
-  S.CompletionSnippetSuffix = SnippetSuffix;
-  S.Documentation = Documentation;
-  S.ReturnType = ReturnType;
   if (!Include.empty())
 S.IncludeHeaders.emplace_back(Include, 1);
 
@@ -569,12 +587,7 @@
 if (TypeStorage)
   S.Type = TypeStorage->raw();
   }
-
-  S.Origin = Opts.Origin;
-  if (ND.getAvailability() == AR_Deprecated)
-S.Flags |= Symbol::Deprecated;
-  Symbols.insert(S);
-  return Symbols.find(S.ID);
+  return Insert(S);
 }
 
 void SymbolCollector::addDefinition(const NamedDecl &ND,
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -185,19 +185,23 @@
   SymbolOrigin Origin = SymbolOrigin::Unknown;
   /// A brief description of the symbol that can be appended in the completion
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// This field is only meaningful when the symbol is indexed for completion.
   llvm::StringRef Signature;
   /// What to insert when completing this symbol, after the symbol name.
   /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
   /// (When snippets are disabled, the symbol name alone is used).
+  /// This field is only meaningful when the symbol is indexed for completion.
   llvm::StringRef CompletionSnippetSuffix;
   /// Docume

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:543
+
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion))
+return Insert(S);

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Most of the fields updated at the bottom aren't useful. However, I feel 
> > > the documentation is actually important, since Sema only has doc comments 
> > > for the **current** file and the rest are currently expected to be 
> > > provided by the index.
> > > 
> > > I'm not sure if we already have the code to query the doc comments via 
> > > index for member completions. If not, it's an oversight.
> > > In any case, I suggest we **always** store the comments in **dynamic** 
> > > index. Not storing the comments in the static index is fine, since any 
> > > data for member completions should be provided by the dynamic index (we 
> > > see a member in completion ⇒ sema has processed the headers ⇒ the dynamic 
> > > index should know about those members)
> > This is a good point.
> > 
> > For class member completions, we rely solely on Sema completions (no query 
> > being queried). I'm not sure it is practical to query the index for member 
> > completions.
> > - this means for **every** code completion, we query the index, it may slow 
> > down completions
> > - `fuzzyFind` is not supported for class members in our internal index 
> > service (due to the large number of them)
> > 
> > So it turns two possibilities:
> > 
> > 1) always store comments (`SymbolCollector` doesn't know whether it is used 
> > in static index or dynamic index)
> > 2) or drop them for now, this wouldn't break anything, and add it back when 
> > we actually use them for class completions
> > 
> > I slightly prefer 2) at the moment. WDYT?
> Yeah, instead of using `fuzzyFind()`, we'll call  `lookup()` to get details 
> of the symbols we've discovered.
> It's true that this is going to add some latency, but I hope we have the 
> latency budget to handle this (these queries should be fast, e.g. we're doing 
> this for signature help and I haven't seen any noticeable latency there from 
> the index query, most of the running time is parsing C++).
> 
> I also like option 2, but unfortunately we already rely on this to get the 
> comments in signature help, so this change would actually introduce 
> regressions there (less used than code completion, but still not nice to 
> break it)
> Would the third option of having a config option (e.g. 
> `SymbolCollector::Options::StoreAllComments`) work?  `clangd-indexer` and 
> auto-indexer would set the option to false, `indexSymbols` would set the 
> option to true. We'll both get the optimizations and avoid introducing any 
> regressions. The plumbing should be no more than a few lines of code.
Sounds fair. I totally missed `signature help`, it is unfortunate that our test 
case doesn't find this regression (added one!)

> Would the third option of having a config option (e.g. 
> SymbolCollector::Options::StoreAllComments) work?  clangd-indexer and 
> auto-indexer would set the option to false, indexSymbols would set the option 
> to true. We'll both get the optimizations and avoid introducing any 
> regressions. The plumbing should be no more than a few lines of code.

This is a reasonable solution, but I prefer to do it in a separated patch. Now 
I make this patch always store docs, which should not introduce any 
regressions. There is another optimization opportunity here -- unlike header 
symbols, docs from main-file symbols can be dropped from the index, we can 
retrieve them from Sema.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D56444#1350746 , @sammccall wrote:

> In D56444#1350252 , @JonasToth wrote:
>
> > I still see the unit-test crashing for `ExprMutAnalyzer` (just apply the 
> > last two tests 
> > https://github.com/JonasToth/clang/blob/fix_crash/unittests/Analysis/ExprMutationAnalyzerTest.cpp
> >  and run `check-clang-unit`)
>
>
> The `ReproduceFailure11` test passes for me, and the 
> `ReproduceFailureMinimal` fails to compile (`a&&`).
>
> > The clang-tidy check does not crash anymore, but 
> > `readability-function-size` regresses. It probably doesn't properly count 
> > for lambdas anymore.
>
> Rather, it previously **didn't** count lambda bodies as functions (because 
> `functionDecl()` didn't include them), and now it does :-)
>  I think counting them is better behavior, but I've modified the matcher to 
> preserve the behavior for now.
>
> @klimek: would it be better to preserve the odd behavior of the 
> `functionDecl()` matcher, and add a new `functionOrLambdaDecl()`? It seems 
> too surprising to me.


I'm in favor of functionDecl really being a very simple matcher. Every time we 
tried to be smart in the end it turned out that wasn't the right call.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D54428: [clangd] XPC transport layer, framework, test-client

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@arphaman or @sammccall, could you please take the final look?


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

https://reviews.llvm.org/D54428



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

[PATCH] D54428: [clangd] XPC transport layer, framework, test-client

[Jan Korous via Phabricator via cfe-commits]

jkorous updated this revision to Diff 180809.
jkorous edited the summary of this revision.
jkorous added a comment.

Fixed the synchronous handling of events.


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

https://reviews.llvm.org/D54428

Files:
  CMakeLists.txt
  clangd/CMakeLists.txt
  clangd/Features.inc.in
  clangd/Transport.h
  clangd/tool/CMakeLists.txt
  clangd/tool/ClangdMain.cpp
  clangd/xpc/CMakeLists.txt
  clangd/xpc/Conversion.cpp
  clangd/xpc/Conversion.h
  clangd/xpc/README.txt
  clangd/xpc/XPCTransport.cpp
  clangd/xpc/cmake/Info.plist.in
  clangd/xpc/cmake/XPCServiceInfo.plist.in
  clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
  clangd/xpc/framework/CMakeLists.txt
  clangd/xpc/framework/ClangdXPC.cpp
  clangd/xpc/test-client/CMakeLists.txt
  clangd/xpc/test-client/ClangdXPCTestClient.cpp
  test/CMakeLists.txt
  test/clangd/xpc/initialize.test
  test/lit.cfg
  test/lit.site.cfg.in
  unittests/clangd/CMakeLists.txt
  unittests/clangd/xpc/CMakeLists.txt
  unittests/clangd/xpc/ConversionTests.cpp

Index: unittests/clangd/xpc/ConversionTests.cpp
===
--- /dev/null
+++ unittests/clangd/xpc/ConversionTests.cpp
@@ -0,0 +1,36 @@
+//===-- ConversionTests.cpp  --*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "xpc/Conversion.h"
+#include "gtest/gtest.h"
+
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+TEST(JsonXpcConversionTest, JsonToXpcToJson) {
+
+  for (auto &testcase :
+   {json::Value(false), json::Value(3.14), json::Value(42),
+json::Value(-100), json::Value("foo"), json::Value(""),
+json::Value("123"), json::Value(" "),
+json::Value{true, "foo", nullptr, 42},
+json::Value(json::Object{
+{"a", true}, {"b", "foo"}, {"c", nullptr}, {"d", 42}})}) {
+EXPECT_TRUE(testcase == xpcToJson(jsonToXpc(testcase))) << testcase;
+  }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/xpc/CMakeLists.txt
===
--- /dev/null
+++ unittests/clangd/xpc/CMakeLists.txt
@@ -0,0 +1,21 @@
+set(LLVM_LINK_COMPONENTS
+  support
+  )
+
+get_filename_component(CLANGD_SOURCE_DIR
+  ${CMAKE_CURRENT_SOURCE_DIR}/../../clangd REALPATH)
+include_directories(
+  ${CLANGD_SOURCE_DIR}
+  )
+
+add_extra_unittest(ClangdXpcTests
+  ConversionTests.cpp
+  )
+
+target_link_libraries(ClangdXpcTests
+  PRIVATE
+  clangdXpcJsonConversions
+  clangDaemon
+  LLVMSupport
+  LLVMTestingSupport
+  )
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -65,3 +65,7 @@
   LLVMSupport
   LLVMTestingSupport
   )
+
+if (CLANGD_BUILD_XPC)
+  add_subdirectory(xpc)
+endif ()
Index: test/lit.site.cfg.in
===
--- test/lit.site.cfg.in
+++ test/lit.site.cfg.in
@@ -11,6 +11,7 @@
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.clang_staticanalyzer = @CLANG_ENABLE_STATIC_ANALYZER@
+config.clangd_xpc_support = @CLANGD_BUILD_XPC_SUPPORT@
 
 # Support substitution of the tools and libs dirs with user parameters. This is
 # used when we can't determine the tool dir at configuration time.
Index: test/lit.cfg
===
--- test/lit.cfg
+++ test/lit.cfg
@@ -117,6 +117,10 @@
 if platform.system() not in ['Windows']:
 config.available_features.add('ansi-escape-sequences')
 
+# XPC support for Clangd.
+if config.clangd_xpc_support:
+config.available_features.add('clangd-xpc-support')
+
 if config.clang_staticanalyzer:
 config.available_features.add('static-analyzer')
 
Index: test/clangd/xpc/initialize.test
===
--- /dev/null
+++ test/clangd/xpc/initialize.test
@@ -0,0 +1,10 @@
+# RUN: clangd-xpc-test-client < %s | FileCheck %s
+# REQUIRES: clangd-xpc-support
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
+# CHECK: {"id":0,"jsonrpc":"2.0","result":{"capabilities"
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"id":3,"jsonrpc":"2.0","result":null}
+
+{"jsonrpc":"2.0","method":"exit"}
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -16,7 +16,8 @@
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} CLANG_TOOLS_DIR ${LLVM_RUNTIME_OUTPUT_INTDI

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:197
+Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
+const std::string FileName = Cmd.Filename;
+if (auto Error = index(std::move(Cmd), Storage))

use `llvm::StringRef`



Comment at: clangd/index/Background.cpp:259
+  // Skip if file is already up to date.
+  auto DigestIt = IndexedFileDigests.try_emplace(Path);
+  if (!DigestIt.second && DigestIt.first->second == Hash)

I now see that my previous comment about lock/unlock for each file was wrong in 
the first place.
It feels that's we'd better keep the `IndexedFileDigests`  and `IndexedSymbols` 
in sync.

Why don't we update the digest in the next loop that actually updates the 
symbols?



Comment at: clangd/index/Background.h:111
+bool NeedsReIndexing;
+Source(llvm::StringRef Path, bool NeedsReIndexing = true)
+: Path(Path), NeedsReIndexing(NeedsReIndexing) {}

NIT: maybe remove the constructor? plain structs can easily be initialized with 
init lists and adding a constructor forbids per-field assignment, which is a 
bit nicer in some cases.
Not very important, since it's a private API, but still.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

The timing of this is unfortunate because the change will not be needed soon. 
Refactoring is underway to extract a generic traverser from ASTDumper. Then the 
parent/child traversal of ASTMatchFinder can be implemented in terms of it 
without any of these kinds of problems. See

http://clang-developers.42468.n3.nabble.com/Dumping-AST-information-to-other-formats-tp4062988p4063004.html


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:27
+static bool isGoogletestTestMacro(StringRef MacroName) {
+  static const llvm::StringSet<> MacroNames = {"TEST", "TEST_F", "TEST_P",
+   "TYPED_TEST", "TYPED_TEST_P"};

MyDeveloperDay wrote:
> karepker wrote:
> > MyDeveloperDay wrote:
> > > Is there value in making the list of macros and option?, I've worked with 
> > > other unit testing packages (some inhouse) that use the same format as 
> > > google test, but don't use TEST() itself
> > > 
> > > e.g.  (to name a few)
> > > 
> > > ```
> > > TEST_CASE(testclass, testname)
> > > BOOST_TEST(statement, floating_point_comparison_manipulation);
> > > BOOST_DATA_TEST_CASE(test_case_name, dataset)
> > > BOOST_FIXTURE_TEST_CASE( test_case2, F )
> > > BOOST_AUTO_TEST_CASE( test_case3 )
> > > 
> > > ```
> > > 
> > > too many for you to capture in the checker...but a nice addition for 
> > > those who use alternative frameworks and would like to benefit from 
> > > similar  "no underscore" naming conventions
> > > 
> > I'm not familiar with, e.g. the boost testing framework, so I don't know 
> > how closely it mirrors Googletest, but I think my preference would be to 
> > have a separate check for other testing frameworks.
> > 
> > While the testing frameworks might share some high-level similarities, 
> > there could be some different corner cases which would make having a 
> > separate check worth it as opposed to making this code more complex by 
> > trying to generalize it for several cases. At the very least, a different 
> > diagnostic message would be required. Factoring out similar functionality 
> > into some utility functions might reduce some of the boilerplate from 
> > implementing separate checks.
> Maybe, but a shame as the code for a different check would be almost 
> identical and unlikely to be able to support seperate in house systems that 
> work on the same principle, of combining the testcase and testname as a macro 
> and the presense of _ at the start or in the middle generating invalid 
> symbols or ambiguities.
> 
> So perhaps even the
> 
> > "according to Googletest FAQ"
> 
> might be unnecessary text anyway for the warning, its easily something that 
> can go in the documentation for the justification for the checker, I'm not 
> sure everyone needs to see this every time they look at a warning..
> 
> most checks just say what to do, not why its a bad idea or where to find the 
> justification.
> 
> e.g.
> 
> 
> ```
> diag(Loc, "do not use unnamed namespaces in header files");
> ```
> 
> 
> Maybe, but a shame as the code for a different check would be almost 
> identical and unlikely to be able to support seperate in house systems that 
> work on the same principle, of combining the testcase and testname as a macro 
> and the presense of _ at the start or in the middle generating invalid 
> symbols or ambiguities.

I agree that we should reuse the code as much as possible. As @karepker 
mentioned, we don't know the details about other testing frameworks (they may 
use the same principle), it is unsafe to make this assumption.

Note that the original motivation of this check is for google test framework, I 
think we should focus on this scope in this patch, rather than trying to make 
this check fit everything. And we could always find a way to reuse the code 
(either by making this check more general, configurable or by creating a base 
class) if in the future we want to support other test frameworks.


> So perhaps even the
> 
> "according to Googletest FAQ"
> 
> might be unnecessary text anyway for the warning, its easily something that 
> can go in the documentation for the justification for the checker, I'm not 
> sure everyone needs to see this every time they look at a warning..

As a user not familiar with gtest internal implementation, I found the 
diagnostic "avoid using \"_\" in test case name \"%0\"" a bit confusing, I 
don't know why `_` is not allowed, adding `according to Googletest FAQ` words 
would make it clearer to users, at least given them a clue ;)




Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

r350703 - Use DeclSpec for quals in DeclaratorChunk::FunctionTypeInfo.

[Anastasia Stulova via cfe-commits]

Author: stulova
Date: Wed Jan  9 03:25:09 2019
New Revision: 350703

URL: http://llvm.org/viewvc/llvm-project?rev=350703&view=rev
Log:
Use DeclSpec for quals in DeclaratorChunk::FunctionTypeInfo.

Rather than duplicating data fields, use DeclSpec directly to store
the qualifiers for the functions/methods. This change doesn't handle
attributes yet and has to be extended further.

Differential revision: https://reviews.llvm.org/D55948


Modified:
cfe/trunk/include/clang/Sema/DeclSpec.h
cfe/trunk/lib/Parse/ParseDecl.cpp
cfe/trunk/lib/Parse/ParseDeclCXX.cpp
cfe/trunk/lib/Parse/ParseExpr.cpp
cfe/trunk/lib/Parse/ParseExprCXX.cpp
cfe/trunk/lib/Sema/DeclSpec.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaLambda.cpp
cfe/trunk/lib/Sema/SemaType.cpp

Modified: cfe/trunk/include/clang/Sema/DeclSpec.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/DeclSpec.h?rev=350703&r1=350702&r2=350703&view=diff
==
--- cfe/trunk/include/clang/Sema/DeclSpec.h (original)
+++ cfe/trunk/include/clang/Sema/DeclSpec.h Wed Jan  9 03:25:09 2019
@@ -593,6 +593,18 @@ public:
 FS_noreturnLoc = SourceLocation();
   }
 
+  /// This method calls the passed in handler on each CVRU qual being
+  /// set.
+  /// Handle - a handler to be invoked.
+  void forEachCVRUQualifier(
+  llvm::function_ref Handle);
+
+  /// This method calls the passed in handler on each qual being
+  /// set.
+  /// Handle - a handler to be invoked.
+  void forEachQualifier(
+  llvm::function_ref Handle);
+
   /// Return true if any type-specifier has been found.
   bool hasTypeSpecifier() const {
 return getTypeSpecType() != DeclSpec::TST_unspecified ||
@@ -683,6 +695,8 @@ public:
 ExprRep = Rep;
   }
 
+  bool SetTypeQual(TQ T, SourceLocation Loc);
+
   bool SetTypeQual(TQ T, SourceLocation Loc, const char *&PrevSpec,
unsigned &DiagID, const LangOptions &Lang);
 
@@ -1250,10 +1264,6 @@ struct DeclaratorChunk {
 /// Otherwise, it's an rvalue reference.
 unsigned RefQualifierIsLValueRef : 1;
 
-/// The type qualifiers: const/volatile/restrict/__unaligned
-/// The qualifier bitmask values are the same as in QualType.
-unsigned TypeQuals : 4;
-
 /// ExceptionSpecType - An ExceptionSpecificationType value.
 unsigned ExceptionSpecType : 4;
 
@@ -1287,21 +1297,6 @@ struct DeclaratorChunk {
 /// If this is an invalid location, there is no ref-qualifier.
 unsigned RefQualifierLoc;
 
-/// The location of the const-qualifier, if any.
-///
-/// If this is an invalid location, there is no const-qualifier.
-unsigned ConstQualifierLoc;
-
-/// The location of the volatile-qualifier, if any.
-///
-/// If this is an invalid location, there is no volatile-qualifier.
-unsigned VolatileQualifierLoc;
-
-/// The location of the restrict-qualifier, if any.
-///
-/// If this is an invalid location, there is no restrict-qualifier.
-unsigned RestrictQualifierLoc;
-
 /// The location of the 'mutable' qualifer in a lambda-declarator, if
 /// any.
 unsigned MutableLoc;
@@ -1317,6 +1312,12 @@ struct DeclaratorChunk {
 /// there are no parameters specified.
 ParamInfo *Params;
 
+/// DeclSpec for the function with the qualifier related info.
+DeclSpec *MethodQualifiers;
+
+/// AtttibuteFactory for the MethodQualifiers.
+AttributeFactory *QualAttrFactory;
+
 union {
   /// Pointer to a new[]'d array of TypeAndRange objects that
   /// contain the types in the function's dynamic exception specification
@@ -1356,6 +1357,8 @@ struct DeclaratorChunk {
 
 void destroy() {
   freeParams();
+  delete QualAttrFactory;
+  delete MethodQualifiers;
   switch (getExceptionSpecType()) {
   default:
 break;
@@ -1372,6 +1375,14 @@ struct DeclaratorChunk {
   }
 }
 
+DeclSpec &getOrCreateMethodQualifiers() {
+  if (!MethodQualifiers) {
+QualAttrFactory = new AttributeFactory();
+MethodQualifiers = new DeclSpec(*QualAttrFactory);
+  }
+  return *MethodQualifiers;
+}
+
 /// isKNRPrototype - Return true if this is a K&R style identifier list,
 /// like "void foo(a,b,c)".  In a function definition, this will be 
followed
 /// by the parameter type definitions.
@@ -1406,19 +1417,22 @@ struct DeclaratorChunk {
   return SourceLocation::getFromRawEncoding(RefQualifierLoc);
 }
 
-/// Retrieve the location of the 'const' qualifier, if any.
+/// Retrieve the location of the 'const' qualifier.
 SourceLocation getConstQualifierLoc() const {
-  return SourceLocation::getFromRawEncoding(ConstQualifierLoc);
+  assert(MethodQualifiers);
+  return MethodQualifiers->getConstSpecLoc();
 }
 
-/// Retrieve the location of the 'volatile' qualifier, if any.
+///

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC350703: Use DeclSpec for quals in 
DeclaratorChunk::FunctionTypeInfo. (authored by stulova, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D55948

Files:
  include/clang/Sema/DeclSpec.h
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/DeclSpec.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp

Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -1206,12 +1206,8 @@
   /*hasProto=*/true,
   /*isAmbiguous=*/false, LParenLoc, ParamInfo.data(),
   ParamInfo.size(), EllipsisLoc, RParenLoc,
-  DS.getTypeQualifiers(),
   /*RefQualifierIsLValueRef=*/true,
-  /*RefQualifierLoc=*/NoLoc,
-  /*ConstQualifierLoc=*/NoLoc,
-  /*VolatileQualifierLoc=*/NoLoc,
-  /*RestrictQualifierLoc=*/NoLoc, MutableLoc, ESpecType,
+  /*RefQualifierLoc=*/NoLoc, MutableLoc, ESpecType,
   ESpecRange, DynamicExceptions.data(),
   DynamicExceptionRanges.data(), DynamicExceptions.size(),
   NoexceptExpr.isUsable() ? NoexceptExpr.get() : nullptr,
@@ -1273,12 +1269,8 @@
   /*NumParams=*/0,
   /*EllipsisLoc=*/NoLoc,
   /*RParenLoc=*/NoLoc,
-  /*TypeQuals=*/0,
   /*RefQualifierIsLValueRef=*/true,
-  /*RefQualifierLoc=*/NoLoc,
-  /*ConstQualifierLoc=*/NoLoc,
-  /*VolatileQualifierLoc=*/NoLoc,
-  /*RestrictQualifierLoc=*/NoLoc, MutableLoc, EST_None,
+  /*RefQualifierLoc=*/NoLoc, MutableLoc, EST_None,
   /*ESpecRange=*/SourceRange(),
   /*Exceptions=*/nullptr,
   /*ExceptionRanges=*/nullptr,
Index: lib/Parse/ParseDeclCXX.cpp
===
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -2346,32 +2346,22 @@
   if (D.isFunctionDeclarator()) {
 auto &Function = D.getFunctionTypeInfo();
 if (DS.getTypeQualifiers() != DeclSpec::TQ_unspecified) {
-  auto DeclSpecCheck = [&] (DeclSpec::TQ TypeQual,
-const char *FixItName,
-SourceLocation SpecLoc,
-unsigned* QualifierLoc) {
+  auto DeclSpecCheck = [&](DeclSpec::TQ TypeQual, StringRef FixItName,
+   SourceLocation SpecLoc) {
 FixItHint Insertion;
-if (DS.getTypeQualifiers() & TypeQual) {
-  if (!(Function.TypeQuals & TypeQual)) {
-std::string Name(FixItName);
-Name += " ";
-Insertion = FixItHint::CreateInsertion(VS.getFirstLocation(), Name);
-Function.TypeQuals |= TypeQual;
-*QualifierLoc = SpecLoc.getRawEncoding();
-  }
-  Diag(SpecLoc, diag::err_declspec_after_virtspec)
+auto &MQ = Function.getOrCreateMethodQualifiers();
+if (!(MQ.getTypeQualifiers() & TypeQual)) {
+  std::string Name(FixItName.data());
+  Name += " ";
+  Insertion = FixItHint::CreateInsertion(VS.getFirstLocation(), Name);
+  MQ.SetTypeQual(TypeQual, SpecLoc);
+}
+Diag(SpecLoc, diag::err_declspec_after_virtspec)
 << FixItName
 << VirtSpecifiers::getSpecifierName(VS.getLastSpecifier())
-<< FixItHint::CreateRemoval(SpecLoc)
-<< Insertion;
-}
+<< FixItHint::CreateRemoval(SpecLoc) << Insertion;
   };
-  DeclSpecCheck(DeclSpec::TQ_const, "const", DS.getConstSpecLoc(),
-&Function.ConstQualifierLoc);
-  DeclSpecCheck(DeclSpec::TQ_volatile, "volatile", DS.getVolatileSpecLoc(),
-&Function.VolatileQualifierLoc);
-  DeclSpecCheck(DeclSpec::TQ_restrict, "restrict", DS.getRestrictSpecLoc(),
-&Function.RestrictQualifierLoc);
+  DS.forEachQualifier(DeclSpecCheck);
 }
 
 // Parse ref-qualifiers.
Index: lib/Parse/ParseExpr.cpp
===
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -3012,12 +3012,8 @@
  /*NumArgs=*/0,
  /*EllipsisLoc=*/NoLoc,
  /*RParenLoc=*/NoLoc,
- /*TypeQuals=*/0,
 

[libunwind] r350705 - [Sparc] Add Sparc V8 support

[Daniel Cederman via cfe-commits]

Author: dcederman
Date: Wed Jan  9 04:06:05 2019
New Revision: 350705

URL: http://llvm.org/viewvc/llvm-project?rev=350705&view=rev
Log:
[Sparc] Add Sparc V8 support

Summary:
Adds the register class implementation for Sparc.
Adds support for DW_CFA_GNU_window_save.
Adds save and restore context functionality.

On Sparc the return address is the address of the call instruction,
so an offset needs to be added when returning to skip the call instruction
and its delay slot. If the function returns a struct it is also necessary
to skip one extra instruction.

Reviewers: jyknight, mclow.lists, mstorsjo, compnerd

Reviewed By: compnerd

Subscribers: fedor.sergeev, JDevlieghere, ldionne, libcxx-commits

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

Modified:
libunwind/trunk/include/__libunwind_config.h
libunwind/trunk/include/libunwind.h
libunwind/trunk/src/DwarfInstructions.hpp
libunwind/trunk/src/DwarfParser.hpp
libunwind/trunk/src/Registers.hpp
libunwind/trunk/src/UnwindCursor.hpp
libunwind/trunk/src/UnwindRegistersRestore.S
libunwind/trunk/src/UnwindRegistersSave.S
libunwind/trunk/src/assembly.h
libunwind/trunk/src/libunwind.cpp

Modified: libunwind/trunk/include/__libunwind_config.h
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/include/__libunwind_config.h?rev=350705&r1=350704&r2=350705&view=diff
==
--- libunwind/trunk/include/__libunwind_config.h (original)
+++ libunwind/trunk/include/__libunwind_config.h Wed Jan  9 04:06:05 2019
@@ -23,6 +23,7 @@
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_ARM   287
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_OR1K  32
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_MIPS  65
+#define _LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC 31
 
 #if defined(_LIBUNWIND_IS_NATIVE_ONLY)
 # if defined(__i386__)
@@ -113,6 +114,11 @@
 #error "Unsupported MIPS ABI and/or environment"
 #  endif
 #  define _LIBUNWIND_HIGHEST_DWARF_REGISTER 
_LIBUNWIND_HIGHEST_DWARF_REGISTER_MIPS
+# elif defined(__sparc__)
+  #define _LIBUNWIND_TARGET_SPARC 1
+  #define _LIBUNWIND_HIGHEST_DWARF_REGISTER 
_LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC
+  #define _LIBUNWIND_CONTEXT_SIZE 16
+  #define _LIBUNWIND_CURSOR_SIZE 23
 # else
 #  error "Unsupported architecture."
 # endif
@@ -126,6 +132,7 @@
 # define _LIBUNWIND_TARGET_OR1K 1
 # define _LIBUNWIND_TARGET_MIPS_O32 1
 # define _LIBUNWIND_TARGET_MIPS_NEWABI 1
+# define _LIBUNWIND_TARGET_SPARC 1
 # define _LIBUNWIND_CONTEXT_SIZE 167
 # define _LIBUNWIND_CURSOR_SIZE 179
 # define _LIBUNWIND_HIGHEST_DWARF_REGISTER 287

Modified: libunwind/trunk/include/libunwind.h
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/include/libunwind.h?rev=350705&r1=350704&r2=350705&view=diff
==
--- libunwind/trunk/include/libunwind.h (original)
+++ libunwind/trunk/include/libunwind.h Wed Jan  9 04:06:05 2019
@@ -823,4 +823,40 @@ enum {
   UNW_MIPS_LO = 65,
 };
 
+// SPARC registers
+enum {
+  UNW_SPARC_G0 = 0,
+  UNW_SPARC_G1 = 1,
+  UNW_SPARC_G2 = 2,
+  UNW_SPARC_G3 = 3,
+  UNW_SPARC_G4 = 4,
+  UNW_SPARC_G5 = 5,
+  UNW_SPARC_G6 = 6,
+  UNW_SPARC_G7 = 7,
+  UNW_SPARC_O0 = 8,
+  UNW_SPARC_O1 = 9,
+  UNW_SPARC_O2 = 10,
+  UNW_SPARC_O3 = 11,
+  UNW_SPARC_O4 = 12,
+  UNW_SPARC_O5 = 13,
+  UNW_SPARC_O6 = 14,
+  UNW_SPARC_O7 = 15,
+  UNW_SPARC_L0 = 16,
+  UNW_SPARC_L1 = 17,
+  UNW_SPARC_L2 = 18,
+  UNW_SPARC_L3 = 19,
+  UNW_SPARC_L4 = 20,
+  UNW_SPARC_L5 = 21,
+  UNW_SPARC_L6 = 22,
+  UNW_SPARC_L7 = 23,
+  UNW_SPARC_I0 = 24,
+  UNW_SPARC_I1 = 25,
+  UNW_SPARC_I2 = 26,
+  UNW_SPARC_I3 = 27,
+  UNW_SPARC_I4 = 28,
+  UNW_SPARC_I5 = 29,
+  UNW_SPARC_I6 = 30,
+  UNW_SPARC_I7 = 31,
+};
+
 #endif

Modified: libunwind/trunk/src/DwarfInstructions.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfInstructions.hpp?rev=350705&r1=350704&r2=350705&view=diff
==
--- libunwind/trunk/src/DwarfInstructions.hpp (original)
+++ libunwind/trunk/src/DwarfInstructions.hpp Wed Jan  9 04:06:05 2019
@@ -223,6 +223,14 @@ int DwarfInstructions::stepWithDwa
   }
 #endif
 
+#if defined(_LIBUNWIND_TARGET_SPARC)
+  // Skip call site instruction and delay slot
+  returnAddress += 8;
+  // Skip unimp instruction if function returns a struct
+  if ((addressSpace.get32(returnAddress) & 0xC1C0) == 0)
+returnAddress += 4;
+#endif
+
   // Return address is address after call site instruction, so setting IP 
to
   // that does simualates a return.
   newRegisters.setIP(returnAddress);

Modified: libunwind/trunk/src/DwarfParser.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfParser.hpp?rev=350705&r1=350704&r2=350705&view=diff
==
--- libunwind/trunk/src/DwarfParser.hpp (origin

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56444#1350897 , @steveire wrote:

> The timing of this is unfortunate because the change will not be needed soon.


RecursiveASTVisitor should be fixed (even if it won't be used for the parent 
map) unless it's going away entirely - it's supposed to traverse the whole AST.

> Refactoring is underway to extract a generic traverser from ASTDumper. Then 
> the parent/child traversal of ASTMatchFinder can be implemented in terms of 
> it without any of these kinds of problems.

That sounds interesting (though it's not obvious to me why ASTDumper's 
traversal is inherently less prone to these issues than RecursiveASTVisitor).
I'm not inclined to hold off fixing this bug though, because:

- it's blocking other patches, and the changes you're talking about seem likely 
to be quite involved and need some details worked out
- if we are going to switch from the RAV parent map to an 
ASTDumper-traverser-derived map, then the closer that switch is to a no-op, the 
easier it will be to land.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1350935 , @sammccall wrote:

> In D56444#1350897 , @steveire wrote:
>
> >
>
>
> I'm not inclined to hold off fixing this bug though, because:
>
> - it's blocking other patches, and the changes you're talking about seem 
> likely to be quite involved and need some details worked out
> - if we are going to switch from the RAV parent map to an 
> ASTDumper-traverser-derived map, then the closer that switch is to a no-op, 
> the easier it will be to land.


That makes sense to me, but I wanted to draw your attention to the refactoring 
effort.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D56444#1350897 , @steveire wrote:

> The timing of this is unfortunate because the change will not be needed soon. 
> Refactoring is underway to extract a generic traverser from ASTDumper. Then 
> the parent/child traversal of ASTMatchFinder can be implemented in terms of 
> it without any of these kinds of problems. See
>
> http://clang-developers.42468.n3.nabble.com/Dumping-AST-information-to-other-formats-tp4062988p4063004.html


Should still be fixed for 8.0 (probably with this patch). The refactoring is 
more realistic to land in 9.0 i guess?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56446: [Driver] Fix libcxx detection on Darwin with clang run as ./clang

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

…can you land this? :-)


Repository:
  rC Clang

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

https://reviews.llvm.org/D56446



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1350946 , @JonasToth wrote:

> Should still be fixed for 8.0 (probably with this patch). The refactoring is 
> more realistic to land in 9.0 i guess?


That's why I said timing is unfortunate :).


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

r350714 - [Driver] Fix libcxx detection on Darwin with clang run as ./clang

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Jan  9 05:08:11 2019
New Revision: 350714

URL: http://llvm.org/viewvc/llvm-project?rev=350714&view=rev
Log:
[Driver] Fix libcxx detection on Darwin with clang run as ./clang

Summary:
By using '..' instead of fs::parent_path.

The intention of the code was to go from 'path/to/clang/bin' to
'path/to/clang/include'. In most cases parent_path works, however it
would fail when clang is run as './clang'.

This was noticed in Chromium's bug tracker, see
https://bugs.chromium.org/p/chromium/issues/detail?id=919761

Reviewers: arphaman, thakis, EricWF

Reviewed By: arphaman, thakis

Subscribers: christof, cfe-commits

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

Added:
cfe/trunk/test/Tooling/Inputs/mock-libcxx/bin/
cfe/trunk/test/Tooling/Inputs/mock-libcxx/bin/clang
Modified:
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/test/Driver/darwin-stdlib.cpp

Modified: cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Darwin.cpp?rev=350714&r1=350713&r2=350714&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Darwin.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Darwin.cpp Wed Jan  9 05:08:11 2019
@@ -1752,10 +1752,11 @@ void DarwinClang::AddClangCXXStdlibInclu
   break;
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-// Get from 'foo/bin' to 'foo'.
-SmallString<128> P = llvm::sys::path::parent_path(InstallDir);
-// Get to 'foo/include/c++/v1'.
-llvm::sys::path::append(P, "include", "c++", "v1");
+// Get from 'foo/bin' to 'foo/include/c++/v1'.
+SmallString<128> P = InstallDir;
+// Note that InstallDir can be relative, so we have to '..' and not
+// parent_path.
+llvm::sys::path::append(P, "..", "include", "c++", "v1");
 addSystemInclude(DriverArgs, CC1Args, P);
 break;
   }

Modified: cfe/trunk/test/Driver/darwin-stdlib.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/darwin-stdlib.cpp?rev=350714&r1=350713&r2=350714&view=diff
==
--- cfe/trunk/test/Driver/darwin-stdlib.cpp (original)
+++ cfe/trunk/test/Driver/darwin-stdlib.cpp Wed Jan  9 05:08:11 2019
@@ -14,7 +14,7 @@
 // optional absolute include for libc++ from InitHeaderSearch.cpp also fires.
 
 // CHECK-LIBCXX: "-stdlib=libc++"
-// CHECK-LIBCXX: "-internal-isystem" 
"{{[^"]*}}{{/|}}Inputs{{/|}}darwin_toolchain_tree{{/|}}bin{{/|}}include{{/|}}c++{{/|}}v1"
+// CHECK-LIBCXX: "-internal-isystem" 
"{{[^"]*}}{{/|}}Inputs{{/|}}darwin_toolchain_tree{{/|}}bin{{/|}}..{{/|}}include{{/|}}c++{{/|}}v1"
 
 // CHECK-LIBSTDCXX-NOT: -stdlib=libc++
 // CHECK-LIBSTDCXX-NOT: -stdlib=libstdc++

Added: cfe/trunk/test/Tooling/Inputs/mock-libcxx/bin/clang
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Tooling/Inputs/mock-libcxx/bin/clang?rev=350714&view=auto
==
--- cfe/trunk/test/Tooling/Inputs/mock-libcxx/bin/clang (added)
+++ cfe/trunk/test/Tooling/Inputs/mock-libcxx/bin/clang Wed Jan  9 05:08:11 2019
@@ -0,0 +1 @@
+This file is a placeholder to keep its parent directory in git.


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

[PATCH] D56446: [Driver] Fix libcxx detection on Darwin with clang run as ./clang

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC350714: [Driver] Fix libcxx detection on Darwin with clang 
run as ./clang (authored by ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56446?vs=180691&id=180822#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56446

Files:
  lib/Driver/ToolChains/Darwin.cpp
  test/Driver/darwin-stdlib.cpp
  test/Tooling/Inputs/mock-libcxx/bin/clang


Index: test/Driver/darwin-stdlib.cpp
===
--- test/Driver/darwin-stdlib.cpp
+++ test/Driver/darwin-stdlib.cpp
@@ -14,7 +14,7 @@
 // optional absolute include for libc++ from InitHeaderSearch.cpp also fires.
 
 // CHECK-LIBCXX: "-stdlib=libc++"
-// CHECK-LIBCXX: "-internal-isystem" 
"{{[^"]*}}{{/|}}Inputs{{/|}}darwin_toolchain_tree{{/|}}bin{{/|}}include{{/|}}c++{{/|}}v1"
+// CHECK-LIBCXX: "-internal-isystem" 
"{{[^"]*}}{{/|}}Inputs{{/|}}darwin_toolchain_tree{{/|}}bin{{/|}}..{{/|}}include{{/|}}c++{{/|}}v1"
 
 // CHECK-LIBSTDCXX-NOT: -stdlib=libc++
 // CHECK-LIBSTDCXX-NOT: -stdlib=libstdc++
Index: test/Tooling/Inputs/mock-libcxx/bin/clang
===
--- test/Tooling/Inputs/mock-libcxx/bin/clang
+++ test/Tooling/Inputs/mock-libcxx/bin/clang
@@ -0,0 +1 @@
+This file is a placeholder to keep its parent directory in git.
Index: lib/Driver/ToolChains/Darwin.cpp
===
--- lib/Driver/ToolChains/Darwin.cpp
+++ lib/Driver/ToolChains/Darwin.cpp
@@ -1752,10 +1752,11 @@
   break;
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-// Get from 'foo/bin' to 'foo'.
-SmallString<128> P = llvm::sys::path::parent_path(InstallDir);
-// Get to 'foo/include/c++/v1'.
-llvm::sys::path::append(P, "include", "c++", "v1");
+// Get from 'foo/bin' to 'foo/include/c++/v1'.
+SmallString<128> P = InstallDir;
+// Note that InstallDir can be relative, so we have to '..' and not
+// parent_path.
+llvm::sys::path::append(P, "..", "include", "c++", "v1");
 addSystemInclude(DriverArgs, CC1Args, P);
 break;
   }


Index: test/Driver/darwin-stdlib.cpp
===
--- test/Driver/darwin-stdlib.cpp
+++ test/Driver/darwin-stdlib.cpp
@@ -14,7 +14,7 @@
 // optional absolute include for libc++ from InitHeaderSearch.cpp also fires.
 
 // CHECK-LIBCXX: "-stdlib=libc++"
-// CHECK-LIBCXX: "-internal-isystem" "{{[^"]*}}{{/|}}Inputs{{/|}}darwin_toolchain_tree{{/|}}bin{{/|}}include{{/|}}c++{{/|}}v1"
+// CHECK-LIBCXX: "-internal-isystem" "{{[^"]*}}{{/|}}Inputs{{/|}}darwin_toolchain_tree{{/|}}bin{{/|}}..{{/|}}include{{/|}}c++{{/|}}v1"
 
 // CHECK-LIBSTDCXX-NOT: -stdlib=libc++
 // CHECK-LIBSTDCXX-NOT: -stdlib=libstdc++
Index: test/Tooling/Inputs/mock-libcxx/bin/clang
===
--- test/Tooling/Inputs/mock-libcxx/bin/clang
+++ test/Tooling/Inputs/mock-libcxx/bin/clang
@@ -0,0 +1 @@
+This file is a placeholder to keep its parent directory in git.
Index: lib/Driver/ToolChains/Darwin.cpp
===
--- lib/Driver/ToolChains/Darwin.cpp
+++ lib/Driver/ToolChains/Darwin.cpp
@@ -1752,10 +1752,11 @@
   break;
 // On Darwin, libc++ may be installed alongside the compiler in
 // include/c++/v1.
-// Get from 'foo/bin' to 'foo'.
-SmallString<128> P = llvm::sys::path::parent_path(InstallDir);
-// Get to 'foo/include/c++/v1'.
-llvm::sys::path::append(P, "include", "c++", "v1");
+// Get from 'foo/bin' to 'foo/include/c++/v1'.
+SmallString<128> P = InstallDir;
+// Note that InstallDir can be relative, so we have to '..' and not
+// parent_path.
+llvm::sys::path::append(P, "..", "include", "c++", "v1");
 addSystemInclude(DriverArgs, CC1Args, P);
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r350715 - Fix clang-tidy test after r350714. NFC

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Jan  9 05:08:31 2019
New Revision: 350715

URL: http://llvm.org/viewvc/llvm-project?rev=350715&view=rev
Log:
Fix clang-tidy test after r350714. NFC

Added:
clang-tools-extra/trunk/test/clang-tidy/Inputs/mock-libcxx/bin/
clang-tools-extra/trunk/test/clang-tidy/Inputs/mock-libcxx/bin/clang

Added: clang-tools-extra/trunk/test/clang-tidy/Inputs/mock-libcxx/bin/clang
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/Inputs/mock-libcxx/bin/clang?rev=350715&view=auto
==
--- clang-tools-extra/trunk/test/clang-tidy/Inputs/mock-libcxx/bin/clang (added)
+++ clang-tools-extra/trunk/test/clang-tidy/Inputs/mock-libcxx/bin/clang Wed 
Jan  9 05:08:31 2019
@@ -0,0 +1 @@
+This file is a placeholder to keep its parent directory in git.


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

Re: r346652 - Make clang-based tools find libc++ on MacOS

[Ilya Biryukov via cfe-commits]

Glad to help. The fix has landed. Let me know if the problem persists after
it's integrated.

On Tue, Jan 8, 2019 at 7:36 PM Nico Weber  wrote:

> That looks like it should help. Thanks for the quick fix!
>
> On Tue, Jan 8, 2019 at 1:11 PM Ilya Biryukov  wrote:
>
>> Hi Nico,
>>
>> This is clearly a bug, it's supposed to search in a sibling directory.
>> Are you running clang as './clang' in the scripts?  The code seems to
>> break in that case, https://reviews.llvm.org/D56446 should fix this.
>>
>> On Tue, Jan 8, 2019 at 5:12 PM Nico Weber  wrote:
>>
>>> It looks like clang now looks for libc++ headers in -internal-isystem
>>> Release+Asserts/bin/include/c++/v1 , compared to -internal-isystem
>>> Release+Asserts/include/c++/v1. `make install` puts the libc++ headers in
>>> Release+Asserts/include, the old location. Was this an intentional change?
>>>
>>> As-is, this seems to break chromium's clang ability to find libc++
>>> headers (https://crbug.com/919761) because we bundle libc++ headers in
>>> an "include" directory that's a sibling to the "bin" directory (and have
>>> been doing so for 4.5 years, since
>>> https://codereview.chromium.org/281753002).
>>>
>>> On Mon, Nov 12, 2018 at 8:58 AM Ilya Biryukov via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: ibiryukov
 Date: Mon Nov 12 05:55:55 2018
 New Revision: 346652

 URL: http://llvm.org/viewvc/llvm-project?rev=346652&view=rev
 Log:
 Make clang-based tools find libc++ on MacOS

 Summary:
 When they read compiler args from compile_commands.json.
 This change allows to run clang-based tools, like clang-tidy or clangd,
 built from head using the compile_commands.json file produced for XCode
 toolchains.

 On MacOS clang can find the C++ standard library relative to the
 compiler installation dir.

 The logic to do this was based on resource dir as an approximation of
 where the compiler is installed. This broke the tools that read
 'compile_commands.json' and don't ship with the compiler, as they
 typically change resource dir.

 To workaround this, we now use compiler install dir detected by the
 driver
 to better mimic the behavior of the original compiler when replaying the
 compilations using other tools.

 Reviewers: sammccall, arphaman, EricWF

 Reviewed By: sammccall

 Subscribers: ioeric, christof, kadircet, cfe-commits

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

 Added:
 cfe/trunk/test/Tooling/Inputs/mock-libcxx/
 cfe/trunk/test/Tooling/Inputs/mock-libcxx/include/
 cfe/trunk/test/Tooling/Inputs/mock-libcxx/include/c++/
 cfe/trunk/test/Tooling/Inputs/mock-libcxx/include/c++/v1/
 cfe/trunk/test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
 cfe/trunk/test/Tooling/clang-check-mac-libcxx-abspath.cpp
 cfe/trunk/test/Tooling/clang-check-mac-libcxx-relpath.cpp
 Modified:
 cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
 cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
 cfe/trunk/lib/Frontend/InitHeaderSearch.cpp
 cfe/trunk/lib/Tooling/Tooling.cpp

 Modified: cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/HeaderSearchOptions.h?rev=346652&r1=346651&r2=346652&view=diff

 ==
 --- cfe/trunk/include/clang/Lex/HeaderSearchOptions.h (original)
 +++ cfe/trunk/include/clang/Lex/HeaderSearchOptions.h Mon Nov 12
 05:55:55 2018
 @@ -108,6 +108,13 @@ public:
/// etc.).
std::string ResourceDir;

 +  /// Compiler install dir as detected by the Driver.
 +  /// This is typically the directory that contains the clang
 executable, i.e.
 +  /// the 'bin/' subdir of a clang distribution.
 +  /// Only used to add include dirs for libc++ on Darwin. Please avoid
 relying
 +  /// on this field for other purposes.
 +  std::string InstallDir;
 +
/// The directory used for the module cache.
std::string ModuleCachePath;


 Modified: cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp?rev=346652&r1=346651&r2=346652&view=diff

 ==
 --- cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp
 (original)
 +++ cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp Mon Nov
 12 05:55:55 2018
 @@ -11,17 +11,18 @@
  //

  
 //===--===//

 -#include "clang/Frontend/Utils.h"
  #include "clang/Basic/DiagnosticOptions.h"
>>>

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 180823.
kadircet marked 8 inline comments as done.
kadircet added a comment.

Address comments && rebase


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224

Files:
  clangd/SourceCode.h
  clangd/index/Background.cpp
  clangd/index/Background.h
  clangd/index/SymbolCollector.cpp
  test/clangd/background-index.test
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -9,6 +9,7 @@
 
 using testing::_;
 using testing::AllOf;
+using testing::Contains;
 using testing::ElementsAre;
 using testing::Not;
 using testing::UnorderedElementsAre;
@@ -146,7 +147,7 @@
FileURI("unittest:///root/B.cc")}));
 }
 
-TEST_F(BackgroundIndexTest, ShardStorageWriteTest) {
+TEST_F(BackgroundIndexTest, ShardStorageTest) {
   MockFSProvider FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
   void common();
@@ -175,6 +176,16 @@
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+  EXPECT_EQ(Storage.size(), 2U);
+
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(
@@ -278,5 +289,73 @@
   EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("Y")));
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageLoad) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  // Change header.
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  class A_CCnew {};
+  )cpp";
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+
+  // Change source.
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardSource->Symbols,
+  Contains(AllOf(Named("f_b"), Declared(), Defined(;
+}
+
 } // namespace clangd
 } // namespace clang
Index: test/clangd/background-index.test
===
--- test/clangd/background-index.test
+++ test/clangd/background-index.test
@@ -16,6 +16,5 @@
 # RUN: ls %t/.clangd-index/foo.cpp.*.idx
 
 # Test the index is read from disk: delete code and restart clangd.
-# FIXME: This test currently fails as we don't read the index yet.
 # RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | not FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

There seems to be no unexpected changes after rebase




Comment at: clangd/index/Background.cpp:259
+  // if this thread sees the older version but finishes later. This should
+  // be rare in practice.
+  DigestIt.first->second = Hash;

ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > kadircet wrote:
> > > > ilya-biryukov wrote:
> > > > > > "should be rare in practice"
> > > > > And yet, can we avoid this altogether?
> > > > > 
> > > > > Also, I believe it won't be rare. When processing multiple different 
> > > > > TUs, we can easily run into the same header multiple times, possibly 
> > > > > with the different contents.
> > > > > E.g. imagine the index for the whole of clang is being built and the 
> > > > > user is editing `Sema.h` at the same time. We'll definitely be 
> > > > > running into this case over and over again.
> > > > Well I am open to ideas, but currently there is no way of knowing 
> > > > whether this is the newer version for the file. Because only 
> > > > information we have is the digest is different than what we currently 
> > > > have and this doesn't yield any chronological information.
> > > Do we know which request is "newer" when scheduling it?
> > > If so, we could keep the map of the **latest** hashes of the files we've 
> > > seen (in addition to the `IndexedFileDigests`, which correspond to the 
> > > digest for which we built the index IIUC).
> > > 
> > > This would give us a simple invariant of the final state we want to bring 
> > > the index to: `IndexedFileDigests` should correspond to the latest hashes 
> > > seen so far. If not, we have to rebuild the index for the corresponding 
> > > files. That, in turn, gives us a way to resolve conflicts: we should 
> > > never replace with symbols built for the latest version (hash) of the 
> > > file we've seen.
> > > 
> > > Would that work?
> > I am not sure if it would work for non-main files. We update the Hash for 
> > the main files at each indexing action, so we can safely keep track of the 
> > latest versions. But we collect hashes for dependencies while performing 
> > the indexing which happens in parallel. Therefore an indexing action 
> > triggered earlier might get an up-to-date version of a dependency than an 
> > action triggered later-on.
> If updates for each TU are atomic (i.e. all files included in the TU are 
> updated in a single go) we would get the up-to-date index eventually, 
> assuming we rebuild the index when TU dependencies change (we don't schedule 
> rebuilds on changes to included header now, but we're planning to do so at 
> some point).
Sure, that assumption seems more relaxed than the previous one, just wanna make 
sure I got it right:
Your suggested solution assumes: Dependencies of a TU won't change during 
indexing of that TU
Whereas the previous assumption was: Files won't change between close indexing 
actions



Comment at: clangd/index/Background.cpp:197
+Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
+const std::string FileName = Cmd.Filename;
+if (auto Error = index(std::move(Cmd), Storage))

ilya-biryukov wrote:
> use `llvm::StringRef`
If the command fails we want to show the filename but we are moving the Cmd, so 
we need to keep a copy of the filename.



Comment at: clangd/index/Background.h:111
+bool NeedsReIndexing;
+Source(llvm::StringRef Path, bool NeedsReIndexing = true)
+: Path(Path), NeedsReIndexing(NeedsReIndexing) {}

ilya-biryukov wrote:
> NIT: maybe remove the constructor? plain structs can easily be initialized 
> with init lists and adding a constructor forbids per-field assignment, which 
> is a bit nicer in some cases.
> Not very important, since it's a private API, but still.
Once I assign a default value for NeedsReIndexing, I cannot construct Source 
objects with init lists, therefore I've added a constructor instead.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224



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

[PATCH] D56483: [clangd] Add a test for SignatureHelp on dynamic index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Many thanks! Should've been there from the start, sorry about the inconvenience


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56483



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

[PATCH] D56062: [compiler-rt] [test] Detect glibc-2.27+ and XFAIL appropriate tests

[Michał Górny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCRT350717: [test] Detect glibc-2.27+ and XFAIL appropriate 
tests (authored by mgorny, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56062?vs=179456&id=180824#toc

Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56062

Files:
  test/lit.common.cfg
  test/lsan/TestCases/Linux/use_tls_dynamic.cc
  test/msan/dtls_test.c


Index: test/lit.common.cfg
===
--- test/lit.common.cfg
+++ test/lit.common.cfg
@@ -289,6 +289,21 @@
   if android_api_level >= 28:
 config.available_features.add('android-28')
 
+if config.host_os == 'Linux':
+  # detect whether we are using glibc, and which version
+  # NB: 'ldd' is just one of the tools commonly installed as part of glibc
+  ldd_ver_cmd = subprocess.Popen(['ldd', '--version'],
+ stdout=subprocess.PIPE,
+ env={'LANG': 'C'})
+  sout, _ = ldd_ver_cmd.communicate()
+  ver_line = sout.splitlines()[0]
+  if ver_line.startswith(b"ldd "):
+from distutils.version import LooseVersion
+ver = LooseVersion(ver_line.split()[-1].decode())
+# 2.27 introduced some incompatibilities
+if ver >= LooseVersion("2.27"):
+  config.available_features.add("glibc-2.27")
+
 sancovcc_path = os.path.join(config.llvm_tools_dir, "sancov")
 if os.path.exists(sancovcc_path):
   config.available_features.add("has_sancovcc")
Index: test/msan/dtls_test.c
===
--- test/msan/dtls_test.c
+++ test/msan/dtls_test.c
@@ -11,6 +11,11 @@
 
// Reports use-of-uninitialized-value, not analyzed
XFAIL: netbsd
+
+   // This is known to be broken with glibc-2.27+
+   // https://bugs.llvm.org/show_bug.cgi?id=37804
+   XFAIL: glibc-2.27
+
 */
 
 #ifndef BUILD_SO
Index: test/lsan/TestCases/Linux/use_tls_dynamic.cc
===
--- test/lsan/TestCases/Linux/use_tls_dynamic.cc
+++ test/lsan/TestCases/Linux/use_tls_dynamic.cc
@@ -1,4 +1,9 @@
 // Test that dynamically allocated TLS space is included in the root set.
+
+// This is known to be broken with glibc-2.27+
+// https://bugs.llvm.org/show_bug.cgi?id=37804
+// XFAIL: glibc-2.27
+
 // RUN: 
LSAN_BASE="report_objects=1:use_stacks=0:use_registers=0:use_ld_allocations=0"
 // RUN: %clangxx %s -DBUILD_DSO -fPIC -shared -o %t-so.so
 // RUN: %clangxx_lsan %s -o %t


Index: test/lit.common.cfg
===
--- test/lit.common.cfg
+++ test/lit.common.cfg
@@ -289,6 +289,21 @@
   if android_api_level >= 28:
 config.available_features.add('android-28')
 
+if config.host_os == 'Linux':
+  # detect whether we are using glibc, and which version
+  # NB: 'ldd' is just one of the tools commonly installed as part of glibc
+  ldd_ver_cmd = subprocess.Popen(['ldd', '--version'],
+ stdout=subprocess.PIPE,
+ env={'LANG': 'C'})
+  sout, _ = ldd_ver_cmd.communicate()
+  ver_line = sout.splitlines()[0]
+  if ver_line.startswith(b"ldd "):
+from distutils.version import LooseVersion
+ver = LooseVersion(ver_line.split()[-1].decode())
+# 2.27 introduced some incompatibilities
+if ver >= LooseVersion("2.27"):
+  config.available_features.add("glibc-2.27")
+
 sancovcc_path = os.path.join(config.llvm_tools_dir, "sancov")
 if os.path.exists(sancovcc_path):
   config.available_features.add("has_sancovcc")
Index: test/msan/dtls_test.c
===
--- test/msan/dtls_test.c
+++ test/msan/dtls_test.c
@@ -11,6 +11,11 @@
 
// Reports use-of-uninitialized-value, not analyzed
XFAIL: netbsd
+
+   // This is known to be broken with glibc-2.27+
+   // https://bugs.llvm.org/show_bug.cgi?id=37804
+   XFAIL: glibc-2.27
+
 */
 
 #ifndef BUILD_SO
Index: test/lsan/TestCases/Linux/use_tls_dynamic.cc
===
--- test/lsan/TestCases/Linux/use_tls_dynamic.cc
+++ test/lsan/TestCases/Linux/use_tls_dynamic.cc
@@ -1,4 +1,9 @@
 // Test that dynamically allocated TLS space is included in the root set.
+
+// This is known to be broken with glibc-2.27+
+// https://bugs.llvm.org/show_bug.cgi?id=37804
+// XFAIL: glibc-2.27
+
 // RUN: LSAN_BASE="report_objects=1:use_stacks=0:use_registers=0:use_ld_allocations=0"
 // RUN: %clangxx %s -DBUILD_DSO -fPIC -shared -o %t-so.so
 // RUN: %clangxx_lsan %s -o %t
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r350718 - Revert r350648: "Fix clang for r350647: Missed a function rename"

[Florian Hahn via cfe-commits]

Author: fhahn
Date: Wed Jan  9 05:30:47 2019
New Revision: 350718

URL: http://llvm.org/viewvc/llvm-project?rev=350718&view=rev
Log:
Revert r350648: "Fix clang for r350647: Missed a function rename"

The related commit r350647 breaks thread sanitizer on some macOS builders, e.g.
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/52725/

Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=350718&r1=350717&r2=350718&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Wed Jan  9 05:30:47 2019
@@ -53,10 +53,9 @@
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
 #include "llvm/Transforms/Instrumentation.h"
+#include "llvm/Transforms/Instrumentation/MemorySanitizer.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
-#include "llvm/Transforms/Instrumentation/MemorySanitizer.h"
-#include "llvm/Transforms/Instrumentation/ThreadSanitizer.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
@@ -306,7 +305,7 @@ static void addKernelMemorySanitizerPass
 
 static void addThreadSanitizerPass(const PassManagerBuilder &Builder,
legacy::PassManagerBase &PM) {
-  PM.add(createThreadSanitizerLegacyPassPass());
+  PM.add(createThreadSanitizerPass());
 }
 
 static void addDataFlowSanitizerPass(const PassManagerBuilder &Builder,


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

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clangd/index/Index.h:188
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// This field is only meaningful when the symbol is indexed for completion.
   llvm::StringRef Signature;

NIT: Maybe change to `only set when the symbol...`?  "Meaningful" might create 
confusion.



Comment at: clangd/index/SymbolCollector.cpp:543
+
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion))
+return Insert(S);

hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > Most of the fields updated at the bottom aren't useful. However, I feel 
> > > > the documentation is actually important, since Sema only has doc 
> > > > comments for the **current** file and the rest are currently expected 
> > > > to be provided by the index.
> > > > 
> > > > I'm not sure if we already have the code to query the doc comments via 
> > > > index for member completions. If not, it's an oversight.
> > > > In any case, I suggest we **always** store the comments in **dynamic** 
> > > > index. Not storing the comments in the static index is fine, since any 
> > > > data for member completions should be provided by the dynamic index (we 
> > > > see a member in completion ⇒ sema has processed the headers ⇒ the 
> > > > dynamic index should know about those members)
> > > This is a good point.
> > > 
> > > For class member completions, we rely solely on Sema completions (no 
> > > query being queried). I'm not sure it is practical to query the index for 
> > > member completions.
> > > - this means for **every** code completion, we query the index, it may 
> > > slow down completions
> > > - `fuzzyFind` is not supported for class members in our internal index 
> > > service (due to the large number of them)
> > > 
> > > So it turns two possibilities:
> > > 
> > > 1) always store comments (`SymbolCollector` doesn't know whether it is 
> > > used in static index or dynamic index)
> > > 2) or drop them for now, this wouldn't break anything, and add it back 
> > > when we actually use them for class completions
> > > 
> > > I slightly prefer 2) at the moment. WDYT?
> > Yeah, instead of using `fuzzyFind()`, we'll call  `lookup()` to get details 
> > of the symbols we've discovered.
> > It's true that this is going to add some latency, but I hope we have the 
> > latency budget to handle this (these queries should be fast, e.g. we're 
> > doing this for signature help and I haven't seen any noticeable latency 
> > there from the index query, most of the running time is parsing C++).
> > 
> > I also like option 2, but unfortunately we already rely on this to get the 
> > comments in signature help, so this change would actually introduce 
> > regressions there (less used than code completion, but still not nice to 
> > break it)
> > Would the third option of having a config option (e.g. 
> > `SymbolCollector::Options::StoreAllComments`) work?  `clangd-indexer` and 
> > auto-indexer would set the option to false, `indexSymbols` would set the 
> > option to true. We'll both get the optimizations and avoid introducing any 
> > regressions. The plumbing should be no more than a few lines of code.
> Sounds fair. I totally missed `signature help`, it is unfortunate that our 
> test case doesn't find this regression (added one!)
> 
> > Would the third option of having a config option (e.g. 
> > SymbolCollector::Options::StoreAllComments) work?  clangd-indexer and 
> > auto-indexer would set the option to false, indexSymbols would set the 
> > option to true. We'll both get the optimizations and avoid introducing any 
> > regressions. The plumbing should be no more than a few lines of code.
> 
> This is a reasonable solution, but I prefer to do it in a separated patch. 
> Now I make this patch always store docs, which should not introduce any 
> regressions. There is another optimization opportunity here -- unlike header 
> symbols, docs from main-file symbols can be dropped from the index, we can 
> retrieve them from Sema.
Totally, the main file symbol docs are not necessary. OTOH, the savings for the 
main files should be almost negligible, since there are not that many main 
files open at a time and comments is just a small fraction of the information 
we store for each file (preabmles, source code contents, etc.)



Comment at: clangd/index/SymbolCollector.cpp:538
+
+  auto Insert = [this](const Symbol& S) {
+Symbols.insert(S);

NIT: consider inlining the lamda, the code inside it is simple enough.
Up to you, though.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314



___
cfe-commits mailing list
cfe-commits@lists.llvm.o

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@LegalizeAdulthood your stuck on the commit right things right? If you want I 
can commit for you (maybe even later) as you said you are limited on time.


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

https://reviews.llvm.org/D56323



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

LGTM!
You verified that your fixes, fix the issues in LLVM? But it looks good to go.


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

https://reviews.llvm.org/D55433



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

[clang-tools-extra] r350720 - [clangd] Add a test for SignatureHelp on dynamic index.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Jan  9 05:42:03 2019
New Revision: 350720

URL: http://llvm.org/viewvc/llvm-project?rev=350720&view=rev
Log:
[clangd] Add a test for SignatureHelp on dynamic index.

Summary: This would catch regressions caused by future changes of the index.

Reviewers: ilya-biryukov

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

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

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

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=350720&r1=350719&r2=350720&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Wed Jan  9 
05:42:03 2019
@@ -1793,6 +1793,37 @@ TEST(SignatureHelpTest, IndexDocumentati
 SigDoc("Doc from sema";
 }
 
+TEST(SignatureHelpTest, DynamicIndexDocumentation) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.BuildDynamicSymbolIndex = true;
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+
+  FS.Files[testPath("foo.h")] = R"cpp(
+struct Foo {
+   // Member doc
+   int foo();
+};
+  )cpp";
+  Annotations FileContent(R"cpp(
+#include "foo.h"
+void test() {
+  Foo f;
+  f.foo(^);
+}
+  )cpp");
+  auto File = testPath("test.cpp");
+  Server.addDocument(File, FileContent.code());
+  // Wait for the dynamic index being built.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(
+  llvm::cantFail(runSignatureHelp(Server, File, FileContent.point()))
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Member doc";
+}
+
 TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;


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

[PATCH] D56483: [clangd] Add a test for SignatureHelp on dynamic index.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350720: [clangd] Add a test for SignatureHelp on dynamic 
index. (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56483

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


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1793,6 +1793,37 @@
 SigDoc("Doc from sema";
 }
 
+TEST(SignatureHelpTest, DynamicIndexDocumentation) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.BuildDynamicSymbolIndex = true;
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+
+  FS.Files[testPath("foo.h")] = R"cpp(
+struct Foo {
+   // Member doc
+   int foo();
+};
+  )cpp";
+  Annotations FileContent(R"cpp(
+#include "foo.h"
+void test() {
+  Foo f;
+  f.foo(^);
+}
+  )cpp");
+  auto File = testPath("test.cpp");
+  Server.addDocument(File, FileContent.code());
+  // Wait for the dynamic index being built.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(
+  llvm::cantFail(runSignatureHelp(Server, File, FileContent.point()))
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Member doc";
+}
+
 TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1793,6 +1793,37 @@
 SigDoc("Doc from sema";
 }
 
+TEST(SignatureHelpTest, DynamicIndexDocumentation) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.BuildDynamicSymbolIndex = true;
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+
+  FS.Files[testPath("foo.h")] = R"cpp(
+struct Foo {
+   // Member doc
+   int foo();
+};
+  )cpp";
+  Annotations FileContent(R"cpp(
+#include "foo.h"
+void test() {
+  Foo f;
+  f.foo(^);
+}
+  )cpp");
+  auto File = testPath("test.cpp");
+  Server.addDocument(File, FileContent.code());
+  // Wait for the dynamic index being built.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(
+  llvm::cantFail(runSignatureHelp(Server, File, FileContent.point()))
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Member doc";
+}
+
 TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 180827.
hokein marked an inline comment as done.
hokein added a comment.

address review comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314

Files:
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -654,10 +654,15 @@
 void Foo::ssf() {}
   )";
   runSymbolCollector(Header, Main);
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(QName("Foo"), QName("Foo::f"),
-   QName("Foo::g"), QName("Foo::sf"),
-   QName("Foo::ssf"), QName("Foo::x")));
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  QName("Foo"),
+  AllOf(QName("Foo::f"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::g"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::sf"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::ssf"), ReturnType(""), ForCodeCompletion(false)),
+  AllOf(QName("Foo::x"), ReturnType(""), ForCodeCompletion(false;
 }
 
 TEST_F(SymbolCollectorTest, Scopes) {
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -530,6 +530,10 @@
   getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
 S.CanonicalDeclaration = *DeclLoc;
 
+  S.Origin = Opts.Origin;
+  if (ND.getAvailability() == AR_Deprecated)
+S.Flags |= Symbol::Deprecated;
+
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
@@ -539,13 +543,28 @@
   *ASTCtx, *PP, CodeCompletionContext::CCC_Symbol, *CompletionAllocator,
   *CompletionTUInfo,
   /*IncludeBriefComments*/ false);
-  std::string Signature;
-  std::string SnippetSuffix;
-  getSignature(*CCS, &Signature, &SnippetSuffix);
   std::string Documentation =
   formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
   /*CommentsFromHeaders=*/true));
+  // For symbols not indexed for completion (class members), we also store their
+  // docs in the index, because Sema doesn't load the docs from the preamble, we
+  // rely on the index to get the docs.
+  // FIXME: this can be optimized by only storing the docs in dynamic index --
+  // dynamic index should index these symbols when Sema completes a member
+  // completion.
+  S.Documentation = Documentation;
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
+Symbols.insert(S);
+return Symbols.find(S.ID);
+  }
+
+  std::string Signature;
+  std::string SnippetSuffix;
+  getSignature(*CCS, &Signature, &SnippetSuffix);
+  S.Signature = Signature;
+  S.CompletionSnippetSuffix = SnippetSuffix;
   std::string ReturnType = getReturnType(*CCS);
+  S.ReturnType = ReturnType;
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
@@ -555,10 +574,6 @@
 QName, SM, SM.getExpansionLoc(ND.getLocation()), Opts))
   Include = std::move(*Header);
   }
-  S.Signature = Signature;
-  S.CompletionSnippetSuffix = SnippetSuffix;
-  S.Documentation = Documentation;
-  S.ReturnType = ReturnType;
   if (!Include.empty())
 S.IncludeHeaders.emplace_back(Include, 1);
 
@@ -569,9 +584,6 @@
   S.Type = TypeStorage->raw();
   }
 
-  S.Origin = Opts.Origin;
-  if (ND.getAvailability() == AR_Deprecated)
-S.Flags |= Symbol::Deprecated;
   Symbols.insert(S);
   return Symbols.find(S.ID);
 }
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -185,19 +185,23 @@
   SymbolOrigin Origin = SymbolOrigin::Unknown;
   /// A brief description of the symbol that can be appended in the completion
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// Only set when the symbol is indexed for completion.
   llvm::StringRef Signature;
   /// What to insert when completing this symbol, after the symbol name.
   /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
   /// (When snippets are disabled, the symbol name alone is used).
+  /// Only set when the symbol is indexed for completion.
   llvm::StringRef CompletionSnippetSuffix;
   /// Documentation including comment for the symbol declaration.
   llvm::StringRef Documentation;
   /// Type when this symbol is used in an expression. (Short display form).
   /// e.g. return type of a function, or type of a va

RE: r350643 - Limit COFF 'common' emission to <=32 alignment types.

[Keane, Erich via cfe-commits]

Thank you for that!

From: Shoaib Meenai [mailto:smee...@fb.com]
Sent: Tuesday, January 8, 2019 4:48 PM
To: David Majnemer 
Cc: Keane, Erich ; cfe-commits@lists.llvm.org; Martin 
Storsjo 
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

I sent out https://reviews.llvm.org/D56466 to clarify the comment accordingly.

From: David Majnemer mailto:david.majne...@gmail.com>>
Date: Tuesday, January 8, 2019 at 3:21 PM
To: Shoaib Meenai mailto:smee...@fb.com>>
Cc: "Keane, Erich" mailto:erich.ke...@intel.com>>, 
"cfe-commits@lists.llvm.org" 
mailto:cfe-commits@lists.llvm.org>>, Martin Storsjo 
mailto:mar...@martin.st>>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Yes, the MinGW toolchain can handle this by specifying the alignment of a 
common symbol using the aligncomm directive. The MSVC toolchain has no such 
mechanism.

This is why the check uses isKnownWindowsMSVCEnvironment.

On Tue, Jan 8, 2019 at 1:09 PM Shoaib Meenai 
mailto:smee...@fb.com>> wrote:
It checks for both OS=Win32 and Environment=MSVC, so that wouldn't cover other 
COFF environments. wbs (Martin Storsjo) mentioned on IRC that MinGW adds an 
aligncomm directive to specify alignment for common symbols, so perhaps that's 
part of it?

From: "Keane, Erich" mailto:erich.ke...@intel.com>>
Date: Tuesday, January 8, 2019 at 1:04 PM
To: Shoaib Meenai mailto:smee...@fb.com>>, 
"cfe-commits@lists.llvm.org" 
mailto:cfe-commits@lists.llvm.org>>, David Majnemer 
mailto:david.majne...@gmail.com>>
Subject: RE: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Yep, exactly.  I looked, and isKnownWindowsMSVCEnvironment checks for OS=Win32, 
which I believe would be different for other architectures.

From: Shoaib Meenai [mailto:smee...@fb.com]
Sent: Tuesday, January 8, 2019 12:41 PM
To: Keane, Erich mailto:erich.ke...@intel.com>>; 
cfe-commits@lists.llvm.org; David Majnemer 
mailto:david.majne...@gmail.com>>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Ah, looks like you were originally checking for COFF, and then David suggested 
checking for MSVC instead? I'm curious about why, although I'm sure the 
suggestion is legit :)

From: cfe-commits 
mailto:cfe-commits-boun...@lists.llvm.org>> 
on behalf of Shoaib Meenai via cfe-commits 
mailto:cfe-commits@lists.llvm.org>>
Reply-To: Shoaib Meenai mailto:smee...@fb.com>>
Date: Tuesday, January 8, 2019 at 12:39 PM
To: Erich Keane mailto:erich.ke...@intel.com>>, 
"cfe-commits@lists.llvm.org" 
mailto:cfe-commits@lists.llvm.org>>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Why does this check for isKnownWindowsMSVCEnvironment specifically? Wouldn't 
any COFF target (windows-cygnus, windows-gnu, windows-itanium, etc.) have the 
same limitation, since it's an object file format issue and not an ABI issue?

From: cfe-commits 
mailto:cfe-commits-boun...@lists.llvm.org>> 
on behalf of Erich Keane via cfe-commits 
mailto:cfe-commits@lists.llvm.org>>
Reply-To: Erich Keane mailto:erich.ke...@intel.com>>
Date: Tuesday, January 8, 2019 at 10:48 AM
To: "cfe-commits@lists.llvm.org" 
mailto:cfe-commits@lists.llvm.org>>
Subject: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Author: erichkeane
Date: Tue Jan  8 10:44:22 2019
New Revision: 350643

URL: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D350643-26view-3Drev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=Ph9GOtRaQERmqyeJeAJTFwV3sg3q8fE05FlJ3qwNx4I&e=
Log:
Limit COFF 'common' emission to <=32 alignment types.

As reported in PR33035, LLVM crashes if given a common object with an
alignment of greater than 32 bits. This is because the COFF file format
does not support these alignments, so emitting them is broken anyway.

This patch changes any global definitions greater than 32 bit alignment
to no longer be in 'common'.

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.llvm.org_show-5Fbug.cgi-3Fid-3D33035&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=ac1NEHuvztd6jSTCsOUJajkklfeyqdzW-xqtddJ-hvM&e=

Differential Revision: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D56391&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=AucP9Sp-DYHSaOP-sPfpAOrww3xwdh8FjQkHrLZhhyo&e=

Change-Id: I48609289753b7f3b58c5e2bc1712756750fbd45a

Added:
cfe/trunk/test/CodeGen/microsoft-no-common-align.c
Modified:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_t

[PATCH] D56430: Incorrect implicit data-sharing for nested tasks

[Alexey Bataev via Phabricator via cfe-commits]

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

In D56430#1350706 , @smateo wrote:

> Renaming the `isParallelRegion` function to `isImplicitTaskingRegion`.
>
> Shall we rename the `isParallelOrTaskRegion`to 
> `isImplicitOrExplicitTaskingRegion`? It is only called 4 times.
>
> Thanks!


Yes, go ahead and do this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56430



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

[PATCH] D56446: [Driver] Fix libcxx detection on Darwin with clang run as ./clang

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry about the delay.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56446



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

r350724 - Fix typo in comment

[Nico Weber via cfe-commits]

Author: nico
Date: Wed Jan  9 06:19:16 2019
New Revision: 350724

URL: http://llvm.org/viewvc/llvm-project?rev=350724&view=rev
Log:
Fix typo in comment

Modified:
cfe/trunk/include/clang/Sema/Sema.h

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=350724&r1=350723&r2=350724&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Wed Jan  9 06:19:16 2019
@@ -4819,7 +4819,7 @@ public:
   ImplicitExceptionSpecification
   ComputeDefaultedCopyCtorExceptionSpec(CXXMethodDecl *MD);
 
-  /// Determine what sort of exception specification a defautled
+  /// Determine what sort of exception specification a defaulted
   /// copy assignment operator of a class will have, and whether the
   /// parameter will be const.
   ImplicitExceptionSpecification


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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56444#1350849 , @JonasToth wrote:

> In D56444#1350746 , @sammccall wrote:
>
> > @klimek: would it be better to preserve the odd behavior of the 
> > `functionDecl()` matcher, and add a new `functionOrLambdaDecl()`? It seems 
> > too surprising to me.
>
>
> We can change the clang-tidy check as well. I think lambdas should be 
> considered functionDecls, shouldnt they? WDYT @aaron.ballman ?


I think it depends on whether we want the AST matchers to match what the user 
wrote (syntax) or how the program behaves (semantics). If the user writes a 
lambda, they did not syntactically write a function declaration and so it makes 
sense for `functionDecl()` to not match. However, the semantics of a lambda are 
that they act as a functor (in effect) which includes a class declaration, a 
function declaration for the function call operator, conversion operators, etc 
and so it does make sense for users to want to match those semantic components. 
Given that, I kind of think we should have functionDecl() match only functions, 
and give users some other way to match the semantic declarations in a 
consistent manner. Alternatively, we could decide semantics are what we want to 
match (because it's what the AST encodes) and instead we give users a way to 
request to only match syntax.

It's always bothered me that tools like clang-query have behavior like this 
around lambdas, given:

  int main() {
auto l = [](){};
  }

you get behavior like:

  clang-query> m cxxRecordDecl()
  
  Match #1:
  
  1 match.
  clang-query>

> Should still be fixed for 8.0 (probably with this patch). The refactoring is 
> more realistic to land in 9.0 i guess?

9.0, IMO.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56488: clang-cl: Align help texts for /O1 and O2

[Nico Weber via Phabricator via cfe-commits]

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

Makes it a bit easier to see what exactly the difference is.

Also use "same as" instead of "equivalent to", because that's faster to read.


https://reviews.llvm.org/D56488

Files:
  clang/include/clang/Driver/CLCompatOptions.td


Index: clang/include/clang/Driver/CLCompatOptions.td
===
--- clang/include/clang/Driver/CLCompatOptions.td
+++ clang/include/clang/Driver/CLCompatOptions.td
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for size  (same as /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
@@ -143,7 +143,7 @@
 def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>,
   HelpText<"Optimize for speed">;
 def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>,
-  HelpText<"Deprecated (equivalent to /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
+  HelpText<"Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
 def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>,
   HelpText<"Enable frame pointer omission (x86 only)">;
 def : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>,


Index: clang/include/clang/Driver/CLCompatOptions.td
===
--- clang/include/clang/Driver/CLCompatOptions.td
+++ clang/include/clang/Driver/CLCompatOptions.td
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for size  (same as /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
@@ -143,7 +143,7 @@
 def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>,
   HelpText<"Optimize for speed">;
 def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>,
-  HelpText<"Deprecated (equivalent to /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
+  HelpText<"Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
 def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>,
   HelpText<"Enable frame pointer omission (x86 only)">;
 def : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D56444#1351056 , @aaron.ballman 
wrote:

> In D56444#1350849 , @JonasToth wrote:
>
> > In D56444#1350746 , @sammccall 
> > wrote:
> >
> > > @klimek: would it be better to preserve the odd behavior of the 
> > > `functionDecl()` matcher, and add a new `functionOrLambdaDecl()`? It 
> > > seems too surprising to me.
> >
> >
> > We can change the clang-tidy check as well. I think lambdas should be 
> > considered functionDecls, shouldnt they? WDYT @aaron.ballman ?
>
>
> I think it depends on whether we want the AST matchers to match what the user 
> wrote (syntax) or how the program behaves (semantics). If the user writes a 
> lambda, they did not syntactically write a function declaration and so it 
> makes sense for `functionDecl()` to not match. However, the semantics of a 
> lambda are that they act as a functor (in effect) which includes a class 
> declaration, a function declaration for the function call operator, 
> conversion operators, etc and so it does make sense for users to want to 
> match those semantic components. Given that, I kind of think we should have 
> functionDecl() match only functions, and give users some other way to match 
> the semantic declarations in a consistent manner. Alternatively, we could 
> decide semantics are what we want to match (because it's what the AST 
> encodes) and instead we give users a way to request to only match syntax.


The problem is that while I agree it would be nice if we matched either exactly 
the semantic or syntactic form of the code, the reality is that we match 
whatever the AST represents when we visit all nodes. Instead o f promising 
folks something that we can't hold (because nobody has time to fully check 
which matchers will match which form), I think telling people that we match the 
AST is the more honest and less surprising result in the end.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56489: clang-cl: Fix help text for /O: '/O2y-' means '/O2 /Oy-', not '/O2 /y-'

[Nico Weber via Phabricator via cfe-commits]

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

https://reviews.llvm.org/D56489

Files:
  clang/include/clang/Driver/CLCompatOptions.td


Index: clang/include/clang/Driver/CLCompatOptions.td
===
--- clang/include/clang/Driver/CLCompatOptions.td
+++ clang/include/clang/Driver/CLCompatOptions.td
@@ -116,7 +116,7 @@
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
 def _SLASH_O : CLJoined<"O">,
-  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' is the same as '/O2 
/y-'">,
+  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'">,
   MetaVarName<"">;
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;


Index: clang/include/clang/Driver/CLCompatOptions.td
===
--- clang/include/clang/Driver/CLCompatOptions.td
+++ clang/include/clang/Driver/CLCompatOptions.td
@@ -116,7 +116,7 @@
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
 def _SLASH_O : CLJoined<"O">,
-  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">,
+  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'">,
   MetaVarName<"">;
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/SourceCode.h:21
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"

Looks redundant. Remove?



Comment at: clangd/index/Background.cpp:197
+Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
+const std::string FileName = Cmd.Filename;
+if (auto Error = index(std::move(Cmd), Storage))

kadircet wrote:
> ilya-biryukov wrote:
> > use `llvm::StringRef`
> If the command fails we want to show the filename but we are moving the Cmd, 
> so we need to keep a copy of the filename.
Ah, we're moving out of `Cmd` here... I'd say `index` should accept a `const 
CompileCommand&`? If the goal was to optimize for less copies, then we failed - 
now we need to copy the filename string. `index()` is not part of this patch, 
though, so let's not change it.

Could you add a comment that we can't use `StringRef` because we move from Cmd? 
It's easy to miss.



Comment at: clangd/index/Background.cpp:312
+  // Skip if file is already up to date.
+  auto DigestIt = IndexedFileDigests.try_emplace(Path);
+  if (!DigestIt.second && DigestIt.first->second == Hash)

We still update digests and symbols non-atomically in that case. Could we do 
both under the same lock, similar to how it's done in `loadShards`?



Comment at: clangd/index/Background.cpp:259
+  // if this thread sees the older version but finishes later. This should
+  // be rare in practice.
+  DigestIt.first->second = Hash;

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > ilya-biryukov wrote:
> > > > kadircet wrote:
> > > > > ilya-biryukov wrote:
> > > > > > > "should be rare in practice"
> > > > > > And yet, can we avoid this altogether?
> > > > > > 
> > > > > > Also, I believe it won't be rare. When processing multiple 
> > > > > > different TUs, we can easily run into the same header multiple 
> > > > > > times, possibly with the different contents.
> > > > > > E.g. imagine the index for the whole of clang is being built and 
> > > > > > the user is editing `Sema.h` at the same time. We'll definitely be 
> > > > > > running into this case over and over again.
> > > > > Well I am open to ideas, but currently there is no way of knowing 
> > > > > whether this is the newer version for the file. Because only 
> > > > > information we have is the digest is different than what we currently 
> > > > > have and this doesn't yield any chronological information.
> > > > Do we know which request is "newer" when scheduling it?
> > > > If so, we could keep the map of the **latest** hashes of the files 
> > > > we've seen (in addition to the `IndexedFileDigests`, which correspond 
> > > > to the digest for which we built the index IIUC).
> > > > 
> > > > This would give us a simple invariant of the final state we want to 
> > > > bring the index to: `IndexedFileDigests` should correspond to the 
> > > > latest hashes seen so far. If not, we have to rebuild the index for the 
> > > > corresponding files. That, in turn, gives us a way to resolve 
> > > > conflicts: we should never replace with symbols built for the latest 
> > > > version (hash) of the file we've seen.
> > > > 
> > > > Would that work?
> > > I am not sure if it would work for non-main files. We update the Hash for 
> > > the main files at each indexing action, so we can safely keep track of 
> > > the latest versions. But we collect hashes for dependencies while 
> > > performing the indexing which happens in parallel. Therefore an indexing 
> > > action triggered earlier might get an up-to-date version of a dependency 
> > > than an action triggered later-on.
> > If updates for each TU are atomic (i.e. all files included in the TU are 
> > updated in a single go) we would get the up-to-date index eventually, 
> > assuming we rebuild the index when TU dependencies change (we don't 
> > schedule rebuilds on changes to included header now, but we're planning to 
> > do so at some point).
> Sure, that assumption seems more relaxed than the previous one, just wanna 
> make sure I got it right:
> Your suggested solution assumes: Dependencies of a TU won't change during 
> indexing of that TU
> Whereas the previous assumption was: Files won't change between close 
> indexing actions
Exactly. The idea is that eventually we'll start tracking changes and will be 
able to detect even those cases and rebuild the index accordingly. Just trying 
to keep us from drifting away from that model too much.

> files won't change between close indexing actions
This assumption worked fine for indexing actions running right away, but I 
think the situation with loading shards is different: the shards we are loading 
might be old and if we don't want a stale shard (which might be arbitrarily 
old) to overwrite results of the f

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Ilya Biryukov via Phabricator via cfe-commits]

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

LGTM again :-) I bet the savings are less now that we're always storing the 
comments in the static index, so the numbers in the description might be 
outdated.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56444#1351056 , @aaron.ballman 
wrote:

> Given that, I kind of think we should have functionDecl() match only 
> functions, and give users some other way to match the semantic declarations 
> in a consistent manner. Alternatively, we could decide semantics are what we 
> want to match (because it's what the AST encodes) and instead we give users a 
> way to request to only match syntax.


I believe matching the implied semantic nodes is how closer to how matchers 
behave in general (corresponding to the fact that the ASTMatcher 
RecursiveASTVisitor sets `shouldVisitImplicitCode` to true). e.g.

  $ cat ~/test.cc
  void foo() { for (char c : "abc") {} }
  $ bin/clang-query ~/test.cc --
  clang-query> set output detailed-ast
  clang-query> match binaryOperator()
  
  Match #1:
  
  Binding for "root":
  BinaryOperator 0x471f038  
'_Bool' '!='
  |-ImplicitCastExpr 0x471f008  'const char *':'const char *' 

  | `-DeclRefExpr 0x471efc8  'const char *':'const char *' lvalue Var 
0x471ed48 '__begin1' 'const char *':'const char *'
  `-ImplicitCastExpr 0x471f020  'const char *':'const char *' 

`-DeclRefExpr 0x471efe8  'const char *':'const char *' lvalue Var 
0x471edb8 '__end1' 'const char *':'const char *'
  etc

Obviously this is only true when such nodes are present in the AST at the time 
of matching (if I'm understanding Manuel's comment correctly).


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1350999 , @JonasToth wrote:

> LGTM!
>  You verified that your fixes, fix the issues in LLVM? But it looks good to 
> go.


They look good, you asked before...

> P.S. did you request commit rights already?

I do not have commit rights. I'm not sure what constitutes someone who can 
commit, but let me contribute a little more before taking that step,  I have 
another idea for a checker I'd like to try after this one, I just wanted to get 
one under my belt first.


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

https://reviews.llvm.org/D55433



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

[PATCH] D56326: [OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: lib/AST/DeclOpenMP.cpp:164
+  if (NumClauses) {
+Clauses = (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
+setClauses(CL);

No, bad idea. Use tail allocation for the clauses. Check the implementation of 
`OMPRequiresDecl`



Comment at: lib/CodeGen/CodeGenModule.h:1249
+  void EmitOMPDeclareMapper(const OMPDeclareMapperDecl *D,
+   CodeGenFunction *CGF = nullptr);
+

Formatting.



Comment at: lib/Parse/ParseOpenMP.cpp:503
+} else
+  MapperId = DeclNames.getIdentifier(Tok.getIdentifierInfo());
+ConsumeToken();

Enclose substatement in braces.



Comment at: lib/Parse/ParseOpenMP.cpp:589
+
+TypeResult Parser::parseOpenMPDeclareMapperVarDecl(SourceRange *Range,
+   DeclarationName &Name,

Pass `Range` by reference rather than as a pointer



Comment at: lib/Sema/SemaOpenMP.cpp:13587
   // OpenMP [2.13.2, critical construct, Description]
-  // ... where hint-expression is an integer constant expression that evaluates
-  // to a valid lock hint.
+  // ... where hint-expression is an integer constant expression that
+  // evaluates to a valid lock hint.

Restore this comment.



Comment at: lib/Sema/SemaOpenMP.cpp:13624
   // OpenMP [2.7.1, Restrictions]
-  //  chunk_size must be a loop invariant integer expression with a 
positive
-  //  value.
+  //  chunk_size must be a loop invariant integer expression with a
+  //  positive value.

Restore original comment



Comment at: lib/Sema/SemaOpenMP.cpp:13919
 
-// We need to add a data sharing attribute for this variable to make sure 
it
-// is correctly captured. A variable that shows up in a use_device_ptr has
-// similar properties of a first private variable.
+// We need to add a data sharing attribute for this variable to make sure
+// it is correctly captured. A variable that shows up in a use_device_ptr

Restore al the comments that are not related to the patch itself.



Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2868
+TemplateDeclInstantiator::VisitOMPDeclareMapperDecl(OMPDeclareMapperDecl *D) {
+  assert(!D->getType()->isDependentType() &&
+ !D->getType()->isInstantiationDependentType() &&

Why?



Comment at: lib/Serialization/ASTReader.cpp:12304
   for (unsigned i = 0; i != NumVars; ++i)
-Vars.push_back(Record.readSubExpr());
+Vars.push_back(Record.readExpr());
   C->setVarRefs(Vars);

Restore original



Comment at: lib/Serialization/ASTReader.cpp:12328
   for (unsigned i = 0; i < TotalComponents; ++i) {
-Expr *AssociatedExpr = Record.readSubExpr();
+Expr *AssociatedExpr = Record.readExpr();
 auto *AssociatedDecl = Record.readDeclAs();

Restore original


Repository:
  rC Clang

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

https://reviews.llvm.org/D56326



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56444#1351067 , @klimek wrote:

> In D56444#1351056 , @aaron.ballman 
> wrote:
>
> > In D56444#1350849 , @JonasToth 
> > wrote:
> >
> > > In D56444#1350746 , @sammccall 
> > > wrote:
> > >
> > > > @klimek: would it be better to preserve the odd behavior of the 
> > > > `functionDecl()` matcher, and add a new `functionOrLambdaDecl()`? It 
> > > > seems too surprising to me.
> > >
> > >
> > > We can change the clang-tidy check as well. I think lambdas should be 
> > > considered functionDecls, shouldnt they? WDYT @aaron.ballman ?
> >
> >
> > I think it depends on whether we want the AST matchers to match what the 
> > user wrote (syntax) or how the program behaves (semantics). If the user 
> > writes a lambda, they did not syntactically write a function declaration 
> > and so it makes sense for `functionDecl()` to not match. However, the 
> > semantics of a lambda are that they act as a functor (in effect) which 
> > includes a class declaration, a function declaration for the function call 
> > operator, conversion operators, etc and so it does make sense for users to 
> > want to match those semantic components. Given that, I kind of think we 
> > should have functionDecl() match only functions, and give users some other 
> > way to match the semantic declarations in a consistent manner. 
> > Alternatively, we could decide semantics are what we want to match (because 
> > it's what the AST encodes) and instead we give users a way to request to 
> > only match syntax.
>
>
> The problem is that while I agree it would be nice if we matched either 
> exactly the semantic or syntactic form of the code, the reality is that we 
> match whatever the AST represents when we visit all nodes. Instead o f 
> promising folks something that we can't hold (because nobody has time to 
> fully check which matchers will match which form), I think telling people 
> that we match the AST is the more honest and less surprising result in the 
> end.


I both agree and disagree. I think being able to point users to the concrete 
thing we're matching against is really useful (and honest). I also think 
surprising is subjective -- the current behavior with lambdas is baffling to me 
because it's an inconsistent mixture of syntax and semantics that we match 
against and that can be hard to remember when writing matchers. Also, it's hard 
to say we're matching the exact AST for lambdas.

  int main() {
auto l = [](){ return 12; };
int (*fp)() = l;
  }

gives us this AST

  TranslationUnitDecl 0x2c3521ed618 <> 
  |-TypedefDecl 0x2c3521edef0 <>  implicit 
__int128_t '__int128'
  | `-BuiltinType 0x2c3521edbb0 '__int128'
  |-TypedefDecl 0x2c3521edf58 <>  implicit 
__uint128_t 'unsigned __int128'
  | `-BuiltinType 0x2c3521edbd0 'unsigned __int128'
  |-TypedefDecl 0x2c3521ee2a8 <>  implicit 
__NSConstantString '__NSConstantString_tag'
  | `-RecordType 0x2c3521ee030 '__NSConstantString_tag'
  |   `-CXXRecord 0x2c3521edfa8 '__NSConstantString_tag'
  |-TypedefDecl 0x2c3521ee340 <>  implicit 
__builtin_ms_va_list 'char *'
  | `-PointerType 0x2c3521ee300 'char *'
  |   `-BuiltinType 0x2c3521ed6b0 'char'
  |-TypedefDecl 0x2c3521ee3a8 <>  implicit 
__builtin_va_list 'char *'
  | `-PointerType 0x2c3521ee300 'char *'
  |   `-BuiltinType 0x2c3521ed6b0 'char'
  `-FunctionDecl 0x2c3521ee450 
 line:1:5 main 
'int ()'
`-CompoundStmt 0x2c3524c1d00 
  |-DeclStmt 0x2c3524c1a40 
  | `-VarDecl 0x2c3521ee590  col:8 used l '(lambda at 
C:\Users\aballman.GRAMMATECH\Desktop\test.cpp:2:12)':'(lambda at 
C:\Users\aballman.GRAMMATECH\Desktop\test.cpp:2:12)' cinit
  |   `-LambdaExpr 0x2c3524c1830  '(lambda at 
C:\Users\aballman.GRAMMATECH\Desktop\test.cpp:2:12)'
  | |-CXXRecordDecl 0x2c3524c1228  col:12 implicit class 
definition
  | | |-DefinitionData lambda pass_in_registers empty standard_layout 
trivially_copyable literal can_const_default_init
  | | | |-DefaultConstructor defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  | | | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveAssignment
  | | | `-Destructor simple irrelevant trivial
  | | |-CXXMethodDecl 0x2c3524c1360  col:12 used 
constexpr operator() 'int () const' inline
  | | | `-CompoundStmt 0x2c3524c1568 
  | | |   `-ReturnStmt 0x2c3524c1558 
  | | | `-IntegerLiteral 0x2c3524c1408  'int' 12
  | | |-CXXConversionDecl 0x2c3524c16e0  col:12 implicit used 
constexpr operator int (*)() 'int (*() const)()' inline
  | | | `-CompoundStmt 0x2c3524c1c98 
  | | |   `-ReturnStmt 0x2c3524c1c88 
  

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56444#1351096 , @sammccall wrote:

> In D56444#1351056 , @aaron.ballman 
> wrote:
>
> > Given that, I kind of think we should have functionDecl() match only 
> > functions, and give users some other way to match the semantic declarations 
> > in a consistent manner. Alternatively, we could decide semantics are what 
> > we want to match (because it's what the AST encodes) and instead we give 
> > users a way to request to only match syntax.
>
>
> I believe matching the implied semantic nodes is how closer to how matchers 
> behave in general (corresponding to the fact that the ASTMatcher 
> RecursiveASTVisitor sets `shouldVisitImplicitCode` to true). e.g.


I agree that we more closely match on semantics than we do on syntax and I 
think that's the correct default (as it will match the nodes in the AST that 
the user can see through dumping).


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

r350727 - Remove dependency-related arguments in clang-check.

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Jan  9 07:00:06 2019
New Revision: 350727

URL: http://llvm.org/viewvc/llvm-project?rev=350727&view=rev
Log:
Remove dependency-related arguments in clang-check.

This is the default behavior of clang tools, but clang-check overrides default
argument adjusters for some reason.

Modified:
cfe/trunk/tools/clang-check/ClangCheck.cpp

Modified: cfe/trunk/tools/clang-check/ClangCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-check/ClangCheck.cpp?rev=350727&r1=350726&r2=350727&view=diff
==
--- cfe/trunk/tools/clang-check/ClangCheck.cpp (original)
+++ cfe/trunk/tools/clang-check/ClangCheck.cpp Wed Jan  9 07:00:06 2019
@@ -167,6 +167,7 @@ int main(int argc, const char **argv) {
   // Clear adjusters because -fsyntax-only is inserted by the default chain.
   Tool.clearArgumentsAdjusters();
   Tool.appendArgumentsAdjuster(getClangStripOutputAdjuster());
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
 
   // Running the analyzer requires --analyze. Other modes can work with the
   // -fsyntax-only option.


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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351096 , @sammccall wrote:

> In D56444#1351056 , @aaron.ballman 
> wrote:
>
> > Given that, I kind of think we should have functionDecl() match only 
> > functions, and give users some other way to match the semantic declarations 
> > in a consistent manner. Alternatively, we could decide semantics are what 
> > we want to match (because it's what the AST encodes) and instead we give 
> > users a way to request to only match syntax.
>
>
> I believe matching the implied semantic nodes is how closer to how matchers 
> behave in general (corresponding to the fact that the ASTMatcher 
> RecursiveASTVisitor sets `shouldVisitImplicitCode` to true). e.g.
>
>   $ cat ~/test.cc
>   void foo() { for (char c : "abc") {} }
>   $ bin/clang-query ~/test.cc --
>   clang-query> set output detailed-ast
>   clang-query> match binaryOperator()
>  
>   Match #1:
>  
>   Binding for "root":
>   BinaryOperator 0x471f038  
> '_Bool' '!='
>   |-ImplicitCastExpr 0x471f008  'const char *':'const char *' 
> 
>   | `-DeclRefExpr 0x471efc8  'const char *':'const char *' lvalue Var 
> 0x471ed48 '__begin1' 'const char *':'const char *'
>   `-ImplicitCastExpr 0x471f020  'const char *':'const char *' 
> 
> `-DeclRefExpr 0x471efe8  'const char *':'const char *' lvalue Var 
> 0x471edb8 '__end1' 'const char *':'const char *'
>   etc
>
>
> Obviously this is only true when such nodes are present in the AST at the 
> time of matching (if I'm understanding Manuel's comment correctly).


Note that I've fixed that too,

http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/7sLs34

but both the matcher and the dumper need to get the new behavior at the same 
time. I agree that we shouldn't match on implicit nodes. See also

http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/sNny36

See how Manuel had to explain this here years ago: 
https://youtu.be/VqCkCDFLSsc?t=1748

There's more that we should do to simplify AST dumping and matching: 
http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/uBshw1

My point being that we shouldn't try to rush some way of treating lambdas like 
functionDecl()s or anything like that.

I suggest not trying to make any such drastic changes for 8.0, try to fix the 
bug in a minimal way if possible, and have a more considered approach to the 
future of AST Matchers for after the release.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56444#1351127 , @steveire wrote:

> I suggest not trying to make any such drastic changes for 8.0, try to fix the 
> bug in a minimal way if possible, and have a more considered approach to the 
> future of AST Matchers for after the release.


+1


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351125 , @aaron.ballman 
wrote:

> if the location isn't somewhere in user code, then don't consider the node or 
> its children for traversal. However, that may be insufficient and equally as 
> mysterious behavior.


That is exactly what I've implemented. I skip invisible nodes in matching and 
dumping: http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn

This requires the ast dump traverser to be extracted so that the 
'IgnoreInvisble' state can be set when it is dumping, so that it skips over 
such nodes. Currently there is no public API for ASTDumper. That can be changed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351130 , @steveire wrote:

> In D56444#1351125 , @aaron.ballman 
> wrote:
>
> > if the location isn't somewhere in user code, then don't consider the node 
> > or its children for traversal. However, that may be insufficient and 
> > equally as mysterious behavior.
>
>
> That is exactly what I've implemented. I skip invisible nodes in matching and 
> dumping: http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn


Just pasting the AST for comparison with the one Aaron posted:

  int main() {
auto l = [](){ return 12; };
int (*fp)() = l;
  }



  TranslationUnitDecl 0x5604ea6343e8 <> 
  `-FunctionDecl 0x5604ea670470 <:1:1, line:4:1> line:1:5 main 'int 
(void)'
`-CompoundStmt 0x5604ea69b0e0 
  |-DeclStmt 0x5604ea671260 
  | `-VarDecl 0x5604ea6705b0  col:8 used l 'class (lambda at 
:2:12)':'class (lambda at :2:12)' cinit
  |   `-LambdaExpr 0x5604ea670c60  'class (lambda at 
:2:12)'
  | `-CompoundStmt 0x5604ea670990 
  |   `-ReturnStmt 0x5604ea670978 
  | `-IntegerLiteral 0x5604ea670838  'int' 12
  `-DeclStmt 0x5604ea69b0c8 
`-VarDecl 0x5604ea671310  col:9 fp 'int (*)(void)' cinit
  `-DeclRefExpr 0x5604ea69af00  'class (lambda at 
:2:12)':'class (lambda at :2:12)' lvalue Var 0x5604ea6705b0 'l' 
'class (lambda at :2:12)':'class (lambda at :2:12)'




Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D55781: Make CC mangling conditional on the ABI version

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.
Herald added a subscriber: mstorsjo.

Sorry for not noticing this sooner.  The TL;DR is, the current patch does not 
affect PS4, so go ahead; but see the long version for other considerations.

First, the general point: The PS4 ABI does not fit neatly into the sequential 
ClangABI versioning scheme. We forked a while back, and adopted a few 
subsequent changes and didn't adopt other changes. Just to make everyone's life 
less boring. And give me migraines. So, whenever ABI-related things happen, we 
need to be conscious of that (not fitting neatly; my headaches are not a 
consideration).

Second, IIUC the PS4 ABI ignores all the calling-convention attributes that are 
handled specially by this patch. So to that degree, PS4 doesn't care.  We don't 
support the MS-specific ones, in particular.

That said, I've discovered that there are at least a few of these 
non-target-specific CC attributes that we fail to ignore, such as vectorcall, 
and there's internal discussion to sort out what to do about that.  None of 
which should prevent this patch from going forward.


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

https://reviews.llvm.org/D55781



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56444#1351128 , @aaron.ballman 
wrote:

> In D56444#1351127 , @steveire wrote:
>
> > I suggest not trying to make any such drastic changes for 8.0, try to fix 
> > the bug in a minimal way if possible, and have a more considered approach 
> > to the future of AST Matchers for after the release.
>
>
> +1


Can you elaborate?

This patch is an attempt to solve the problem "some nodes associated with 
lambdas have no parents".
This manifests as assertion failures (or with assertions off, incorrect 
results) for some matchers, on some code - people have encountered several 
examples where this happens, it's hard to know where to target a more targeted 
fix.
The invariant violated is "RAV traverses every AST node (when implicit 
traversal is enabled)" - is there a way to fix this issue sufficiently without 
addressing that?

Is the suggestion to make that change, but then modify the semantics of the 
`functionDecl()` etc matchers to hide it? Or something else?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56444#1351130 , @steveire wrote:

> In D56444#1351125 , @aaron.ballman 
> wrote:
>
> > if the location isn't somewhere in user code, then don't consider the node 
> > or its children for traversal. However, that may be insufficient and 
> > equally as mysterious behavior.
>
>
> That is exactly what I've implemented. I skip invisible nodes in matching and 
> dumping: http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn


So what happens when someone asks about the parent of an invisible node?

e.g. `match(hasParent(decl().bind("parent")), X, Ctx)` where X is the 
`operator()` function of a lambda class. (This is basically the case in the bug 
that this patch fixes)


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D55488: Add utility for dumping a label with child nodes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

@rsmith -- do you have opinions on the new format for how we display the child 
node? I think this is a more clear approach, but a second opinion would be nice.




Comment at: include/clang/AST/TextNodeDumper.h:43
+  /// Add a child of the current node.
+  /// Calls doAddChild without arguments
+  template 

Why move this comment down a line? Might as well leave it where it was before 
(feel free to add a full stop to the end of the comment though).



Comment at: include/clang/AST/TextNodeDumper.h:45
+  template 
+  void addChild(Fn doAddChild) {
+return addChild("", doAddChild);

doAddChild -> DoAddChild



Comment at: include/clang/AST/TextNodeDumper.h:48
+  }
+  /// Add a child of the current node with an optional label.
+  /// Calls doAddChild without arguments.

Can you add a newline above this comment to give it some visual space?



Comment at: include/clang/AST/TextNodeDumper.h:51
+  template 
+  void addChild(StringRef Label, Fn doAddChild) {
 // If we're at the top level, there's nothing interesting to do; just

doAddChild -> DoAddChild



Comment at: include/clang/AST/TextNodeDumper.h:68
+// We need to capture an owning-string in the lambda because the lambda 
+// is invoked in a deffered manner.
+std::string LabelStr = Label;

deffered -> deferred



Comment at: include/clang/AST/TextNodeDumper.h:70
+std::string LabelStr = Label;
+auto dumpWithIndent = [this, doAddChild, LabelStr](bool isLastChild) {
   // Print out the appropriate tree structure and work out the prefix for

dumpWithIndent -> DumpWithIndent
isLastChild -> IsLastChild



Comment at: include/clang/AST/TextNodeDumper.h:87
 OS << Prefix << (isLastChild ? '`' : '|') << '-';
+if (!LabelStr.empty()) {
+  OS << LabelStr << ": ";

Elide braces


Repository:
  rC Clang

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

https://reviews.llvm.org/D55488



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

[PATCH] D56492: [clangd] Add Documentations for member completions.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric.

We are missing docs for class member completion -- we don't store
comments in the preamble, so Sema doesn't return any docs. To get docs
for class members, we rely on the index (mostly dynamic index).

Tried it on llvm, didn't get noticeble delay for the completion.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56492

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -729,6 +729,62 @@
 Doc("Doooc"), ReturnType("void";
 }
 
+TEST(CompletionTest, DocumentationFromIndex) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  FS.Files[testPath("foo.h")] = R"cpp(
+  class Foo {
+  public:
+// Doc for foo
+int foo();
+
+// Doc for foo int
+int foo2(int);
+// Doc for foo bool
+int foo2(bool);
+  };
+  )cpp";
+
+  auto File = testPath("bar.cpp");
+  Annotations Test(R"cpp(
+  #include "foo.h"
+  void test() {
+Foo f;
+f.^
+  }
+  )cpp");
+  auto Opts = ClangdServer::optsForTest();
+  {
+Opts.BuildDynamicSymbolIndex = false;
+ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+runAddDocument(Server, File, Test.code());
+auto Results = cantFail(runCodeComplete(Server, File, Test.point(), {}));
+EXPECT_THAT(
+Results.Completions,
+Contains((Named("foo"), Kind(CompletionItemKind::Method), Doc("";
+EXPECT_THAT(
+Results.Completions,
+Contains((Named("foo2"), Kind(CompletionItemKind::Method), Doc("";
+  }
+  {
+Opts.BuildDynamicSymbolIndex = true;
+ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+runAddDocument(Server, File, Test.code());
+clangd::CodeCompleteOptions CCOpts;
+CCOpts.BundleOverloads = true;
+auto Results =
+cantFail(runCodeComplete(Server, File, Test.point(), CCOpts));
+EXPECT_THAT(Results.Completions,
+Contains((Named("foo"), Kind(CompletionItemKind::Method),
+  Doc("Doc for foo";
+// No doc for overload bundle.
+EXPECT_THAT(
+Results.Completions,
+Contains((Named("foo2"), Kind(CompletionItemKind::Method), Doc("";
+  }
+}
+
 TEST(CompletionTest, Documentation) {
   auto Results = completions(
   R"cpp(
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -1366,11 +1366,45 @@
 auto Top = mergeResults(Recorder->Results, IndexResults);
 CodeCompleteResult Output;
 
+// Keys are indices into Output vector.
+llvm::DenseMap OutputIndex;
+LookupRequest DocIndexRequest;
 // Convert the results to final form, assembling the expensive strings.
-for (auto &C : Top) {
-  Output.Completions.push_back(toCodeCompletion(C.first));
-  Output.Completions.back().Score = C.second;
+for (size_t i = 0; i < Top.size(); ++i) {
+  const ScoredBundle &BundleAndScope = Top[i];
+  const CompletionCandidate::Bundle &CandidateBundle = BundleAndScope.first;
+  Output.Completions.push_back(toCodeCompletion(CandidateBundle));
+  Output.Completions.back().Score = BundleAndScope.second;
   Output.Completions.back().CompletionTokenRange = TextEditRange;
+
+  if (Opts.IncludeComments &&
+  Output.Completions.back().Documentation.empty()) {
+if (CandidateBundle.size() == 1) {
+  if (const CodeCompletionResult *SemaR =
+  CandidateBundle.front().SemaResult) {
+if (auto ID =
+getSymbolID(*SemaR, Recorder->CCSema->getSourceManager())) {
+  OutputIndex[i] = *ID;
+  DocIndexRequest.IDs.insert(*ID);
+}
+  }
+}
+  }
+}
+// Sema doesn't load docs from the preamble, so we get the docs from the
+// index and assemble them for the final results.
+if (!DocIndexRequest.IDs.empty() && Opts.Index) {
+  llvm::DenseMap FetchedDocs;
+  Opts.Index->lookup(DocIndexRequest, [&](const Symbol &S) {
+if (!S.Documentation.empty())
+  FetchedDocs[S.ID] = S.Documentation;
+  });
+  for (auto IndexAndID : OutputIndex) {
+auto FetchDocsIt = FetchedDocs.find(IndexAndID.second);
+if (FetchDocsIt != FetchedDocs.end())
+  Output.Completions[IndexAndID.first].Documentation =
+  FetchDocsIt->second;
+  }
 }
 Output.HasMore = Incomplete;
 Output.Context = Recorder->CCContext.getKind();
___
cfe-commits mailing list
cfe-commits@lists.ll

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351145 , @sammccall wrote:

> In D56444#1351128 , @aaron.ballman 
> wrote:
>
> > In D56444#1351127 , @steveire 
> > wrote:
> >
> > > I suggest not trying to make any such drastic changes for 8.0, try to fix 
> > > the bug in a minimal way if possible, and have a more considered approach 
> > > to the future of AST Matchers for after the release.
> >
> >
> > +1
>
>
> Can you elaborate?
>
> This patch is an attempt to solve the problem "some nodes associated with 
> lambdas have no parents".


This is not a new kind of issue. I reported a variant of it too months ago: 
https://bugs.llvm.org/show_bug.cgi?id=37629.

As such I do think that this is not so urgent and can be fixed after the 
traverser class is extracted.

One of the problems is that visually tracking through parents/children of an 
AST dump does not represent the same relationships as can be expressed with 
hasParent() AST Matcher, because AST Matchers use the RAV and AST dumps do not.

However, we can make both use the same class (the generic traverser I am 
extracting from ASTDumper), and then that class of bug will be solved as I 
understand it.

> This manifests as assertion failures (or with assertions off, incorrect 
> results) for some matchers, on some code - people have encountered several 
> examples where this happens, it's hard to know where to target a more 
> targeted fix.
>  The invariant violated is "RAV traverses every AST node (when implicit 
> traversal is enabled)" - is there a way to fix this issue sufficiently 
> without addressing that?

Yes - don't use RAV to traverse parents when AST-matching.

> Is the suggestion to make that change, but then modify the semantics of the 
> `functionDecl()` etc matchers to hide it? Or something else?

My suggestion is to extract the traverser from ASTDumper first, then fix this 
issue.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56492: [clangd] Add Documentations for member completions.

[Haojian Wu via Phabricator via cfe-commits]

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

Add a comment.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56492

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -729,6 +729,64 @@
 Doc("Doooc"), ReturnType("void";
 }
 
+TEST(CompletionTest, DocumentationFromIndex) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  FS.Files[testPath("foo.h")] = R"cpp(
+  class Foo {
+  public:
+// Doc for foo
+int foo();
+
+// Doc for foo int
+int foo2(int);
+// Doc for foo bool
+int foo2(bool);
+  };
+  )cpp";
+
+  auto File = testPath("bar.cpp");
+  Annotations Test(R"cpp(
+  #include "foo.h"
+  void test() {
+Foo f;
+f.^
+  }
+  )cpp");
+  auto Opts = ClangdServer::optsForTest();
+  {
+// Run code completion without index, verify that we don't get any docs from
+// Sema.
+Opts.BuildDynamicSymbolIndex = false;
+ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+runAddDocument(Server, File, Test.code());
+auto Results = cantFail(runCodeComplete(Server, File, Test.point(), {}));
+EXPECT_THAT(
+Results.Completions,
+Contains((Named("foo"), Kind(CompletionItemKind::Method), Doc("";
+EXPECT_THAT(
+Results.Completions,
+Contains((Named("foo2"), Kind(CompletionItemKind::Method), Doc("";
+  }
+  {
+Opts.BuildDynamicSymbolIndex = true;
+ClangdServer Server(CDB, FS, DiagConsumer, Opts);
+runAddDocument(Server, File, Test.code());
+clangd::CodeCompleteOptions CCOpts;
+CCOpts.BundleOverloads = true;
+auto Results =
+cantFail(runCodeComplete(Server, File, Test.point(), CCOpts));
+EXPECT_THAT(Results.Completions,
+Contains((Named("foo"), Kind(CompletionItemKind::Method),
+  Doc("Doc for foo";
+// No doc for overload bundle.
+EXPECT_THAT(
+Results.Completions,
+Contains((Named("foo2"), Kind(CompletionItemKind::Method), Doc("";
+  }
+}
+
 TEST(CompletionTest, Documentation) {
   auto Results = completions(
   R"cpp(
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -1366,11 +1366,45 @@
 auto Top = mergeResults(Recorder->Results, IndexResults);
 CodeCompleteResult Output;
 
+// Keys are indices into Output vector.
+llvm::DenseMap OutputIndex;
+LookupRequest DocIndexRequest;
 // Convert the results to final form, assembling the expensive strings.
-for (auto &C : Top) {
-  Output.Completions.push_back(toCodeCompletion(C.first));
-  Output.Completions.back().Score = C.second;
+for (size_t i = 0; i < Top.size(); ++i) {
+  const ScoredBundle &BundleAndScope = Top[i];
+  const CompletionCandidate::Bundle &CandidateBundle = BundleAndScope.first;
+  Output.Completions.push_back(toCodeCompletion(CandidateBundle));
+  Output.Completions.back().Score = BundleAndScope.second;
   Output.Completions.back().CompletionTokenRange = TextEditRange;
+
+  if (Opts.IncludeComments &&
+  Output.Completions.back().Documentation.empty()) {
+if (CandidateBundle.size() == 1) {
+  if (const CodeCompletionResult *SemaR =
+  CandidateBundle.front().SemaResult) {
+if (auto ID =
+getSymbolID(*SemaR, Recorder->CCSema->getSourceManager())) {
+  OutputIndex[i] = *ID;
+  DocIndexRequest.IDs.insert(*ID);
+}
+  }
+}
+  }
+}
+// Sema doesn't load docs from the preamble, so we get the docs from the
+// index and assemble them for the final results.
+if (!DocIndexRequest.IDs.empty() && Opts.Index) {
+  llvm::DenseMap FetchedDocs;
+  Opts.Index->lookup(DocIndexRequest, [&](const Symbol &S) {
+if (!S.Documentation.empty())
+  FetchedDocs[S.ID] = S.Documentation;
+  });
+  for (auto IndexAndID : OutputIndex) {
+auto FetchDocsIt = FetchedDocs.find(IndexAndID.second);
+if (FetchDocsIt != FetchedDocs.end())
+  Output.Completions[IndexAndID.first].Documentation =
+  FetchDocsIt->second;
+  }
 }
 Output.HasMore = Incomplete;
 Output.Context = Recorder->CCContext.getKind();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351150 , @sammccall wrote:

> In D56444#1351130 , @steveire wrote:
>
> > In D56444#1351125 , @aaron.ballman 
> > wrote:
> >
> > > if the location isn't somewhere in user code, then don't consider the 
> > > node or its children for traversal. However, that may be insufficient and 
> > > equally as mysterious behavior.
> >
> >
> > That is exactly what I've implemented. I skip invisible nodes in matching 
> > and dumping: 
> > http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn
>
>
> So what happens when someone asks about the parent of an invisible node?
>
> e.g. `match(hasParent(decl().bind("parent")), X, Ctx)` where X is the 
> `operator()` function of a lambda class. (This is basically the case in the 
> bug that this patch fixes)


Assuming that whether to skip invisible nodes is part of the `Ctx`, the `X` 
would simply not be in context, just like if the `X` were not in the 
`TraversalScope` of the `Ctx`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

>> Is the suggestion to make that change, but then modify the semantics of the 
>> `functionDecl()` etc matchers to hide it? Or something else?
> 
> My suggestion is to extract the traverser from ASTDumper first, then fix this 
> issue.

I understand that you are working on a clean solution which is fine and can be 
done in 9.0, but:

- this patch is small, almost non-intrusive and can be obsolete in 9.0, i think 
no one would be sad about that
- we need to fix this issue, lambda traversal is absolutly normal in 
ASTMatchers. Once you include the STL in your code, you automatically encounter 
this use case
- we break current code that is in trunk already (for a longer time)
- this patch does not break any other tests so far, so it does not seem to 
inccur bigger issues, on the other hand redoing AST matching most likely will.

The semantic issues that came up make sense to address (within this or a 
similar complex patch), but the current change does not seem to change a lot.
Even if it does, introducing false positives/negatives for lambda-cases is 
still better then leaving the crashing as is.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351182 , @steveire wrote:

> In D56444#1351150 , @sammccall wrote:
>
> > In D56444#1351130 , @steveire 
> > wrote:
> >
> > > In D56444#1351125 , 
> > > @aaron.ballman wrote:
> > >
> > > > if the location isn't somewhere in user code, then don't consider the 
> > > > node or its children for traversal. However, that may be insufficient 
> > > > and equally as mysterious behavior.
> > >
> > >
> > > That is exactly what I've implemented. I skip invisible nodes in matching 
> > > and dumping: 
> > > http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn
> >
> >
> > So what happens when someone asks about the parent of an invisible node?
> >
> > e.g. `match(hasParent(decl().bind("parent")), X, Ctx)` where X is the 
> > `operator()` function of a lambda class. (This is basically the case in the 
> > bug that this patch fixes)
>
>
> Assuming that whether to skip invisible nodes is part of the `Ctx`, the `X` 
> would simply not be in context, just like if the `X` were not in the 
> `TraversalScope` of the `Ctx`.


Note that as I have prototyped it, the system is perhaps too-eager, not 
representing the FunctionDecl (or the ParmVarDecls) at all: 
http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/tTEjs3

I know what the fix is, and the result will probably be something like

  TranslationUnitDecl 0x5604ea6343e8 <> 
  `-FunctionDecl 0x5604ea670470 <:1:1, line:4:1> line:1:5 main 'int 
(void)'
`-CompoundStmt 0x5604ea69b0e0 
  |-DeclStmt 0x5604ea671260 
  | `-VarDecl 0x5604ea6705b0  col:8 used l 'class (lambda at 
:2:12)':'class (lambda at :2:12)' cinit
  |   `-LambdaExpr 0x5604ea670c60  'class (lambda at 
:2:12)'
  | |-ParmVarDecl 0x558298e6c4b8  col:19 used param 
'int'
  | `-CompoundStmt 0x5604ea670990 
  |   `-ReturnStmt 0x5604ea670978 
  | `-IntegerLiteral 0x5604ea670838  'int' 12
  `-DeclStmt 0x5604ea69b0c8 
`-VarDecl 0x5604ea671310  col:9 fp 'int (*)(void)' cinit
  `-DeclRefExpr 0x5604ea69af00  'class (lambda at 
:2:12)':'class (lambda at :2:12)' lvalue Var 0x5604ea6705b0 'l' 
'class (lambda at :2:12)':'class (lambda at :2:12)'

and ``parmVarDecl(hasName('param'), hasParent(lambdaExpr()))` would be true, as 
you would expect from reading the AST.

This is of course subject to change and subject to review.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

ping - @pcc do you have any more comments or can this be committed along with 
D53890 ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53891



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

[PATCH] D56430: Incorrect implicit data-sharing for nested tasks

[Sergi Mateo via Phabricator via cfe-commits]

smateo updated this revision to Diff 180844.
smateo added a comment.

Done! Thanks,

Sergi


Repository:
  rC Clang

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

https://reviews.llvm.org/D56430

Files:
  lib/Sema/SemaOpenMP.cpp
  test/OpenMP/task_messages.cpp

Index: test/OpenMP/task_messages.cpp
===
--- test/OpenMP/task_messages.cpp
+++ test/OpenMP/task_messages.cpp
@@ -8,7 +8,7 @@
 #pragma omp task // expected-error {{unexpected OpenMP directive '#pragma omp task'}}
 
 class S {
-  S(const S &s) { a = s.a + 12; } // expected-note 14 {{implicitly declared private here}}
+  S(const S &s) { a = s.a + 12; } // expected-note 16 {{implicitly declared private here}}
   int a;
 
 public:
@@ -51,6 +51,15 @@
   ++a; // expected-error {{calling a private constructor of class 'S'}}
 #pragma omp task default(shared)
 #pragma omp task
+  // expected-error@+1 {{calling a private constructor of class 'S'}}
+  ++a;
+#pragma omp parallel shared(a)
+#pragma omp task
+#pragma omp task
+  ++a;
+#pragma omp parallel shared(a)
+#pragma omp task default(shared)
+#pragma omp task
   ++a;
 #pragma omp task
 #pragma omp parallel
@@ -205,6 +214,15 @@
   ++sa; // expected-error {{calling a private constructor of class 'S'}}
 #pragma omp task default(shared)
 #pragma omp task
+  // expected-error@+1 {{calling a private constructor of class 'S'}}
+  ++sa;
+#pragma omp parallel shared(sa)
+#pragma omp task
+#pragma omp task
+  ++sa;
+#pragma omp parallel shared(sa)
+#pragma omp task default(shared)
+#pragma omp task
   ++sa;
 #pragma omp task
 #pragma omp parallel
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -676,9 +676,13 @@
   }
 
 };
-bool isParallelOrTaskRegion(OpenMPDirectiveKind DKind) {
-  return isOpenMPParallelDirective(DKind) || isOpenMPTaskingDirective(DKind) ||
- isOpenMPTeamsDirective(DKind) || DKind == OMPD_unknown;
+
+bool isImplicitTaskingRegion(OpenMPDirectiveKind DKind) {
+  return isOpenMPParallelDirective(DKind) || isOpenMPTeamsDirective(DKind);
+}
+
+bool isImplicitOrExplicitTaskingRegion(OpenMPDirectiveKind DKind) {
+  return isImplicitTaskingRegion(DKind) || isOpenMPTaskingDirective(DKind) || DKind == OMPD_unknown;
 }
 
 } // namespace
@@ -819,7 +823,7 @@
   DVar.CKind = OMPC_firstprivate;
   return DVar;
 }
-  } while (I != E && !isParallelOrTaskRegion(I->Directive));
+  } while (I != E && !isImplicitTaskingRegion(I->Directive));
   DVar.CKind =
   (DVarTemp.CKind == OMPC_unknown) ? OMPC_firstprivate : OMPC_shared;
   return DVar;
@@ -1066,7 +1070,7 @@
   if (!isStackEmpty()) {
 iterator I = Iter, E = Stack.back().first.rend();
 Scope *TopScope = nullptr;
-while (I != E && !isParallelOrTaskRegion(I->Directive) &&
+while (I != E && !isImplicitOrExplicitTaskingRegion(I->Directive) &&
!isOpenMPTargetExecutionDirective(I->Directive))
   ++I;
 if (I == E)
@@ -1292,7 +1296,7 @@
   if (FromParent && I != EndI)
 std::advance(I, 1);
   for (; I != EndI; std::advance(I, 1)) {
-if (!DPred(I->Directive) && !isParallelOrTaskRegion(I->Directive))
+if (!DPred(I->Directive) && !isImplicitOrExplicitTaskingRegion(I->Directive))
   continue;
 iterator NewI = I;
 DSAVarData DVar = getDSA(NewI, D);
@@ -1636,7 +1640,7 @@
 auto &&Info = DSAStack->isLoopControlVariable(D);
 if (Info.first ||
 (VD && VD->hasLocalStorage() &&
- isParallelOrTaskRegion(DSAStack->getCurrentDirective())) ||
+ isImplicitOrExplicitTaskingRegion(DSAStack->getCurrentDirective())) ||
 (VD && DSAStack->isForceVarCapturing()))
   return VD ? VD : Info.second;
 DSAStackTy::DSAVarData DVarPrivate =
@@ -2244,7 +2248,7 @@
   // attribute, must have its data-sharing attribute explicitly determined
   // by being listed in a data-sharing attribute clause.
   if (DVar.CKind == OMPC_unknown && Stack->getDefaultDSA() == DSA_none &&
-  isParallelOrTaskRegion(DKind) &&
+  isImplicitOrExplicitTaskingRegion(DKind) &&
   VarsWithInheritedDSA.count(VD) == 0) {
 VarsWithInheritedDSA[VD] = E;
 return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r350732 - [AST] Store the results in OverloadExpr in a trailing array

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Wed Jan  9 07:43:19 2019
New Revision: 350732

URL: http://llvm.org/viewvc/llvm-project?rev=350732&view=rev
Log:
[AST] Store the results in OverloadExpr in a trailing array

Use the newly available space in the bit-fields of Stmt to pack
OverloadExpr, UnresolvedLookupExpr and UnresolvedMemberExpr.

Additionally store the results in the overload set in a trailing array.
This saves 1 pointer + 8 bytes per UnresolvedLookupExpr and
UnresolvedMemberExpr.

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

Reviewed By: rjmccall


Modified:
cfe/trunk/include/clang/AST/ExprCXX.h
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/ExprCXX.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp

Modified: cfe/trunk/include/clang/AST/ExprCXX.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExprCXX.h?rev=350732&r1=350731&r2=350732&view=diff
==
--- cfe/trunk/include/clang/AST/ExprCXX.h (original)
+++ cfe/trunk/include/clang/AST/ExprCXX.h Wed Jan  9 07:43:19 2019
@@ -2658,58 +2658,54 @@ public:
 /// A reference to an overloaded function set, either an
 /// \c UnresolvedLookupExpr or an \c UnresolvedMemberExpr.
 class OverloadExpr : public Expr {
+  friend class ASTStmtReader;
+  friend class ASTStmtWriter;
+
   /// The common name of these declarations.
   DeclarationNameInfo NameInfo;
 
   /// The nested-name-specifier that qualifies the name, if any.
   NestedNameSpecifierLoc QualifierLoc;
 
-  /// The results.  These are undesugared, which is to say, they may
-  /// include UsingShadowDecls.  Access is relative to the naming
-  /// class.
-  // FIXME: Allocate this data after the OverloadExpr subclass.
-  DeclAccessPair *Results = nullptr;
-
-  unsigned NumResults = 0;
-
 protected:
-  /// Whether the name includes info for explicit template
-  /// keyword and arguments.
-  bool HasTemplateKWAndArgsInfo = false;
-
-  OverloadExpr(StmtClass K, const ASTContext &C,
+  OverloadExpr(StmtClass SC, const ASTContext &Context,
NestedNameSpecifierLoc QualifierLoc,
SourceLocation TemplateKWLoc,
const DeclarationNameInfo &NameInfo,
const TemplateArgumentListInfo *TemplateArgs,
UnresolvedSetIterator Begin, UnresolvedSetIterator End,
-   bool KnownDependent,
-   bool KnownInstantiationDependent,
+   bool KnownDependent, bool KnownInstantiationDependent,
bool KnownContainsUnexpandedParameterPack);
 
-  OverloadExpr(StmtClass K, EmptyShell Empty) : Expr(K, Empty) {}
+  OverloadExpr(StmtClass SC, EmptyShell Empty, unsigned NumResults,
+   bool HasTemplateKWAndArgsInfo);
 
-  /// Return the optional template keyword and arguments info.
-  ASTTemplateKWAndArgsInfo *
-  getTrailingASTTemplateKWAndArgsInfo(); // defined far below.
+  /// Return the results. Defined after UnresolvedMemberExpr.
+  inline DeclAccessPair *getTrailingResults();
+  const DeclAccessPair *getTrailingResults() const {
+return const_cast(this)->getTrailingResults();
+  }
 
   /// Return the optional template keyword and arguments info.
+  /// Defined after UnresolvedMemberExpr.
+  inline ASTTemplateKWAndArgsInfo *getTrailingASTTemplateKWAndArgsInfo();
   const ASTTemplateKWAndArgsInfo *getTrailingASTTemplateKWAndArgsInfo() const {
 return const_cast(this)
 ->getTrailingASTTemplateKWAndArgsInfo();
   }
 
-  /// Return the optional template arguments.
-  TemplateArgumentLoc *getTrailingTemplateArgumentLoc(); // defined far below
+  /// Return the optional template arguments. Defined after
+  /// UnresolvedMemberExpr.
+  inline TemplateArgumentLoc *getTrailingTemplateArgumentLoc();
+  const TemplateArgumentLoc *getTrailingTemplateArgumentLoc() const {
+return const_cast(this)->getTrailingTemplateArgumentLoc();
+  }
 
-  void initializeResults(const ASTContext &C,
- UnresolvedSetIterator Begin,
- UnresolvedSetIterator End);
+  bool hasTemplateKWAndArgsInfo() const {
+return OverloadExprBits.HasTemplateKWAndArgsInfo;
+  }
 
 public:
-  friend class ASTStmtReader;
-  friend class ASTStmtWriter;
-
   struct FindResult {
 OverloadExpr *Expression;
 bool IsAddressOfOperand;
@@ -2745,20 +2741,26 @@ public:
   }
 
   /// Gets the naming class of this lookup, if any.
-  CXXRecordDecl *getNamingClass() const;
+  /// Defined after UnresolvedMemberExpr.
+  inline CXXRecordDecl *getNamingClass();
+  const CXXRecordDecl *getNamingClass() const {
+return const_cast(this)->getNamingClass();
+  }
 
   using decls_iterator = UnresolvedSetImpl::iterator;
 
-  decls_iterator decls_begin() const { return UnresolvedSetIterator(Results); }
+  decls_iterator decls_begin() const {
+return UnresolvedSetIterator(getTrailingResults());
+  }
   decls_iterator decls_end() const {
-

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351194 , @JonasToth wrote:

> >> Is the suggestion to make that change, but then modify the semantics of 
> >> the `functionDecl()` etc matchers to hide it? Or something else?
> > 
> > My suggestion is to extract the traverser from ASTDumper first, then fix 
> > this issue.
>
> I understand that you are working on a clean solution which is fine and can 
> be done in 9.0, but:
>
> - this patch is small, almost non-intrusive and can be obsolete in 9.0, i 
> think no one would be sad about that
> - we need to fix this issue, lambda traversal is absolutly normal in 
> ASTMatchers. Once you include the STL in your code, you automatically 
> encounter this use case
> - we break current code that is in trunk already (for a longer time)
> - this patch does not break any other tests so far, so it does not seem to 
> inccur bigger issues, on the other hand redoing AST matching most likely will.
>
>   The semantic issues that came up make sense to address (within this or a 
> similar complex patch), but the current change does not seem to change a lot. 
> Even if it does, introducing false positives/negatives for lambda-cases is 
> still better then leaving the crashing as is.


Yes, I was mostly responding to the suggestion to treat lambdas as 
functionDecls(), which I think is too drastic to do now, with other solutions 
on the horizon which may be better.

I don't object to something targeted to fixing the assert as you describe and 
which this patch seems to be. Sorry if that wasn't clear.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56430: Incorrect implicit data-sharing for nested tasks

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Will you commit this patch? Or I should do it?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56430



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

[PATCH] D56368: [AST] Store the results in OverloadExpr in a trailing array

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350732: [AST] Store the results in OverloadExpr in a 
trailing array (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56368?vs=180416&id=180845#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56368

Files:
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/include/clang/AST/Stmt.h
  cfe/trunk/lib/AST/ExprCXX.cpp
  cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
  cfe/trunk/lib/Serialization/ASTWriterStmt.cpp

Index: cfe/trunk/include/clang/AST/ExprCXX.h
===
--- cfe/trunk/include/clang/AST/ExprCXX.h
+++ cfe/trunk/include/clang/AST/ExprCXX.h
@@ -2658,58 +2658,54 @@
 /// A reference to an overloaded function set, either an
 /// \c UnresolvedLookupExpr or an \c UnresolvedMemberExpr.
 class OverloadExpr : public Expr {
+  friend class ASTStmtReader;
+  friend class ASTStmtWriter;
+
   /// The common name of these declarations.
   DeclarationNameInfo NameInfo;
 
   /// The nested-name-specifier that qualifies the name, if any.
   NestedNameSpecifierLoc QualifierLoc;
 
-  /// The results.  These are undesugared, which is to say, they may
-  /// include UsingShadowDecls.  Access is relative to the naming
-  /// class.
-  // FIXME: Allocate this data after the OverloadExpr subclass.
-  DeclAccessPair *Results = nullptr;
-
-  unsigned NumResults = 0;
-
 protected:
-  /// Whether the name includes info for explicit template
-  /// keyword and arguments.
-  bool HasTemplateKWAndArgsInfo = false;
-
-  OverloadExpr(StmtClass K, const ASTContext &C,
+  OverloadExpr(StmtClass SC, const ASTContext &Context,
NestedNameSpecifierLoc QualifierLoc,
SourceLocation TemplateKWLoc,
const DeclarationNameInfo &NameInfo,
const TemplateArgumentListInfo *TemplateArgs,
UnresolvedSetIterator Begin, UnresolvedSetIterator End,
-   bool KnownDependent,
-   bool KnownInstantiationDependent,
+   bool KnownDependent, bool KnownInstantiationDependent,
bool KnownContainsUnexpandedParameterPack);
 
-  OverloadExpr(StmtClass K, EmptyShell Empty) : Expr(K, Empty) {}
+  OverloadExpr(StmtClass SC, EmptyShell Empty, unsigned NumResults,
+   bool HasTemplateKWAndArgsInfo);
 
-  /// Return the optional template keyword and arguments info.
-  ASTTemplateKWAndArgsInfo *
-  getTrailingASTTemplateKWAndArgsInfo(); // defined far below.
+  /// Return the results. Defined after UnresolvedMemberExpr.
+  inline DeclAccessPair *getTrailingResults();
+  const DeclAccessPair *getTrailingResults() const {
+return const_cast(this)->getTrailingResults();
+  }
 
   /// Return the optional template keyword and arguments info.
+  /// Defined after UnresolvedMemberExpr.
+  inline ASTTemplateKWAndArgsInfo *getTrailingASTTemplateKWAndArgsInfo();
   const ASTTemplateKWAndArgsInfo *getTrailingASTTemplateKWAndArgsInfo() const {
 return const_cast(this)
 ->getTrailingASTTemplateKWAndArgsInfo();
   }
 
-  /// Return the optional template arguments.
-  TemplateArgumentLoc *getTrailingTemplateArgumentLoc(); // defined far below
+  /// Return the optional template arguments. Defined after
+  /// UnresolvedMemberExpr.
+  inline TemplateArgumentLoc *getTrailingTemplateArgumentLoc();
+  const TemplateArgumentLoc *getTrailingTemplateArgumentLoc() const {
+return const_cast(this)->getTrailingTemplateArgumentLoc();
+  }
 
-  void initializeResults(const ASTContext &C,
- UnresolvedSetIterator Begin,
- UnresolvedSetIterator End);
+  bool hasTemplateKWAndArgsInfo() const {
+return OverloadExprBits.HasTemplateKWAndArgsInfo;
+  }
 
 public:
-  friend class ASTStmtReader;
-  friend class ASTStmtWriter;
-
   struct FindResult {
 OverloadExpr *Expression;
 bool IsAddressOfOperand;
@@ -2745,20 +2741,26 @@
   }
 
   /// Gets the naming class of this lookup, if any.
-  CXXRecordDecl *getNamingClass() const;
+  /// Defined after UnresolvedMemberExpr.
+  inline CXXRecordDecl *getNamingClass();
+  const CXXRecordDecl *getNamingClass() const {
+return const_cast(this)->getNamingClass();
+  }
 
   using decls_iterator = UnresolvedSetImpl::iterator;
 
-  decls_iterator decls_begin() const { return UnresolvedSetIterator(Results); }
+  decls_iterator decls_begin() const {
+return UnresolvedSetIterator(getTrailingResults());
+  }
   decls_iterator decls_end() const {
-return UnresolvedSetIterator(Results + NumResults);
+return UnresolvedSetIterator(getTrailingResults() + getNumDecls());
   }
   llvm::iterator_range decls() const {
 return llvm::make_range(decls_begin(), decls_end());
   }
 
   /// Gets the number of declarations in the unresolved set.
-  unsig

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Can you add a test that checks that a lambda declared within another lambda 
also traverses as expected?

In D56444#1351145 , @sammccall wrote:

> In D56444#1351128 , @aaron.ballman 
> wrote:
>
> > In D56444#1351127 , @steveire 
> > wrote:
> >
> > > I suggest not trying to make any such drastic changes for 8.0, try to fix 
> > > the bug in a minimal way if possible, and have a more considered approach 
> > > to the future of AST Matchers for after the release.
> >
> >
> > +1
>
>
> Can you elaborate?


I can elaborate my +1. :-) I don't think we should try to do anything remotely 
close to what I was talking about with semantic vs syntactic traversals in 8.0, 
but only consider it for 9.0. Instead, I prefer this minimal fix that addresses 
the regression with parent maps. We can revisit the question about adding a new 
traversal model later.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56430: Incorrect implicit data-sharing for nested tasks

[Sergi Mateo via Phabricator via cfe-commits]

smateo added a comment.

I don't have commit access yet. Would you mind to commit it for me? Thanks!,

Sergi


Repository:
  rC Clang

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

https://reviews.llvm.org/D56430



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

[PATCH] D56430: Incorrect implicit data-sharing for nested tasks

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D56430#1351212 , @smateo wrote:

> I don't have commit access yet. Would you mind to commit it for me? Thanks!,
>
> Sergi


Sure, no problems.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56430



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

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D56314#1351091 , @ilya-biryukov 
wrote:

> LGTM again :-) I bet the savings are less now that we're always storing the 
> comments in the static index, so the numbers in the description might be 
> outdated.


Yes, fixed.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314



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

[PATCH] D56492: [clangd] Add Documentations for member completions.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:1373
 // Convert the results to final form, assembling the expensive strings.
-for (auto &C : Top) {
-  Output.Completions.push_back(toCodeCompletion(C.first));
-  Output.Completions.back().Score = C.second;
+for (size_t i = 0; i < Top.size(); ++i) {
+  const ScoredBundle &BundleAndScope = Top[i];

naming NIT: the loop var should be `I`



Comment at: clangd/CodeComplete.cpp:1375
+  const ScoredBundle &BundleAndScope = Top[i];
+  const CompletionCandidate::Bundle &CandidateBundle = 
BundleAndScope.first;
+  Output.Completions.push_back(toCodeCompletion(CandidateBundle));

Since we already have two variables, maybe deconstruct the pair into one of 
them instead of keeping a variable for a pair too? I.e.
```
auto &Bundle = Top[I].first;
auto &Score = Top[I].second;
```

should make the code a little simpler




Comment at: clangd/CodeComplete.cpp:1382
+  Output.Completions.back().Documentation.empty()) {
+if (CandidateBundle.size() == 1) {
+  if (const CodeCompletionResult *SemaR =

NIT: use early exits (see the [[ 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
 | LLVM Style guide  ]]) to reduce nesting here.

```
if (size() != 1)
  continue;
auto *SemaR = ;
if (!SemaR)
  continue;
// ...


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56492



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

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D56411#1350233 , @jlebar wrote:

>   __host__ void bar() {}
>   __device__ int bar() { return 0; }
>   __host__ __device__ void foo() { int x = bar(); }
>   template  __global__ void kernel() { devF();}
>  
>   kernel();
>
>
>
>
> > we DTRT for this case. Here __host__ bar needs to return int since foo() 
> > expects that. will add a test for that.
>
> `__host__ bar()` should not need to return int if `foo` is inline (or 
> templated), because then we should never codegen `foo` for host.  I guess my 
> question is, we should be sure that `kernel()` does not force an 
> inline/templated `foo` to be codegen'ed for host.  (Sorry that wasn't more 
> clear before.)


Sorry I am not quite get it. bar() is a `__host__` function with definition, so 
clang does codegen for it. clang also does codegen for foo() since it has 
`__host__ __device__` attribute.


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

https://reviews.llvm.org/D56411



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

r350734 - Incorrect implicit data-sharing for nested tasks

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Wed Jan  9 07:58:05 2019
New Revision: 350734

URL: http://llvm.org/viewvc/llvm-project?rev=350734&view=rev
Log:
Incorrect implicit data-sharing for nested tasks

Summary:
There is a minor issue in how the implicit data-sharings for nested tasks are 
computed.

For the following example:
```
int x;
#pragma omp task shared(x)
#pragma omp task
x++;
```
We compute an implicit data-sharing of shared for `x` in the second task 
although I think that it should be firstprivate. Below you can find the part of 
the OpenMP spec that covers this example:
- // In a task generating construct, if no default clause is present, a 
variable for which the data-sharing attribute is not determined by the rules 
above and that in the enclosing context is determined to be shared by all 
implicit tasks bound to the current team is shared.//
- //In a task generating construct, if no default clause is present, a variable 
for which the data-sharing attribute is not determined by the rules above is 
firstprivate.//

Since each implicit-task has its own copy of `x`, we shouldn't apply the first 
rule.

Reviewers: ABataev

Reviewed By: ABataev

Subscribers: cfe-commits, rogfer01

Tags: #openmp

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

Modified:
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/test/OpenMP/task_messages.cpp

Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOpenMP.cpp?rev=350734&r1=350733&r2=350734&view=diff
==
--- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)
+++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Wed Jan  9 07:58:05 2019
@@ -676,9 +676,13 @@ public:
   }
 
 };
-bool isParallelOrTaskRegion(OpenMPDirectiveKind DKind) {
-  return isOpenMPParallelDirective(DKind) || isOpenMPTaskingDirective(DKind) ||
- isOpenMPTeamsDirective(DKind) || DKind == OMPD_unknown;
+
+bool isImplicitTaskingRegion(OpenMPDirectiveKind DKind) {
+  return isOpenMPParallelDirective(DKind) || isOpenMPTeamsDirective(DKind);
+}
+
+bool isImplicitOrExplicitTaskingRegion(OpenMPDirectiveKind DKind) {
+  return isImplicitTaskingRegion(DKind) || isOpenMPTaskingDirective(DKind) || 
DKind == OMPD_unknown;
 }
 
 } // namespace
@@ -819,7 +823,7 @@ DSAStackTy::DSAVarData DSAStackTy::getDS
   DVar.CKind = OMPC_firstprivate;
   return DVar;
 }
-  } while (I != E && !isParallelOrTaskRegion(I->Directive));
+  } while (I != E && !isImplicitTaskingRegion(I->Directive));
   DVar.CKind =
   (DVarTemp.CKind == OMPC_unknown) ? OMPC_firstprivate : OMPC_shared;
   return DVar;
@@ -1066,7 +1070,7 @@ bool DSAStackTy::isOpenMPLocal(VarDecl *
   if (!isStackEmpty()) {
 iterator I = Iter, E = Stack.back().first.rend();
 Scope *TopScope = nullptr;
-while (I != E && !isParallelOrTaskRegion(I->Directive) &&
+while (I != E && !isImplicitOrExplicitTaskingRegion(I->Directive) &&
!isOpenMPTargetExecutionDirective(I->Directive))
   ++I;
 if (I == E)
@@ -1292,7 +1296,7 @@ DSAStackTy::hasDSA(ValueDecl *D,
   if (FromParent && I != EndI)
 std::advance(I, 1);
   for (; I != EndI; std::advance(I, 1)) {
-if (!DPred(I->Directive) && !isParallelOrTaskRegion(I->Directive))
+if (!DPred(I->Directive) && 
!isImplicitOrExplicitTaskingRegion(I->Directive))
   continue;
 iterator NewI = I;
 DSAVarData DVar = getDSA(NewI, D);
@@ -1636,7 +1640,7 @@ VarDecl *Sema::isOpenMPCapturedDecl(Valu
 auto &&Info = DSAStack->isLoopControlVariable(D);
 if (Info.first ||
 (VD && VD->hasLocalStorage() &&
- isParallelOrTaskRegion(DSAStack->getCurrentDirective())) ||
+ isImplicitOrExplicitTaskingRegion(DSAStack->getCurrentDirective())) ||
 (VD && DSAStack->isForceVarCapturing()))
   return VD ? VD : Info.second;
 DSAStackTy::DSAVarData DVarPrivate =
@@ -2244,7 +2248,7 @@ public:
   // attribute, must have its data-sharing attribute explicitly determined
   // by being listed in a data-sharing attribute clause.
   if (DVar.CKind == OMPC_unknown && Stack->getDefaultDSA() == DSA_none &&
-  isParallelOrTaskRegion(DKind) &&
+  isImplicitOrExplicitTaskingRegion(DKind) &&
   VarsWithInheritedDSA.count(VD) == 0) {
 VarsWithInheritedDSA[VD] = E;
 return;

Modified: cfe/trunk/test/OpenMP/task_messages.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/task_messages.cpp?rev=350734&r1=350733&r2=350734&view=diff
==
--- cfe/trunk/test/OpenMP/task_messages.cpp (original)
+++ cfe/trunk/test/OpenMP/task_messages.cpp Wed Jan  9 07:58:05 2019
@@ -8,7 +8,7 @@ void foo() {
 #pragma omp task // expected-error {{unexpected OpenMP directive '#pragma omp 
task'}}
 
 class S {
-  S(const S &s) { a = s.a + 12; } // expected-note 14 {{implicitly declared 
private here}}
+  S(

[PATCH] D56430: Incorrect implicit data-sharing for nested tasks

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350734: Incorrect implicit data-sharing for nested tasks 
(authored by ABataev, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56430?vs=180844&id=180847#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56430

Files:
  cfe/trunk/lib/Sema/SemaOpenMP.cpp
  cfe/trunk/test/OpenMP/task_messages.cpp

Index: cfe/trunk/test/OpenMP/task_messages.cpp
===
--- cfe/trunk/test/OpenMP/task_messages.cpp
+++ cfe/trunk/test/OpenMP/task_messages.cpp
@@ -8,7 +8,7 @@
 #pragma omp task // expected-error {{unexpected OpenMP directive '#pragma omp task'}}
 
 class S {
-  S(const S &s) { a = s.a + 12; } // expected-note 14 {{implicitly declared private here}}
+  S(const S &s) { a = s.a + 12; } // expected-note 16 {{implicitly declared private here}}
   int a;
 
 public:
@@ -51,6 +51,15 @@
   ++a; // expected-error {{calling a private constructor of class 'S'}}
 #pragma omp task default(shared)
 #pragma omp task
+  // expected-error@+1 {{calling a private constructor of class 'S'}}
+  ++a;
+#pragma omp parallel shared(a)
+#pragma omp task
+#pragma omp task
+  ++a;
+#pragma omp parallel shared(a)
+#pragma omp task default(shared)
+#pragma omp task
   ++a;
 #pragma omp task
 #pragma omp parallel
@@ -205,6 +214,15 @@
   ++sa; // expected-error {{calling a private constructor of class 'S'}}
 #pragma omp task default(shared)
 #pragma omp task
+  // expected-error@+1 {{calling a private constructor of class 'S'}}
+  ++sa;
+#pragma omp parallel shared(sa)
+#pragma omp task
+#pragma omp task
+  ++sa;
+#pragma omp parallel shared(sa)
+#pragma omp task default(shared)
+#pragma omp task
   ++sa;
 #pragma omp task
 #pragma omp parallel
Index: cfe/trunk/lib/Sema/SemaOpenMP.cpp
===
--- cfe/trunk/lib/Sema/SemaOpenMP.cpp
+++ cfe/trunk/lib/Sema/SemaOpenMP.cpp
@@ -676,9 +676,13 @@
   }
 
 };
-bool isParallelOrTaskRegion(OpenMPDirectiveKind DKind) {
-  return isOpenMPParallelDirective(DKind) || isOpenMPTaskingDirective(DKind) ||
- isOpenMPTeamsDirective(DKind) || DKind == OMPD_unknown;
+
+bool isImplicitTaskingRegion(OpenMPDirectiveKind DKind) {
+  return isOpenMPParallelDirective(DKind) || isOpenMPTeamsDirective(DKind);
+}
+
+bool isImplicitOrExplicitTaskingRegion(OpenMPDirectiveKind DKind) {
+  return isImplicitTaskingRegion(DKind) || isOpenMPTaskingDirective(DKind) || DKind == OMPD_unknown;
 }
 
 } // namespace
@@ -819,7 +823,7 @@
   DVar.CKind = OMPC_firstprivate;
   return DVar;
 }
-  } while (I != E && !isParallelOrTaskRegion(I->Directive));
+  } while (I != E && !isImplicitTaskingRegion(I->Directive));
   DVar.CKind =
   (DVarTemp.CKind == OMPC_unknown) ? OMPC_firstprivate : OMPC_shared;
   return DVar;
@@ -1066,7 +1070,7 @@
   if (!isStackEmpty()) {
 iterator I = Iter, E = Stack.back().first.rend();
 Scope *TopScope = nullptr;
-while (I != E && !isParallelOrTaskRegion(I->Directive) &&
+while (I != E && !isImplicitOrExplicitTaskingRegion(I->Directive) &&
!isOpenMPTargetExecutionDirective(I->Directive))
   ++I;
 if (I == E)
@@ -1292,7 +1296,7 @@
   if (FromParent && I != EndI)
 std::advance(I, 1);
   for (; I != EndI; std::advance(I, 1)) {
-if (!DPred(I->Directive) && !isParallelOrTaskRegion(I->Directive))
+if (!DPred(I->Directive) && !isImplicitOrExplicitTaskingRegion(I->Directive))
   continue;
 iterator NewI = I;
 DSAVarData DVar = getDSA(NewI, D);
@@ -1636,7 +1640,7 @@
 auto &&Info = DSAStack->isLoopControlVariable(D);
 if (Info.first ||
 (VD && VD->hasLocalStorage() &&
- isParallelOrTaskRegion(DSAStack->getCurrentDirective())) ||
+ isImplicitOrExplicitTaskingRegion(DSAStack->getCurrentDirective())) ||
 (VD && DSAStack->isForceVarCapturing()))
   return VD ? VD : Info.second;
 DSAStackTy::DSAVarData DVarPrivate =
@@ -2244,7 +2248,7 @@
   // attribute, must have its data-sharing attribute explicitly determined
   // by being listed in a data-sharing attribute clause.
   if (DVar.CKind == OMPC_unknown && Stack->getDefaultDSA() == DSA_none &&
-  isParallelOrTaskRegion(DKind) &&
+  isImplicitOrExplicitTaskingRegion(DKind) &&
   VarsWithInheritedDSA.count(VD) == 0) {
 VarsWithInheritedDSA[VD] = E;
 return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56444#1351174 , @steveire wrote:

> In D56444#1351145 , @sammccall wrote:
>
> > This manifests as assertion failures (or with assertions off, incorrect 
> > results) for some matchers, on some code - people have encountered several 
> > examples where this happens, it's hard to know where to target a more 
> > targeted fix.
> >  The invariant violated is "RAV traverses every AST node (when implicit 
> > traversal is enabled)" - is there a way to fix this issue sufficiently 
> > without addressing that?
>
>
> Yes - don't use RAV to traverse parents when AST-matching.


OK, this is certainly a much more invasive change, and isn't going to make the 
release.
I understand that you don't think this crash is worth fixing for 8.0, while 
others disagree.
I don't particularly have a dog in this fight, but having seen now three 
reports of this crash makes me nervous about ignoring it.

In D56444#1351182 , @steveire wrote:

> In D56444#1351150 , @sammccall wrote:
>
> > In D56444#1351130 , @steveire 
> > wrote:
> >
> > > In D56444#1351125 , 
> > > @aaron.ballman wrote:
> > >
> > > > if the location isn't somewhere in user code, then don't consider the 
> > > > node or its children for traversal. However, that may be insufficient 
> > > > and equally as mysterious behavior.
> > >
> > >
> > > That is exactly what I've implemented. I skip invisible nodes in matching 
> > > and dumping: 
> > > http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn
> >
> >
> > So what happens when someone asks about the parent of an invisible node?
> >
> > e.g. `match(hasParent(decl().bind("parent")), X, Ctx)` where X is the 
> > `operator()` function of a lambda class. (This is basically the case in the 
> > bug that this patch fixes)
>
>
> Assuming that whether to skip invisible nodes is part of the `Ctx`, the `X` 
> would simply not be in context, just like if the `X` were not in the 
> `TraversalScope` of the `Ctx`.


Ah, so any skipped nodes would not have any parents at all.
The implementation of the idea would just be removing the assert - instead of 
assuming non-visited nodes is a bug, we assume they're *meant* to be excluded.

However, such nodes are very easy to reach, inside or outside of a matcher.
e.g. `callExpr(callee(functionDecl()))` will match a lambda call, and "func" 
will be the `operator()`.
Having such nodes then not work with parent/ancestor matchers seems surprising 
and not **obviously** the best design choice (vs e.g. visiting all nodes 
including implicit, or breaking symmetry between downward and upward traversal).

I guess what I'm saying is your suggestion seems like a significant change to 
the design of (ancestor) matchers, and that the approach in this patch is the 
more conservative one - making e.g. the implicit `FunctionDecl`s visible to 
matchers is consistent with `clang -ast-dump` and the behavior of matchers on 
other parts of the AST, even if that's something you'd like to change on an 
LLVM-9 timeline.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D56444#1351247 , @sammccall wrote:

> In D56444#1351174 , @steveire wrote:
>
> > Yes - don't use RAV to traverse parents when AST-matching.
>
>
> OK, this is certainly a much more invasive change, and isn't going to make 
> the release.
>  I understand that you don't think this crash is worth fixing for 8.0, while 
> others disagree.


Actually, I was trying to explain what can come in the future in response to a 
question, but I replied with a stronger-looking opinion than I intended.

I don't mind a non-invasive change like this going in now/soon, but I oppose 
more-drastic changes right now.

> However, such nodes are very easy to reach, inside or outside of a matcher.
>  e.g. `callExpr(callee(functionDecl()))` will match a lambda call, and "func" 
> will be the `operator()`.
>  Having such nodes then not work with parent/ancestor matchers seems 
> surprising and not **obviously** the best design choice (vs e.g. visiting all 
> nodes including implicit, or breaking symmetry between downward and upward 
> traversal).

I believe I have a solution to this, but this is not the place to discuss it. 
We can discuss it when I propose that patch :). I'll be sure to add you to the 
review.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56444



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

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 180848.
yaxunl added a comment.

Add test for `__host__ __device__`.
Removing the flag IsParsingTemplateArgument in Sema. Instead, check 
ExprEvalContexts
for disabling checking device/host consistency.
I did not use ExprEvalContext Unevaluated to condition the check because
the issue happens with ExprEvalContext ConstantEvaluated. Also we do not want to
totally remove the check, we just want to defer the check until the arg 
evaluation is done.
When the deferred check is performed, ExprEvalContext is still in 
ConstantEvaluated but
its kind is no longer EK_TemplateArgument, therefore we can use the expr kind 
to condition
the check.


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

https://reviews.llvm.org/D56411

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaCUDA.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaTemplate.cpp

Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -4784,8 +4784,8 @@
   TemplateArgument Result;
   unsigned CurSFINAEErrors = NumSFINAEErrors;
   ExprResult Res =
-CheckTemplateArgument(NTTP, NTTPType, Arg.getArgument().getAsExpr(),
-  Result, CTAK);
+  CheckTemplateArgument(NTTP, NTTPType, Arg.getArgument().getAsExpr(),
+Result, CTAK, dyn_cast(Template));
   if (Res.isInvalid())
 return true;
   // If the current template argument causes an error, give up now.
@@ -6154,6 +6154,27 @@
   return true;
 }
 
+namespace {
+bool CheckCUDATemplateArgument(Sema &S, Expr *Arg, TemplateDecl *Template) {
+  if (Template) {
+Expr *E = Arg;
+if (UnaryOperator *UO = dyn_cast(E)) {
+  E = UO ? UO->getSubExpr() : nullptr;
+}
+if (DeclRefExpr *DRE = dyn_cast_or_null(E)) {
+  ValueDecl *Entity = DRE ? DRE->getDecl() : nullptr;
+  if (Entity) {
+if (auto Callee = dyn_cast(Entity))
+  if (auto Caller =
+  dyn_cast(Template->getTemplatedDecl()))
+if (!S.CheckCUDACall(Arg->getBeginLoc(), Callee, Caller))
+  return false;
+  }
+}
+  }
+  return true;
+}
+} // namespace
 /// Check a template argument against its corresponding
 /// non-type template parameter.
 ///
@@ -6164,7 +6185,8 @@
 ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
QualType ParamType, Expr *Arg,
TemplateArgument &Converted,
-   CheckTemplateArgumentKind CTAK) {
+   CheckTemplateArgumentKind CTAK,
+   TemplateDecl *Template) {
   SourceLocation StartLoc = Arg->getBeginLoc();
 
   // If the parameter type somehow involves auto, deduce the type now.
@@ -6561,7 +6583,11 @@
   if (FunctionDecl *Fn = ResolveAddressOfOverloadedFunction(Arg, ParamType,
 true,
 FoundResult)) {
-if (DiagnoseUseOfDecl(Fn, Arg->getBeginLoc()))
+if (DiagnoseUseOfDecl(Fn, Arg->getBeginLoc(),
+  /*UnknownObjCClass=*/nullptr,
+  /*ObjCPropertyAccess=*/false,
+  /*AvoidPartialAvailabilityChecks=*/false,
+  /*ClassReciever=*/nullptr, Template))
   return ExprError();
 
 Arg = FixOverloadedFunctionReference(Arg, FoundResult, Fn);
@@ -6570,6 +6596,9 @@
 return ExprError();
 }
 
+if (!CheckCUDATemplateArgument(*this, Arg, Template))
+  return ExprError();
+
 if (!ParamType->isMemberPointerType()) {
   if (CheckTemplateArgumentAddressOfObjectOrFunction(*this, Param,
  ParamType,
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -213,7 +213,8 @@
  const ObjCInterfaceDecl *UnknownObjCClass,
  bool ObjCPropertyAccess,
  bool AvoidPartialAvailabilityChecks,
- ObjCInterfaceDecl *ClassReceiver) {
+ ObjCInterfaceDecl *ClassReceiver,
+ TemplateDecl *Template) {
   SourceLocation Loc = Locs.front();
   if (getLangOpts().CPlusPlus && isa(D)) {
 // If there were any diagnostics suppressed by template argument deduction,
@@ -270,7 +271,11 @@
 DeduceReturnType(FD, Loc))
   return true;
 
-if (getLangOpts().CUDA && !CheckCUDACall(Loc, FD))
+if (getLangOpts().CUDA &&
+!CheckCUDACall(
+Loc, FD,
+Template ? dyn_cast(Template->getTemplatedDecl())
+  

r350741 - [AST] Move back BasePathSize to the bit-fields of CastExpr

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Wed Jan  9 08:41:33 2019
New Revision: 350741

URL: http://llvm.org/viewvc/llvm-project?rev=350741&view=rev
Log:
[AST] Move back BasePathSize to the bit-fields of CastExpr

The number of trailing CXXBaseSpecifiers in CastExpr was moved from
CastExprBitfields to a trailing object in r338489 (D50050). At this time these
bit-fields classes were only 32 bits wide. However later r345459 widened these
bit-field classes to 64 bits.

The reason for this change was that on 64 bit archs alignment requirements
caused 4 bytes of padding after the Stmt sub-object in nearly all expression
classes. Reusing this padding yielded an >10% reduction in the size used by all
statement/expressions when parsing all of Boost (on a 64 bit arch). This
increased the size of statement/expressions for 32 bits archs, but this can be
mitigated by moving more data to the bit-fields of Stmt (and moreover most
people now care about 64 bits archs as a host).

Therefore move back the number of CXXBaseSpecifiers in CastExpr to the
bit-fields of Stmt. This in effect mostly revert r338489 while keeping the
added test.

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

Reviewed By: lebedev.ri

Reviewers: lebedev.ri, rjmccall


Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/AST/ExprCXX.h
cfe/trunk/include/clang/AST/ExprObjC.h
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/AST/ExprCXX.cpp

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=350741&r1=350740&r2=350741&view=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Wed Jan  9 08:41:33 2019
@@ -2996,28 +2996,15 @@ public:
 /// representation in the source code (ExplicitCastExpr's derived
 /// classes).
 class CastExpr : public Expr {
-public:
-  using BasePathSizeTy = unsigned int;
-  static_assert(std::numeric_limits::max() >= 16384,
-"[implimits] Direct and indirect base classes [16384].");
-
-private:
   Stmt *Op;
 
   bool CastConsistency() const;
 
-  BasePathSizeTy *BasePathSize();
-
   const CXXBaseSpecifier * const *path_buffer() const {
 return const_cast(this)->path_buffer();
   }
   CXXBaseSpecifier **path_buffer();
 
-  void setBasePathSize(BasePathSizeTy basePathSize) {
-assert(!path_empty() && basePathSize != 0);
-*(BasePathSize()) = basePathSize;
-  }
-
 protected:
   CastExpr(StmtClass SC, QualType ty, ExprValueKind VK, const CastKind kind,
Expr *op, unsigned BasePathSize)
@@ -3038,9 +3025,9 @@ protected:
 Op(op) {
 CastExprBits.Kind = kind;
 CastExprBits.PartOfExplicitCast = false;
-CastExprBits.BasePathIsEmpty = BasePathSize == 0;
-if (!path_empty())
-  setBasePathSize(BasePathSize);
+CastExprBits.BasePathSize = BasePathSize;
+assert((CastExprBits.BasePathSize == BasePathSize) &&
+   "BasePathSize overflow!");
 assert(CastConsistency());
   }
 
@@ -3048,9 +3035,9 @@ protected:
   CastExpr(StmtClass SC, EmptyShell Empty, unsigned BasePathSize)
 : Expr(SC, Empty) {
 CastExprBits.PartOfExplicitCast = false;
-CastExprBits.BasePathIsEmpty = BasePathSize == 0;
-if (!path_empty())
-  setBasePathSize(BasePathSize);
+CastExprBits.BasePathSize = BasePathSize;
+assert((CastExprBits.BasePathSize == BasePathSize) &&
+   "BasePathSize overflow!");
   }
 
 public:
@@ -3077,13 +3064,9 @@ public:
   NamedDecl *getConversionFunction() const;
 
   typedef CXXBaseSpecifier **path_iterator;
-  typedef const CXXBaseSpecifier * const *path_const_iterator;
-  bool path_empty() const { return CastExprBits.BasePathIsEmpty; }
-  unsigned path_size() const {
-if (path_empty())
-  return 0U;
-return *(const_cast(this)->BasePathSize());
-  }
+  typedef const CXXBaseSpecifier *const *path_const_iterator;
+  bool path_empty() const { return path_size() == 0; }
+  unsigned path_size() const { return CastExprBits.BasePathSize; }
   path_iterator path_begin() { return path_buffer(); }
   path_iterator path_end() { return path_buffer() + path_size(); }
   path_const_iterator path_begin() const { return path_buffer(); }
@@ -3131,13 +3114,8 @@ public:
 /// @endcode
 class ImplicitCastExpr final
 : public CastExpr,
-  private llvm::TrailingObjects {
-  size_t numTrailingObjects(OverloadToken) const {
-return path_empty() ? 0 : 1;
-  }
+  private llvm::TrailingObjects {
 
-private:
   ImplicitCastExpr(QualType ty, CastKind kind, Expr *op,
unsigned BasePathLength, ExprValueKind VK)
 : CastExpr(ImplicitCastExprClass, ty, VK, kind, op, BasePathLength) { }
@@ -3245,8 +3223,7 @@ public:
 /// (Type)expr. For example: @c (int)f.
 class CStyleCastExpr final
 : public ExplicitCastExpr,
-  private llvm::TrailingObjects {
+  private llvm::Trail

[PATCH] D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC350741: [AST] Move back BasePathSize to the bit-fields of 
CastExpr (authored by brunoricci, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56358?vs=180377&id=180854#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56358

Files:
  include/clang/AST/Expr.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/ExprObjC.h
  include/clang/AST/Stmt.h
  lib/AST/Expr.cpp
  lib/AST/ExprCXX.cpp

Index: include/clang/AST/Expr.h
===
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -2996,28 +2996,15 @@
 /// representation in the source code (ExplicitCastExpr's derived
 /// classes).
 class CastExpr : public Expr {
-public:
-  using BasePathSizeTy = unsigned int;
-  static_assert(std::numeric_limits::max() >= 16384,
-"[implimits] Direct and indirect base classes [16384].");
-
-private:
   Stmt *Op;
 
   bool CastConsistency() const;
 
-  BasePathSizeTy *BasePathSize();
-
   const CXXBaseSpecifier * const *path_buffer() const {
 return const_cast(this)->path_buffer();
   }
   CXXBaseSpecifier **path_buffer();
 
-  void setBasePathSize(BasePathSizeTy basePathSize) {
-assert(!path_empty() && basePathSize != 0);
-*(BasePathSize()) = basePathSize;
-  }
-
 protected:
   CastExpr(StmtClass SC, QualType ty, ExprValueKind VK, const CastKind kind,
Expr *op, unsigned BasePathSize)
@@ -3038,9 +3025,9 @@
 Op(op) {
 CastExprBits.Kind = kind;
 CastExprBits.PartOfExplicitCast = false;
-CastExprBits.BasePathIsEmpty = BasePathSize == 0;
-if (!path_empty())
-  setBasePathSize(BasePathSize);
+CastExprBits.BasePathSize = BasePathSize;
+assert((CastExprBits.BasePathSize == BasePathSize) &&
+   "BasePathSize overflow!");
 assert(CastConsistency());
   }
 
@@ -3048,9 +3035,9 @@
   CastExpr(StmtClass SC, EmptyShell Empty, unsigned BasePathSize)
 : Expr(SC, Empty) {
 CastExprBits.PartOfExplicitCast = false;
-CastExprBits.BasePathIsEmpty = BasePathSize == 0;
-if (!path_empty())
-  setBasePathSize(BasePathSize);
+CastExprBits.BasePathSize = BasePathSize;
+assert((CastExprBits.BasePathSize == BasePathSize) &&
+   "BasePathSize overflow!");
   }
 
 public:
@@ -3077,13 +3064,9 @@
   NamedDecl *getConversionFunction() const;
 
   typedef CXXBaseSpecifier **path_iterator;
-  typedef const CXXBaseSpecifier * const *path_const_iterator;
-  bool path_empty() const { return CastExprBits.BasePathIsEmpty; }
-  unsigned path_size() const {
-if (path_empty())
-  return 0U;
-return *(const_cast(this)->BasePathSize());
-  }
+  typedef const CXXBaseSpecifier *const *path_const_iterator;
+  bool path_empty() const { return path_size() == 0; }
+  unsigned path_size() const { return CastExprBits.BasePathSize; }
   path_iterator path_begin() { return path_buffer(); }
   path_iterator path_end() { return path_buffer() + path_size(); }
   path_const_iterator path_begin() const { return path_buffer(); }
@@ -3131,13 +3114,8 @@
 /// @endcode
 class ImplicitCastExpr final
 : public CastExpr,
-  private llvm::TrailingObjects {
-  size_t numTrailingObjects(OverloadToken) const {
-return path_empty() ? 0 : 1;
-  }
+  private llvm::TrailingObjects {
 
-private:
   ImplicitCastExpr(QualType ty, CastKind kind, Expr *op,
unsigned BasePathLength, ExprValueKind VK)
 : CastExpr(ImplicitCastExprClass, ty, VK, kind, op, BasePathLength) { }
@@ -3245,8 +3223,7 @@
 /// (Type)expr. For example: @c (int)f.
 class CStyleCastExpr final
 : public ExplicitCastExpr,
-  private llvm::TrailingObjects {
+  private llvm::TrailingObjects {
   SourceLocation LPLoc; // the location of the left paren
   SourceLocation RPLoc; // the location of the right paren
 
@@ -3260,10 +3237,6 @@
   explicit CStyleCastExpr(EmptyShell Shell, unsigned PathSize)
 : ExplicitCastExpr(CStyleCastExprClass, Shell, PathSize) { }
 
-  size_t numTrailingObjects(OverloadToken) const {
-return path_empty() ? 0 : 1;
-  }
-
 public:
   static CStyleCastExpr *Create(const ASTContext &Context, QualType T,
 ExprValueKind VK, CastKind K,
Index: include/clang/AST/ExprObjC.h
===
--- include/clang/AST/ExprObjC.h
+++ include/clang/AST/ExprObjC.h
@@ -1574,8 +1574,7 @@
 /// \endcode
 class ObjCBridgedCastExpr final
 : public ExplicitCastExpr,
-  private llvm::TrailingObjects<
-  ObjCBridgedCastExpr, CastExpr::BasePathSizeTy, CXXBaseSpecifier *> {
+  private llvm::TrailingObjects {
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
   friend class CastExpr;
@@ -1585,10 +1584,6 @@
   SourceLocation BridgeKeywordLoc;
   unsigned Kind : 2;
 
-  size_t numTrailingObjects(

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I wholeheartedly support an openat(2) based VFS, as the current one falls short 
of the expectations you have of it and is pretty broken right now.

As for your assumption #2, I think that depends on what does one want to get 
out of the VFS. I can certainly see a use for having a  thread-local 
(VFS-local) CWD. In fact I remember seeing some code in LLDB, which used some 
darwin-only interface to create a real thread-local CWD in one function. 
However, I can also see a use for a VFS which is so "real" that it shares the 
notion of the CWD with the OS. What could happen in lldb is that a user sets 
the CWD in python (using the proper python api), and then passes a relative 
path to (lib)lldb. Since lldb never advertised to have any notion of a 
"lldb-local" CWD, the user would (righfully) expect that that path is resolved 
relative to the CWD he has just set. And that's what happened until we started 
using VFS. In fact, it still happens now, because the VFS is so bad at having a 
local CWD. So, the only reason we actually discovered this was because  
VFS->getCurrentWorkingDirectory returned an empty string if a it's CWD has 
never been set. This later translated to a null pointer and a crash.

So it almost sounds to me like we should have two implementations of the VFS. A 
"real" one, which shared the CWD with the OS, and an "almost real" one, which 
has a local CWD. Another position would be to just say that lldb was wrong to 
use the VFS in the first place, as it does not promise we want (and it should 
use a different abstraction instead). That view would be supported by the fact 
that there are other interface disconnects relating to the use of VFS in lldb 
(FILE* and friends). However, that's something you and Jonas will have to 
figure out (I'm just the peanut gallery here :P).


Repository:
  rC Clang

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

https://reviews.llvm.org/D51641



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Post-holiday ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 180859.
kadircet marked 12 inline comments as done.
kadircet added a comment.

Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224

Files:
  clangd/URI.cpp
  clangd/URI.h
  clangd/index/Background.cpp
  clangd/index/Background.h
  clangd/index/SymbolCollector.cpp
  test/clangd/background-index.test
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -9,6 +9,7 @@
 
 using testing::_;
 using testing::AllOf;
+using testing::Contains;
 using testing::ElementsAre;
 using testing::Not;
 using testing::UnorderedElementsAre;
@@ -146,7 +147,7 @@
FileURI("unittest:///root/B.cc")}));
 }
 
-TEST_F(BackgroundIndexTest, ShardStorageWriteTest) {
+TEST_F(BackgroundIndexTest, ShardStorageTest) {
   MockFSProvider FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
   void common();
@@ -175,6 +176,16 @@
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+  EXPECT_EQ(Storage.size(), 2U);
+
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(
@@ -278,5 +289,73 @@
   EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("Y")));
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageLoad) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  // Change header.
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  class A_CCnew {};
+  )cpp";
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+
+  // Change source.
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardSource->Symbols,
+  Contains(AllOf(Named("f_b"), Declared(), Defined(;
+}
+
 } // namespace clangd
 } // namespace clang
Index: test/clangd/background-index.test
===
--- test/clangd/background-index.test
+++ test/clangd/background-index.test
@@ -16,6 +16,5 @@
 # RUN: ls %t/.clangd-index/foo.cpp.*.idx
 
 # Test the index is read from disk: delete code and restart clangd.
-# FIXME: This test currently fails as we don't read the index yet.
 # RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | not FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/inde

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.cpp:259
+  // if this thread sees the older version but finishes later. This should
+  // be rare in practice.
+  DigestIt.first->second = Hash;

ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > kadircet wrote:
> > > > ilya-biryukov wrote:
> > > > > kadircet wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > > "should be rare in practice"
> > > > > > > And yet, can we avoid this altogether?
> > > > > > > 
> > > > > > > Also, I believe it won't be rare. When processing multiple 
> > > > > > > different TUs, we can easily run into the same header multiple 
> > > > > > > times, possibly with the different contents.
> > > > > > > E.g. imagine the index for the whole of clang is being built and 
> > > > > > > the user is editing `Sema.h` at the same time. We'll definitely 
> > > > > > > be running into this case over and over again.
> > > > > > Well I am open to ideas, but currently there is no way of knowing 
> > > > > > whether this is the newer version for the file. Because only 
> > > > > > information we have is the digest is different than what we 
> > > > > > currently have and this doesn't yield any chronological information.
> > > > > Do we know which request is "newer" when scheduling it?
> > > > > If so, we could keep the map of the **latest** hashes of the files 
> > > > > we've seen (in addition to the `IndexedFileDigests`, which correspond 
> > > > > to the digest for which we built the index IIUC).
> > > > > 
> > > > > This would give us a simple invariant of the final state we want to 
> > > > > bring the index to: `IndexedFileDigests` should correspond to the 
> > > > > latest hashes seen so far. If not, we have to rebuild the index for 
> > > > > the corresponding files. That, in turn, gives us a way to resolve 
> > > > > conflicts: we should never replace with symbols built for the latest 
> > > > > version (hash) of the file we've seen.
> > > > > 
> > > > > Would that work?
> > > > I am not sure if it would work for non-main files. We update the Hash 
> > > > for the main files at each indexing action, so we can safely keep track 
> > > > of the latest versions. But we collect hashes for dependencies while 
> > > > performing the indexing which happens in parallel. Therefore an 
> > > > indexing action triggered earlier might get an up-to-date version of a 
> > > > dependency than an action triggered later-on.
> > > If updates for each TU are atomic (i.e. all files included in the TU are 
> > > updated in a single go) we would get the up-to-date index eventually, 
> > > assuming we rebuild the index when TU dependencies change (we don't 
> > > schedule rebuilds on changes to included header now, but we're planning 
> > > to do so at some point).
> > Sure, that assumption seems more relaxed than the previous one, just wanna 
> > make sure I got it right:
> > Your suggested solution assumes: Dependencies of a TU won't change during 
> > indexing of that TU
> > Whereas the previous assumption was: Files won't change between close 
> > indexing actions
> Exactly. The idea is that eventually we'll start tracking changes and will be 
> able to detect even those cases and rebuild the index accordingly. Just 
> trying to keep us from drifting away from that model too much.
> 
> > files won't change between close indexing actions
> This assumption worked fine for indexing actions running right away, but I 
> think the situation with loading shards is different: the shards we are 
> loading might be old and if we don't want a stale shard (which might be 
> arbitrarily old) to overwrite results of the fresh indexing action. Let me 
> know if I'm missing something here.
Nope, your point seems to be correct



Comment at: clangd/index/Background.cpp:312
+  // Skip if file is already up to date.
+  auto DigestIt = IndexedFileDigests.try_emplace(Path);
+  if (!DigestIt.second && DigestIt.first->second == Hash)

ilya-biryukov wrote:
> We still update digests and symbols non-atomically in that case. Could we do 
> both under the same lock, similar to how it's done in `loadShards`?
Moved update to the end of the loop, but now we might perform unnecessary work 
for building {symbol,ref}slabs for files that are already up-to-date. It 
shouldn't be too much of an issue, as a solution we can lock at the entrance 
and only check if the file was up-to-date, than we update at the exit. Or we 
can keep a snapshot.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

This seem to conceptually be a list of things rather than an array subscript, 
though, right?  Could we alternatively set SpacesInContainerLiterals to false 
for LK_TableGen?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D55964#1351348 , @djasper wrote:

> This seem to conceptually be a list of things rather than an array subscript, 
> though, right?  Could we alternatively set SpacesInContainerLiterals to false 
> for LK_TableGen?


That seems to work, e.g. if I manually set the option in the test, but I can't 
seem to find a way to set a per-language default. `getLLVMStyle()`, which seems 
to be the default that every style is rooted from, assumes C++ (and does not 
have the language provided). Is there a good way to workaround that?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D56066: [OpenCL] Address space for default class members

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaInit.cpp:4539
+  if (InitCategory.isPRValue() || InitCategory.isXValue())
+T1Quals.removeAddressSpace();
+

rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > rjmccall wrote:
> > > > I can understand why a pr-value wouldn't have an address space, but an 
> > > > x-value's address space is surely important.
> > > No, wait, I think this is more deeply wrong.  We're initializing a 
> > > reference to type `cv1 T1`, and if we're initializing it with a 
> > > temporary, we're dropping the address space from `cv1`?  That's 
> > > definitely wrong.
> > > 
> > > If we're starting with a pr-value, reference-initialization normally has 
> > > to start by initializing a temporary.  (The exception is it's a class 
> > > type with a conversion operator to a reference type; C++ is abysmally 
> > > complicated.  Let's ignore this for a moment.)  Temporaries are always in 
> > > the default address space (the address space of local variables) because 
> > > the language (neither OpenCL nor Embedded C) does not give us any 
> > > facility for allocating temporary memory anywhere else.  So 
> > > reference-initialization from a pr-value creates a temporary in the 
> > > default address space and then attempts to initialize the destination 
> > > reference type with that temporary.  That initialization should fail if 
> > > there's no conversion from the default address space to the destination 
> > > address space.  For example, consider initializing a `const int __global 
> > > &` from a pr-value: this should clearly not succeed, because we have no 
> > > way of putting a temporary in the global address space.  That reference 
> > > can only be initialized with a gl-value that's already in the `__global` 
> > > address space.
> > > 
> > > On the other hand, we can successfully initialize a `const int &` with a 
> > > gl-value of type `const int __global` not by direct reference 
> > > initialization but by loading from the operand and then materializing the 
> > > result into a new temporary.
> > > 
> > > I think what this means is:
> > > 
> > > - We need to be doing this checking as if pr-values were in `__private`.  
> > > This includes both (1) expressions that were originally pr-values and (2) 
> > > expressions that have been converted to pr-values due to a failure to 
> > > perform direct reference initialization.
> > > 
> > > - We need to make sure that reference compatibility correctly prevents 
> > > references from being bound to gl-values in incompatible address spaces.
> > > On the other hand, we can successfully initialize a `const int &` with a 
> > > gl-value of type `const int __global` not by direct reference 
> > > initialization but by loading from the operand and then materializing the 
> > > result into a new temporary.
> > 
> > Is this the right thing to do? It means that initializing such a reference 
> > would incur a cost under the hood that might be unnoticed/unwanted by the 
> > user. 
> Hmm.  I was going to say that it's consistent with the normal behavior of 
> C++, but looking at the actual rule, it's more complicated than that: C++ 
> will only do this if the types are not reference-related or if the gl-value 
> is a bit-field.  So this would only be allowed if the reference type was e.g. 
> `const long &`.  It's a strange rule, but it's straightforward to stay 
> consistent with it.
> 
> I think we will eventually find ourselves needing a special-case rule for 
> copy-initializing and copy-assigning trivially-copyable types, but we're not 
> there yet.
I need some help to understand what's the right way to fix this problem.

As far as I gathered from this particular example. The issues is caused by the 
following statement of **addrspace-of-this.cl** line 47:
  C c3 = c1 + c2;

If I remove this new `if` statement in **SemaInit.cpp** and disable some 
asserts about the incorrect address space cast the AST that Clang is trying to 
create is as follows:


```
| | |-DeclStmt 0x9d6e70 
| | | `-VarDecl 0x9d6b28  col:5 c3 'C' cinit
| | |   `-ExprWithCleanups 0x9d6e58  'C'
| | | `-CXXConstructExpr 0x9d6e20  'C' 'void (__generic C 
&&) __generic' elidable
| | |   `-MaterializeTemporaryExpr 0x9d6e08  '__generic C' 
xvalue
| | | `-ImplicitCastExpr 0x9d6df0  '__generic C' 

| | |   `-CXXOperatorCallExpr 0x9d6c90  'C'
| | | |-ImplicitCastExpr 0x9d6c78  'C (*)(const __generic C 
&) __generic' 
| | | | `-DeclRefExpr 0x9d6bf8  'C (const __generic C &) 
__generic' lvalue CXXMethod 0x9d4da0 'operator+' 'C (const __generic C &) 
__generic'
| | | |-ImplicitCastExpr 0x9d6be0  '__generic C' lvalue 

| | | | `-DeclRefExpr 0x9d6b88  'C' lvalue Var 0x9d6830 
'c1' 'C'
| | | `-ImplicitCastExpr 0x9d6bc8  'const __generic C' 
lvalue 
| | |   `-DeclRefExpr 0x9d6ba8  'C' lvalue Var

[PATCH] D56469: [ObjC] Allow the use of implemented unavailable methods from within the @implementation context

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:7290
+  // when it's actually defined and is referenced from within the
+  // @implementation itself.
+  if (const auto *MD = dyn_cast(OffendingDecl)) {

Maybe add a sentence: In this context, we interpret unavailable as a form of 
access control.



Comment at: lib/Sema/SemaDeclAttr.cpp:7292
+  if (const auto *MD = dyn_cast(OffendingDecl)) {
+if (const auto *Impl = dyn_cast(Ctx)) {
+  if (MD->getClassInterface() == Impl->getClassInterface() &&

This should be `C` instead of `Ctx` right? Sorry, the naming here really sucks.



Comment at: test/SemaObjC/call-unavailable-init-in-self.m:57
+  (void)[Sub new]; // expected-error {{'new' is unavailable}}
+}

Can you add a test for category implementations:
```
@interface X @end

@interface X (Foo) 
-(void)meth __attribute__((unavailable))
@end

@implementation X (Foo)
-(void)meth {}
-(void)call_it { [self meth]; }
@end
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D56469



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

[PATCH] D56504: [WebAssembly] Add unimplemented-simd128 feature, gate builtins

[Thomas Lively via Phabricator via cfe-commits]

tlively created this revision.
tlively added reviewers: aheejin, dschuff.
Herald added subscribers: cfe-commits, kristina, sunfish, jgravelle-google, 
sbc100.

Depends on D56501 . Also adds a macro define
__wasm_unimplemented_simd128__ for feature detection of unimplemented
SIMD builtins.


Repository:
  rC Clang

https://reviews.llvm.org/D56504

Files:
  include/clang/Basic/BuiltinsWebAssembly.def
  include/clang/Driver/Options.td
  lib/Basic/Targets/WebAssembly.cpp
  lib/Basic/Targets/WebAssembly.h
  test/CodeGen/builtins-wasm.c

Index: test/CodeGen/builtins-wasm.c
===
--- test/CodeGen/builtins-wasm.c
+++ test/CodeGen/builtins-wasm.c
@@ -1,9 +1,5 @@
-// RUN: %clang_cc1 -triple wasm32-unknown-unknown -fno-lax-vector-conversions \
-// RUN:   -O3 -emit-llvm -o - %s \
-// RUN:   | FileCheck %s -check-prefixes WEBASSEMBLY,WEBASSEMBLY32
-// RUN: %clang_cc1 -triple wasm64-unknown-unknown -fno-lax-vector-conversions \
-// RUN:   -O3 -emit-llvm -o - %s \
-// RUN:   | FileCheck %s -check-prefixes WEBASSEMBLY,WEBASSEMBLY64
+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +nontrapping-fptoint -target-feature +exception-handling -fno-lax-vector-conversions -O3 -emit-llvm -o - %s | FileCheck %s -check-prefixes WEBASSEMBLY,WEBASSEMBLY32
+// RUN: %clang_cc1 -triple wasm64-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +nontrapping-fptoint -target-feature +exception-handling -fno-lax-vector-conversions -O3 -emit-llvm -o - %s | FileCheck %s -check-prefixes WEBASSEMBLY,WEBASSEMBLY64
 
 // SIMD convenience types
 typedef char i8x16 __attribute((vector_size(16)));
Index: lib/Basic/Targets/WebAssembly.h
===
--- lib/Basic/Targets/WebAssembly.h
+++ lib/Basic/Targets/WebAssembly.h
@@ -28,7 +28,8 @@
   enum SIMDEnum {
 NoSIMD,
 SIMD128,
-  } SIMDLevel;
+UnimplementedSIMD128,
+  } SIMDLevel = NoSIMD;
 
   bool HasNontrappingFPToInt;
   bool HasSignExt;
@@ -59,18 +60,12 @@
 MacroBuilder &Builder) const override;
 
 private:
+  static void setSIMDLevel(llvm::StringMap &Features, SIMDEnum Level);
+
   bool
   initFeatureMap(llvm::StringMap &Features, DiagnosticsEngine &Diags,
  StringRef CPU,
- const std::vector &FeaturesVec) const override {
-if (CPU == "bleeding-edge") {
-  Features["simd128"] = true;
-  Features["nontrapping-fptoint"] = true;
-  Features["sign-ext"] = true;
-}
-return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
-  }
-
+ const std::vector &FeaturesVec) const override;
   bool hasFeature(StringRef Feature) const final;
 
   bool handleTargetFeatures(std::vector &Features,
Index: lib/Basic/Targets/WebAssembly.cpp
===
--- lib/Basic/Targets/WebAssembly.cpp
+++ lib/Basic/Targets/WebAssembly.cpp
@@ -24,6 +24,8 @@
 const Builtin::Info WebAssemblyTargetInfo::BuiltinInfo[] = {
 #define BUILTIN(ID, TYPE, ATTRS)   \
   {#ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, nullptr},
+#define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)   \
+  {#ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, FEATURE},
 #define LIBBUILTIN(ID, TYPE, ATTRS, HEADER)\
   {#ID, TYPE, ATTRS, HEADER, ALL_LANGUAGES, nullptr},
 #include "clang/Basic/BuiltinsWebAssembly.def"
@@ -35,6 +37,7 @@
 bool WebAssemblyTargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch(Feature)
   .Case("simd128", SIMDLevel >= SIMD128)
+  .Case("unimplemented-simd128", SIMDLevel >= UnimplementedSIMD128)
   .Case("nontrapping-fptoint", HasNontrappingFPToInt)
   .Case("sign-ext", HasSignExt)
   .Case("exception-handling", HasExceptionHandling)
@@ -55,8 +58,47 @@
   defineCPUMacros(Builder, "wasm", /*Tuning=*/false);
   if (SIMDLevel >= SIMD128)
 Builder.defineMacro("__wasm_simd128__");
+  if (SIMDLevel >= UnimplementedSIMD128)
+Builder.defineMacro("__wasm_unimplemented_simd128__");
 }
 
+void WebAssemblyTargetInfo::setSIMDLevel(llvm::StringMap &Features, SIMDEnum Level) {
+  switch (Level) {
+  case UnimplementedSIMD128:
+Features["unimplemented-simd128"] = true;
+LLVM_FALLTHROUGH;
+  case SIMD128:
+Features["simd128"] = true;
+Features["sign-ext"] = true;
+LLVM_FALLTHROUGH;
+  case NoSIMD:
+break;
+  }
+}
+
+bool WebAssemblyTargetInfo::initFeatureMap(llvm::StringMap &Features, DiagnosticsEngine &Diags,
+ StringRef CPU,
+ const std::vector &FeaturesVec) const {
+if (CPU == "bleeding-edge") {
+  Features["nontrapping-fptoint"] = true;
+  Features["sign-ext"] = true;
+  setSIMDLevel(Features, SIMD128);
+}
+// Other targets do not consider use

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Look at getGoogleStyle(). It has a bunch of language-specific configs at the 
bottom. You can do the same for TableGen in getLLVMStyle().


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Right, but `getGoogleStyle()` has the inferred language passed in (i.e. 
`getGoogleStyle(FormatStyle::LanguageKind Language)`. Whereas `getLLVMStyle()` 
takes no args and assumes C++.
Would it be worthwhile to refactor getLLVMStyle to take in an optional language 
arg?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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