[PATCH] D50845: [CUDA/OpenMP] Define only some host macros during device compilation

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld updated this revision to Diff 162328.
Hahnfeld added a comment.
Herald added a subscriber: krytarowski.

Add required macros for compiling C++ code.


https://reviews.llvm.org/D50845

Files:
  lib/Frontend/InitPreprocessor.cpp
  test/Preprocessor/aux-triple.c
  test/SemaCUDA/builtins.cu

Index: test/SemaCUDA/builtins.cu
===
--- test/SemaCUDA/builtins.cu
+++ test/SemaCUDA/builtins.cu
@@ -12,8 +12,8 @@
 // RUN: -aux-triple x86_64-unknown-unknown \
 // RUN: -fsyntax-only -verify %s
 
-#if !(defined(__amd64__) && defined(__PTX__))
-#error "Expected to see preprocessor macros from both sides of compilation."
+#if !defined(__x86_64__)
+#error "Expected to see preprocessor macros from the host."
 #endif
 
 void hf() {
Index: test/Preprocessor/aux-triple.c
===
--- /dev/null
+++ test/Preprocessor/aux-triple.c
@@ -0,0 +1,62 @@
+// Ensure that Clang sets some very basic target defines based on -aux-triple.
+
+// RUN: %clang_cc1 -E -dM -ffreestanding < /dev/null \
+// RUN: -triple nvptx64-none-none \
+// RUN:   | FileCheck -match-full-lines -check-prefixes NVPTX64,NONE %s
+// RUN: %clang_cc1 -x c++ -E -dM -ffreestanding < /dev/null \
+// RUN: -triple nvptx64-none-none \
+// RUN:   | FileCheck -match-full-lines -check-prefixes NVPTX64,NONE %s
+// RUN: %clang_cc1 -x cuda -E -dM -ffreestanding < /dev/null \
+// RUN: -triple nvptx64-none-none \
+// RUN:   | FileCheck -match-full-lines -check-prefixes NVPTX64,NONE %s
+
+// CUDA:
+// RUN: %clang_cc1 -x cuda -E -dM -ffreestanding < /dev/null \
+// RUN: -triple nvptx64-none-none -aux-triple powerpc64le-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines %s \
+// RUN: -check-prefixes NVPTX64,PPC64,LINUX,LINUX-CPP
+// RUN: %clang_cc1 -x cuda -E -dM -ffreestanding < /dev/null \
+// RUN: -triple nvptx64-none-none -aux-triple x86_64-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines %s \
+// RUN: -check-prefixes NVPTX64,X86_64,LINUX,LINUX-CPP
+
+// OpenMP:
+// RUN: %clang_cc1 -E -dM -ffreestanding < /dev/null \
+// RUN: -fopenmp -fopenmp-is-device -triple nvptx64-none-none \
+// RUN: -aux-triple powerpc64le-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines -check-prefixes NVPTX64,PPC64,LINUX %s
+// RUN: %clang_cc1 -E -dM -ffreestanding < /dev/null \
+// RUN: -fopenmp -fopenmp-is-device -triple nvptx64-none-none \
+// RUN: -aux-triple x86_64-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines -check-prefixes NVPTX64,X86_64,LINUX %s
+// RUN: %clang_cc1 -x c++ -E -dM -ffreestanding < /dev/null \
+// RUN: -fopenmp -fopenmp-is-device -triple nvptx64-none-none \
+// RUN: -aux-triple powerpc64le-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines %s \
+// RUN: -check-prefixes NVPTX64,PPC64,LINUX,LINUX-CPP
+// RUN: %clang_cc1 -x c++ -E -dM -ffreestanding < /dev/null \
+// RUN: -fopenmp -fopenmp-is-device -triple nvptx64-none-none \
+// RUN: -aux-triple x86_64-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines %s \
+// RUN: -check-prefixes NVPTX64,X86_64,LINUX,LINUX-CPP
+
+// NONE-NOT:#define _GNU_SOURCE
+// LINUX-CPP:#define _GNU_SOURCE 1
+
+// NVPTX64:#define _LP64 1
+
+// NONE-NOT:#define __ELF__
+// LINUX:#define __ELF__ 1
+
+// NVPTX64:#define __LP64__ 1
+// NVPTX64:#define __NVPTX__ 1
+// NVPTX64:#define __PTX__ 1
+
+// NONE-NOT:#define __linux__
+// LINUX:#define __linux__ 1
+
+// NONE-NOT:#define __powerpc64__
+// PPC64:#define __powerpc64__ 1
+
+// NONE-NOT:#define __x86_64__
+// X86_64:#define __x86_64__ 1
Index: lib/Frontend/InitPreprocessor.cpp
===
--- lib/Frontend/InitPreprocessor.cpp
+++ lib/Frontend/InitPreprocessor.cpp
@@ -1099,6 +1099,37 @@
   TI.getTargetDefines(LangOpts, Builder);
 }
 
+/// Initialize macros based on AuxTargetInfo.
+static void InitializePredefinedAuxMacros(const TargetInfo &AuxTI,
+  const LangOptions &LangOpts,
+  MacroBuilder &Builder) {
+  auto AuxTriple = AuxTI.getTriple();
+
+  // Define basic target macros needed by at least bits/wordsize.h and
+  // bits/mathinline.h
+  switch (AuxTriple.getArch()) {
+  case llvm::Triple::x86_64:
+Builder.defineMacro("__x86_64__");
+break;
+  case llvm::Triple::ppc64:
+  case llvm::Triple::ppc64le:
+Builder.defineMacro("__powerpc64__");
+break;
+  default:
+break;
+  }
+
+  // Checked in libc++ to find out object file format and threading API.
+  if (AuxTriple.getOS() == llvm::Triple::Linux) {
+Builder.defineMacro("__ELF__");
+Builder.defineMacro("__linux__");
+// Used in features.h. If this is omitted, math.h doesn't declare float
+// versions of the functions in bits/mathcalls.h.
+if (LangOpts.CPlusPlus)
+  Builder.defineMacro("_GNU_SOURCE");
+  }
+}
+
 /// Ini

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:123
+  size_t Bytes = Index.estimateMemoryUsage();
+  for (const auto &Scheme : URISchemes) {
+// std::string contains chars with sizeof(char) == 1.

ioeric wrote:
> I think the URI scheme names should be negligible.
yeah, just drop this I think.



Comment at: clang-tools-extra/clangd/index/Index.h:391
+  // FIXME(kbobyrev): Currently, this only returns the size of index itself
+  // excluding the size of actual symbol slab index refers to. It might be
+  // useful to return both.

it might be useful --> we should include both :-)


https://reviews.llvm.org/D51154



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

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109
+  float consume() override {
+if (reachedEnd())
   return DEFAULT_BOOST_SCORE;

this isn't possible, as the item being consumed is the value of peek().
You could assert !reachedEnd() or just delete this.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:227
+  float consume() override {
+if (reachedEnd())
   return DEFAULT_BOOST_SCORE;

and again here



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:341
+  float consume() override {
+if (!reachedEnd())
+  --ItemsLeft;

if condition is always true
you could replace the if with an assertion, but the child will assert, so 
better just to remove it.
replace if with assertion



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:348
+  llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+OS << "(LIMIT items left " << ItemsLeft << *Child << ")";
+return OS;

ISTM this should show the immutable state at the start of the query, rather 
than the mutated state?
Or if you really think it's useful, both.

e.g. (LIMIT 5(3) )



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:358
 
 std::vector> consume(Iterator &It, size_t Limit) {
   std::vector> Result;

I think you can remove the Limit parameter now, replacing it with an outer 
Limit iterator



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:362
It.advance(), ++Retrieved) {
 DocID Document = It.peek();
+Result.push_back(std::make_pair(Document, It.consume()));

you can inline this now



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:267
 
 TEST(DexIndexIterators, Limit) {
+  const PostingList L0 = {3, 6, 7, 20, 42, 100};

Is this testing the "limit" parameter to consume, or the limit iterator?
It shouldn't test both just because they happen to share a name :-)

(As noted above, I think you can remove the limit parameter)


https://reviews.llvm.org/D51029



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

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Kirill Bobyrev via Phabricator via cfe-commits]

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

Address few concerns.


https://reviews.llvm.org/D51154

Files:
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/clangd/index/dex/DexIndex.h
  clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp

Index: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -923,6 +923,10 @@
llvm::function_ref
Callback) const override {}
 
+  // This is incorrect, but IndexRequestCollector is not an actual index and it
+  // isn't used in production code.
+  size_t estimateMemoryUsage() const override { return 0; }
+
   const std::vector allRequests() const { return Requests; }
 
 private:
Index: clang-tools-extra/clangd/index/dex/DexIndex.h
===
--- clang-tools-extra/clangd/index/dex/DexIndex.h
+++ clang-tools-extra/clangd/index/dex/DexIndex.h
@@ -57,7 +57,10 @@
llvm::function_ref
Callback) const override;
 
+  size_t estimateMemoryUsage() const override;
+
 private:
+
   mutable std::mutex Mutex;
 
   std::shared_ptr> Symbols /*GUARDED_BY(Mutex)*/;
Index: clang-tools-extra/clangd/index/dex/DexIndex.cpp
===
--- clang-tools-extra/clangd/index/dex/DexIndex.cpp
+++ clang-tools-extra/clangd/index/dex/DexIndex.cpp
@@ -67,6 +67,9 @@
 InvertedIndex = std::move(TempInvertedIndex);
 SymbolQuality = std::move(TempSymbolQuality);
   }
+
+  vlog("Built DexIndex with estimated memory usage {0} bytes.",
+   estimateMemoryUsage());
 }
 
 std::unique_ptr DexIndex::build(SymbolSlab Slab) {
@@ -171,6 +174,20 @@
   log("findOccurrences is not implemented.");
 }
 
+size_t DexIndex::estimateMemoryUsage() const {
+  std::lock_guard Lock(Mutex);
+
+  size_t Bytes =
+  LookupTable.size() * sizeof(std::pair);
+  Bytes += SymbolQuality.size() * sizeof(std::pair);
+  Bytes += InvertedIndex.size() * sizeof(Token);
+
+  for (const auto &P : InvertedIndex) {
+Bytes += P.second.size() * sizeof(DocID);
+  }
+  return Bytes;
+}
+
 } // namespace dex
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/Merge.cpp
===
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -84,6 +84,10 @@
 log("findOccurrences is not implemented.");
   }
 
+  size_t estimateMemoryUsage() const override {
+return Dynamic->estimateMemoryUsage() + Static->estimateMemoryUsage();
+  }
+
 private:
   const SymbolIndex *Dynamic, *Static;
 };
Index: clang-tools-extra/clangd/index/MemIndex.h
===
--- clang-tools-extra/clangd/index/MemIndex.h
+++ clang-tools-extra/clangd/index/MemIndex.h
@@ -39,7 +39,10 @@
llvm::function_ref
Callback) const override;
 
+  size_t estimateMemoryUsage() const override;
+
 private:
+
   std::shared_ptr> Symbols;
   // Index is a set of symbols that are deduplicated by symbol IDs.
   // FIXME: build smarter index structure.
Index: clang-tools-extra/clangd/index/MemIndex.cpp
===
--- clang-tools-extra/clangd/index/MemIndex.cpp
+++ clang-tools-extra/clangd/index/MemIndex.cpp
@@ -26,6 +26,9 @@
 Index = std::move(TempIndex);
 Symbols = std::move(Syms); // Relase old symbols.
   }
+
+  vlog("Built MemIndex with estimated memory usage {0} bytes.",
+   estimateMemoryUsage());
 }
 
 std::unique_ptr MemIndex::build(SymbolSlab Slab) {
@@ -98,5 +101,10 @@
   &Snap->Pointers);
 }
 
+size_t MemIndex::estimateMemoryUsage() const {
+  std::lock_guard Lock(Mutex);
+  return Index.getMemorySize();
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/Index.h
===
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -385,6 +385,12 @@
   virtual void findOccurrences(
   const OccurrencesRequest &Req,
   llvm::function_ref Callback) const = 0;
+
+  /// Returns estimated size of index (in bytes).
+  // FIXME(kbobyrev): Currently, this only returns the size of index itself
+  // excluding the size of actual symbol slab index r

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162335.
ioeric added a comment.

- fix typos..


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest &Req) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threadi

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In https://reviews.llvm.org/D51154#1211376, @ioeric wrote:

> Do we plan to expose an API in `ClangdServer` to allow C++ API users to track 
> index memory usages?


I think we do, IIUC the conclusion of the offline discussion was that it might 
be useful for the clients. However, that would require `SymbolSlab` estimation, 
too (which is probably out of scope of this patch).


https://reviews.llvm.org/D51154



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

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:21
+namespace {
+
+// Skips any combination of temporary materialization, temporary binding and

I think you can elide the empty line around the matcher.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:49
+   return;
+  // Look for calls to StrCat.
+  const auto StrCat = functionDecl(hasName("::absl::StrCat"));

Not sure if this comment adds value and might even be a little misleading. 
`StrCat` itself does not look for calls, it is a helper to do so.
Maybe you can move the comment to the actual matching part with everything 
composed and explain more what is specifically looked for.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:51
+  const auto StrCat = functionDecl(hasName("::absl::StrCat"));
+  // The arguments of StrCat are implicitly converted to AlphaNum. Match that.
+  const auto AlphaNum = ignoringTemporaries(cxxConstructExpr(

Please make this comment a sentence. `Match that the arguments ...` would be ok 
i think.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:57
+
+  const auto has_another_reference_to_lhs =
+  callExpr(hasAnyArgument(expr(hasDescendant(declRefExpr(

Please rename this variable to follow the convention



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:79
+  const auto *Call = Result.Nodes.getNodeAs("Call");
+
+  // Handles the case where x = absl::StrCat(x), which does nothing.

please add an `assert(Op != nullptr && Call != nullptr && "Matcher does not 
work as expected")`, even though it seems trivial its better then nullptr deref.
And the logic might change in the future, so better be save



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:80
+
+  // Handles the case where x = absl::StrCat(x), which does nothing.
+  if (Call->getNumArgs() == 1) {

I think `  // Handles the case 'x = absl::StrCat(x)', which does nothing.` 
would be better



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:82
+  if (Call->getNumArgs() == 1) {
+diag(Op->getBeginLoc(), "call to absl::StrCat does nothing");
+return;

please use 'absl::StrCat' in the diagnostics (same below) to signify that it is 
code.


https://reviews.llvm.org/D51061



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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping @alexfh what is your take on the issue?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > ioeric wrote:
> > > > > simark wrote:
> > > > > > ioeric wrote:
> > > > > > > simark wrote:
> > > > > > > > What's the rationale for only computing the field if `UFE.File` 
> > > > > > > > is non-null?
> > > > > > > > 
> > > > > > > > Previously, if you looked up the file with `openFile == false` 
> > > > > > > > and then later `openFile == true`, the `RealPathName` field 
> > > > > > > > would not be set because of this.  That doesn't seem right.
> > > > > > > There has been no guarantee that RealFilePath is always set. I 
> > > > > > > think that's the reason why the acceasor is called 
> > > > > > > tryGetRealPathName.
> > > > > > The way I understood it was that it could be empty because 
> > > > > > computing the real path can fail.  Not just because we didn't 
> > > > > > skipped computing it.
> > > > > I agree that the API is confusing and lack of documentation (and we 
> > > > > should fix), but the previous implementation did skip the computation 
> > > > > if File is not available though. 
> > > > > I agree that the API is confusing and lack of documentation (and we 
> > > > > should fix), but the previous implementation did skip the computation 
> > > > > if File is not available though.
> > > > 
> > > > Did it have a reason not to?  What is the `RealPathName` field useful 
> > > > for, if it's unreliable?
> > > I think the biggest concern is the performance.
> > >  
> > > For example, clangd code completion latency increased dramatically with 
> > > `real_path`:
> > > With `real_path`:
> > > {F7039629}
> > > {F7041680}
> > > 
> > > Wihtou `real_path`:
> > > {F7039630}
> > But with this patch, we are not using real_path anymore, so it can't be the 
> > reason for not computing `RealPathName` when not opening the file.  Since 
> > the non-real_path method is much more lightweight, would it make a 
> > significant difference if we always computed the field?
> > 
> > In the end I don't really mind, I am just looking for a rational 
> > explanation to why it's done this way.
> Ohh, sorry that I was confused with the other thread...
> 
> Honestly, I also don't know where all these came from (it was implemented 
> many years ago...). For now, I'm just trying to fix the problem with the 
> safest change, as the old behavior, although confusing, should be pretty 
> stable.  Changing behavior further would likely cause more problems. I would 
> prefer making further change when we are actually cleaning up or redesigning 
> `RealPathName`/`tryGetRealPath`.
Ok, I gave this a try, and it seems that computing absolute paths for unopened 
files doesn't cause any problem. Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D51159



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162337.
ioeric added a comment.

- Compute absolute paths even when file is not opened.


Repository:
  rC Clang

https://reviews.llvm.org/D51159

Files:
  lib/Basic/FileManager.cpp


Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -316,10 +316,14 @@
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
-
+  llvm::SmallString<128> AbsPath(InterndFileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
+  // can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
   return &UFE;
 }
 


Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -316,10 +316,14 @@
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
-
+  llvm::SmallString<128> AbsPath(InterndFileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
+  // can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
   return &UFE;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51204: [COFF, ARM64] Add MS intrinsics: __getReg, _ReadStatusReg, _WriteStatusReg

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In MSVC, these ones seem to be declared in intrin.h, not in the arm64intr.h 
subheader, so to match closely perhaps we should declare them there as well?

The test, arm64-microsoft-intrinsics.c, doesn't actually include intrin.h nor 
arm64intr.h, so this test doesn't actually test anything of what this changes 
(this test would pass just as well before this change). Not sure if a test for 
this is mandatory or how we handle tests for lib/Headers though.


Repository:
  rC Clang

https://reviews.llvm.org/D51204



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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[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 from my side. We need this to unbreak performance in code completion


Repository:
  rC Clang

https://reviews.llvm.org/D51159



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

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Nice fix!




Comment at: lib/Format/Format.cpp:1312
   auto &Line = *AnnotatedLines[i];
   if (Line.startsWith(tok::kw_namespace) ||
+  Line.startsWith(tok::kw_inline, tok::kw_namespace) ||

these 3 startswith checks appear 4 times now, you could pull out a helper 
function `startsWithNamespace` in FormatToken.h or something like that.
Up to you.


Repository:
  rC Clang

https://reviews.llvm.org/D51036



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly NITs, but please take a look at the potential data race




Comment at: clangd/Cancellation.cpp:23
+const Task &getCurrentTask() {
+  return *Context::current().getExisting(TaskKey);
+}

Maybe add 'assert' that TaskKey is present?
To make sure we see the breakages as early as possible when asserts are enabled 
(otherwise it's undefined behavior and it can propagate to breakages in client 
code)



Comment at: clangd/Cancellation.h:27
+//   }
+//   // Start run() in a new async thread, and make sure to propagete Context.
+//   return TH;

s/propagete/propagate



Comment at: clangd/Cancellation.h:114
+
+/// Fetches current task information from Context.
+const Task &getCurrentTask();

Maybe add a comment that the current `Context` must be have the task stashed in 
it?



Comment at: clangd/ClangdLSPServer.cpp:351
+  [this](llvm::Expected List) {
+auto _ = llvm::make_scope_exit([this]() { CleanupTaskHandle(); });
+

CleanupTaskHandle() can run in a separate thread, so can potentially run before 
the `StoreTaskHandle` call.
To avoid memory leaks in that case, let's preallocate the entry **before** 
calling `codeComplete`



Comment at: clangd/ClangdLSPServer.cpp:621
+  std::lock_guard Lock(TaskHandlesMutex);
+  const auto &it = TaskHandles.find(Params.ID);
+  if (it == TaskHandles.end())

s/it/It



Comment at: clangd/Protocol.cpp:629
+ const std::string &Field) {
+  json::ObjectMapper O(Params);
+  if (!O)

Maybe switch on different kinds of `json::Value` instead of using the 
`ObjectMapper`?
The code should be simpler. Albeit the `fromJson` of `CancelParams` might get a 
little more complicated, but that looks like a small price to pay.



Comment at: clangd/Protocol.h:883
+/// converts it into a string.
+bool parseNumberOrString(const llvm::json::Value &Params, std::string &Parsed,
+ const std::string &Field);

Maybe simplify the signature to: `Optional parseNumberOrString(const 
llvm::json::Value&);`
And call as `parseNumberOrString(obj["id"])` instead of 
`parseNumberOrString(obj, "id")`?

Also, maybe call it `normalizeRequestId`? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: unittests/Format/FormatTest.cpp:7582
Style);
+  verifyFormat("export namespace Foo\n"
+   "{};",

you may want to add tests for other modules TS syntax (e.g. non-namespace 
export decls).
It seems this works well today, but without tests it could regress.


Repository:
  rC Clang

https://reviews.llvm.org/D51036



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

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 162338.
ilya-biryukov added a comment.

- Make the option hidden


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51155

Files:
  clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

Index: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -49,9 +49,33 @@
"not given, such headers will have relative paths."),
 llvm::cl::init(""));
 
+static llvm::cl::opt MergeOnTheFly(
+"merge-on-the-fly",
+llvm::cl::desc(
+"Merges symbols for each processed translation unit as soon "
+"they become available. This results in a smaller memory "
+"usage and an almost instant reduce stage. Optimal for running as a "
+"standalone tool, but cannot be used with multi-process executors like "
+"MapReduce."),
+llvm::cl::init(true), llvm::cl::Hidden);
+
+/// Responsible for aggregating symbols from each processed file and producing
+/// the final results. All methods in this class must be thread-safe,
+/// 'consumeSymbols' may be called from multiple threads.
+class SymbolsConsumer {
+public:
+  virtual ~SymbolsConsumer() = default;
+
+  /// Consume a SymbolSlab build for a file.
+  virtual void consumeSymbols(SymbolSlab Symbols) = 0;
+  /// Produce a resulting symbol slab, by combining  occurrences of the same
+  /// symbols across translation units.
+  virtual SymbolSlab mergeResults() = 0;
+};
+
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
+  SymbolIndexActionFactory(SymbolsConsumer &Consumer) : Consumer(Consumer) {}
 
   clang::FrontendAction *create() override {
 // Wraps the index action and reports collected symbols to the execution
@@ -61,10 +85,10 @@
   WrappedIndexAction(std::shared_ptr C,
  std::unique_ptr Includes,
  const index::IndexingOptions &Opts,
- tooling::ExecutionContext *Ctx)
+ SymbolsConsumer &Consumer)
   : WrapperFrontendAction(
 index::createIndexingAction(C, Opts, nullptr)),
-Ctx(Ctx), Collector(C), Includes(std::move(Includes)),
+Consumer(Consumer), Collector(C), Includes(std::move(Includes)),
 PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {}
 
   std::unique_ptr
@@ -91,14 +115,11 @@
   return;
 }
 
-auto Symbols = Collector->takeSymbols();
-for (const auto &Sym : Symbols) {
-  Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));
-}
+Consumer.consumeSymbols(Collector->takeSymbols());
   }
 
 private:
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
   std::shared_ptr Collector;
   std::unique_ptr Includes;
   std::unique_ptr PragmaHandler;
@@ -118,30 +139,72 @@
 CollectorOpts.Includes = Includes.get();
 return new WrappedIndexAction(
 std::make_shared(std::move(CollectorOpts)),
-std::move(Includes), IndexOpts, Ctx);
+std::move(Includes), IndexOpts, Consumer);
   }
 
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
 };
 
-// Combine occurrences of the same symbol across translation units.
-SymbolSlab mergeSymbols(tooling::ToolResults *Results) {
-  SymbolSlab::Builder UniqueSymbols;
-  llvm::BumpPtrAllocator Arena;
-  Symbol::Details Scratch;
-  Results->forEachResult([&](llvm::StringRef Key, llvm::StringRef Value) {
-Arena.Reset();
-llvm::yaml::Input Yin(Value, &Arena);
-auto Sym = clang::clangd::SymbolFromYAML(Yin, Arena);
-clang::clangd::SymbolID ID;
-Key >> ID;
-if (const auto *Existing = UniqueSymbols.find(ID))
-  UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch));
-else
-  UniqueSymbols.insert(Sym);
-  });
-  return std::move(UniqueSymbols).build();
-}
+/// Stashes per-file results inside ExecutionContext, merges all of them at the
+/// end. Useful for running on MapReduce infrastructure to avoid keeping symbols
+/// from multiple files in memory.
+class ToolExecutorConsumer : public SymbolsConsumer {
+public:
+  ToolExecutorConsumer(ToolExecutor &Executor) : Executor(Executor) {}
+
+  void consumeSymbols(SymbolSlab Symbols) override {
+for (const auto &Sym : Symbols)
+  Executor.getExecutionContext()->reportResult(Sym.ID.str(),
+   SymbolToYAML(Sym));
+  }
+
+  SymbolSlab mergeResults() override {
+SymbolSlab::Builder UniqueSymbols;
+llvm::BumpPtrAllocator Arena;
+Symbol::Details Scratch;
+Executor.getToolResults()->forEachResult(
+[&](llvm::StringRef Key, llvm::StringRef Value) {
+  Arena.Reset();
+  

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 162341.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Address a round of comments & simplify code.


https://reviews.llvm.org/D51029

Files:
  clang-tools-extra/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/clangd/index/dex/Iterator.cpp
  clang-tools-extra/clangd/index/dex/Iterator.h
  clang-tools-extra/unittests/clangd/DexIndexTests.cpp

Index: clang-tools-extra/unittests/clangd/DexIndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/DexIndexTests.cpp
+++ clang-tools-extra/unittests/clangd/DexIndexTests.cpp
@@ -30,8 +30,10 @@
 namespace {
 
 std::vector
-consumeIDs(Iterator &It, size_t Limit = std::numeric_limits::max()) {
-  auto IDAndScore = consume(It, Limit);
+consumeIDs(std::unique_ptr It,
+   size_t Limit = std::numeric_limits::max()) {
+  auto Root = createLimit(move(It), Limit);
+  auto IDAndScore = consume(*Root);
   std::vector IDs(IDAndScore.size());
   for (size_t I = 0; I < IDAndScore.size(); ++I)
 IDs[I] = IDAndScore[I].first;
@@ -71,7 +73,7 @@
   auto AndWithEmpty = createAnd(create(L0), create(L1));
   EXPECT_TRUE(AndWithEmpty->reachedEnd());
 
-  EXPECT_THAT(consumeIDs(*AndWithEmpty), ElementsAre());
+  EXPECT_THAT(consumeIDs(move(AndWithEmpty)), ElementsAre());
 }
 
 TEST(DexIndexIterators, AndTwoLists) {
@@ -81,7 +83,7 @@
   auto And = createAnd(create(L1), create(L0));
 
   EXPECT_FALSE(And->reachedEnd());
-  EXPECT_THAT(consumeIDs(*And), ElementsAre(0U, 7U, 10U, 320U, 9000U));
+  EXPECT_THAT(consumeIDs(move(And)), ElementsAre(0U, 7U, 10U, 320U, 9000U));
 
   And = createAnd(create(L0), create(L1));
 
@@ -122,7 +124,7 @@
   auto OrWithEmpty = createOr(create(L0), create(L1));
   EXPECT_FALSE(OrWithEmpty->reachedEnd());
 
-  EXPECT_THAT(consumeIDs(*OrWithEmpty),
+  EXPECT_THAT(consumeIDs(move(OrWithEmpty)),
   ElementsAre(0U, 5U, 7U, 10U, 42U, 320U, 9000U));
 }
 
@@ -155,7 +157,7 @@
 
   Or = createOr(create(L0), create(L1));
 
-  EXPECT_THAT(consumeIDs(*Or),
+  EXPECT_THAT(consumeIDs(move(Or)),
   ElementsAre(0U, 4U, 5U, 7U, 10U, 30U, 42U, 60U, 320U, 9000U));
 }
 
@@ -234,13 +236,13 @@
   Root->advanceTo(1);
   Root->advanceTo(0);
   EXPECT_EQ(Root->peek(), 1U);
-  auto ElementBoost = Root->consume(Root->peek());
+  auto ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 6);
   Root->advance();
   EXPECT_EQ(Root->peek(), 5U);
   Root->advanceTo(5);
   EXPECT_EQ(Root->peek(), 5U);
-  ElementBoost = Root->consume(Root->peek());
+  ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 8);
   Root->advanceTo(9000);
   EXPECT_TRUE(Root->reachedEnd());
@@ -265,64 +267,67 @@
 }
 
 TEST(DexIndexIterators, Limit) {
-  const PostingList L0 = {4, 7, 8, 20, 42, 100};
-  const PostingList L1 = {1, 3, 5, 8, 9};
-  const PostingList L2 = {1, 5, 7, 9};
-  const PostingList L3 = {0, 5};
-  const PostingList L4 = {0, 1, 5};
-  const PostingList L5;
+  const PostingList L0 = {3, 6, 7, 20, 42, 100};
+  const PostingList L1 = {1, 3, 5, 6, 7, 30, 100};
+  const PostingList L2 = {0, 3, 5, 7, 8, 100};
 
   auto DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator, 42), ElementsAre(4, 7, 8, 20, 42, 100));
+  EXPECT_THAT(consumeIDs(move(DocIterator), 42),
+  ElementsAre(3, 6, 7, 20, 42, 100));
 
   DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre(4, 7, 8, 20, 42, 100));
+  EXPECT_THAT(consumeIDs(move(DocIterator)), ElementsAre(3, 6, 7, 20, 42, 100));
 
   DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator, 3), ElementsAre(4, 7, 8));
+  EXPECT_THAT(consumeIDs(move(DocIterator), 3), ElementsAre(3, 6, 7));
 
   DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator, 0), ElementsAre());
+  EXPECT_THAT(consumeIDs(move(DocIterator), 0), ElementsAre());
+
+  auto AndIterator =
+  createAnd(createLimit(createTrue(9000), 343), createLimit(create(L0), 2),
+createLimit(create(L1), 3), createLimit(create(L2), 42));
+  EXPECT_THAT(consumeIDs(move(AndIterator)), ElementsAre(3, 7));
 }
 
 TEST(DexIndexIterators, True) {
   auto TrueIterator = createTrue(0U);
   EXPECT_TRUE(TrueIterator->reachedEnd());
-  EXPECT_THAT(consumeIDs(*TrueIterator), ElementsAre());
+  EXPECT_THAT(consumeIDs(move(TrueIterator)), ElementsAre());
 
   PostingList L0 = {1, 2, 5, 7};
   TrueIterator = createTrue(7U);
   EXPECT_THAT(TrueIterator->peek(), 0);
   auto AndIterator = createAnd(create(L0), move(TrueIterator));
   EXPECT_FALSE(AndIterator->reachedEnd());
-  EXPECT_THAT(consumeIDs(*AndIterator), ElementsAre(1, 2, 5));
+  EXPECT_THAT(consumeIDs(move(AndIterator)), ElementsAre(1, 2, 5));
 }
 
 TEST(DexIndexIterators, Boost) {
   auto BoostIterator = createBoost(createTrue(5U), 42U);
   EXPECT_FALSE(BoostIterator->reachedEnd());
-  auto ElementBoost = BoostIterator->consume(BoostIterator->peek());
+  auto ElementBoost = BoostIterator->consume();
   EXPECT_THAT(Elemen

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/SymbolCollector.h:74
+// If not null, SymbolCollector will collect symbols.
+const CollectSymbolOptions *SymOpts;
+// If not null, SymbolCollector will collect symbol occurrences.

Use `llvm::Optional`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 162342.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Change a few comments.
- Add some assertions.
- Fix a data race.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502

Files:
  clangd/CMakeLists.txt
  clangd/Cancellation.cpp
  clangd/Cancellation.h
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/CancellationTests.cpp

Index: unittests/clangd/CancellationTests.cpp
===
--- /dev/null
+++ unittests/clangd/CancellationTests.cpp
@@ -0,0 +1,74 @@
+#include "Cancellation.h"
+#include "Context.h"
+#include "Threading.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(CancellationTest, CancellationTest) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  EXPECT_FALSE(isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskTestHandleDiesContextLives) {
+  llvm::Optional ContextWithCancellation;
+  {
+TaskHandle TH = Task::createHandle();
+ContextWithCancellation.emplace(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskContextDiesHandleLives) {
+  TaskHandle TH = Task::createHandle();
+  {
+WithContext ContextWithCancellation(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  // Still should be able to cancel without any problems.
+  TH->cancel();
+}
+
+TEST(CancellationTest, CancellationToken) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  const auto &CT = getCurrentTask();
+  EXPECT_FALSE(CT.isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(CT.isCancelled());
+}
+
+TEST(CancellationTest, AsynCancellationTest) {
+  std::atomic HasCancelled(false);
+  Notification Cancelled;
+  auto TaskToBeCancelled = [&](ConstTaskHandle CT) {
+WithContext ContextGuard(setCurrentTask(std::move(CT)));
+Cancelled.wait();
+HasCancelled = isCancelled();
+  };
+  TaskHandle TH = Task::createHandle();
+  std::thread AsyncTask(TaskToBeCancelled, TH);
+  TH->cancel();
+  Cancelled.notify();
+  AsyncTask.join();
+
+  EXPECT_TRUE(HasCancelled);
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  CancellationTests.cpp
   ClangdTests.cpp
   ClangdUnitTests.cpp
   CodeCompleteTests.cpp
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,6 +55,7 @@
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
+  virtual void onCancelRequest(CancelParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -75,4 +75,5 @@
   Register("workspace/didChangeConfiguration",
&ProtocolCallbacks::onChangeConfiguration);
   Register("workspace/symbol", &ProtocolCallbacks::onWorkspaceSymbol);
+  Register("$/cancelRequest", &ProtocolCallbacks::onCancelRequest);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -867,6 +867,22 @@
 llvm::json::Value toJSON(const DocumentHighlight &DH);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
+struct CancelParams {
+  /// The request id to cancel.
+  /// This can be either a number or string, if it is a number simply print it
+  /// out and always use a string.
+  std::string ID;
+};
+llvm::json::Value toJSON(const CancelParams &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const CancelParams &);
+bool fromJSON(const llvm::json::Value &, CancelParams &);
+
+/// Parses the Field in Params into Parsed. Params[Field] can be either of type
+/// string or number. Returns true if parsing was succesful. In case of a number
+/// converts it into a string

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clangd/ClangdLSPServer.cpp:351
+  [this](llvm::Expected List) {
+auto _ = llvm::make_scope_exit([this]() { CleanupTaskHandle(); });
+

ilya-biryukov wrote:
> CleanupTaskHandle() can run in a separate thread, so can potentially run 
> before the `StoreTaskHandle` call.
> To avoid memory leaks in that case, let's preallocate the entry **before** 
> calling `codeComplete`
Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

r340598 - [FileManager] Do not call 'real_path' in getFile().

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Aug 24 01:59:54 2018
New Revision: 340598

URL: http://llvm.org/viewvc/llvm-project?rev=340598&view=rev
Log:
[FileManager] Do not call 'real_path' in getFile().

Summary:
This partially rolls back the change in D48903:
https://github.com/llvm-mirror/clang/commit/89aa7f45a1f728144935289d4ce69d8522999de0#diff-0025af005307891b5429b6a834823d5eR318

`real_path` can be very expensive on real file systems, and calling it on each
opened file can slow down the compilation. This also slows down deserialized
ASTs for which real paths need to be recalculated for each input files again.

For clangd code completion latency (using preamble):
Before
{F7039629}
After
{F7039630}

Reviewers: ilya-biryukov, simark

Reviewed By: ilya-biryukov

Subscribers: kadircet, cfe-commits

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

Modified:
cfe/trunk/lib/Basic/FileManager.cpp

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=340598&r1=340597&r2=340598&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Fri Aug 24 01:59:54 2018
@@ -316,10 +316,14 @@ const FileEntry *FileManager::getFile(St
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
-
+  llvm::SmallString<128> AbsPath(InterndFileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
+  // can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
   return &UFE;
 }
 


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

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340598: [FileManager] Do not call 'real_path' in 
getFile(). (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51159?vs=162337&id=162344#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51159

Files:
  lib/Basic/FileManager.cpp


Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -316,10 +316,14 @@
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
-
+  llvm::SmallString<128> AbsPath(InterndFileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
+  // can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
   return &UFE;
 }
 


Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -316,10 +316,14 @@
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
-
+  llvm::SmallString<128> AbsPath(InterndFileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
+  // can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
   return &UFE;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:159
+  }
+  if (SpecFuzzyReq) {
+if (auto Filter = speculateCompletionFilter(Content, Pos)) {

ioeric wrote:
> ilya-biryukov wrote:
> > NIT: maybe invert condition to reduce nesting?
> It would become something like:
> ```
> if (!SpecFuzzyReq)
>   return SpecFuzzyReq;
>  // some work
> return SpecFuzzyReq;
> ```
> 
> Having to return the same thing twice seems a bit worse IMO.
I think it's better. Besides, you could replace return `llvm::None` in the 
first statement (would be even more straightforward from my point of view).
There's a secion in [[ 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
 | LLVM Style Guide ]] on this.



Comment at: clangd/CodeComplete.cpp:1388
+  if (Len > 0)
+St++; // Shift to the first valid character.
+  return Content.substr(St, Len);

ioeric wrote:
> ilya-biryukov wrote:
> > This looks incorrect in case of the identifiers starting at offset 0. A 
> > corner case, but still.
> `Offset == 0` is handled above.
Didn't notice. LG, thanks



Comment at: clangd/CodeComplete.h:186
+
+struct SpeculativeFuzzyFind {
+  /// A cached request from past code completions.

Maybe a short comment describing on why we have this parameter?



Comment at: clangd/CodeComplete.h:196
+  /// Set by `codeComplete()`.
+  llvm::Optional NewReq;
+  /// Result from the speculative/asynchonous call. This is only valid if

`NewReq` is an implementation detail only needed in `CodeComplete.cpp`, right? 
Maybe remove from this struct and only set in .cpp file?



Comment at: clangd/CodeComplete.h:197
+  llvm::Optional NewReq;
+  /// Result from the speculative/asynchonous call. This is only valid if
+  /// `SpecReq` is set.

Maybe remove description of the value in the result?
We don't want anyone to use, so a simple comment that the future is only there 
to wait until the async calls complete should be enough.



Comment at: clangd/CodeComplete.h:217
+ std::shared_ptr PCHs,
+ SpeculativeFuzzyFind *SpecFuzzyFind, CodeCompleteOptions Opts);
 

SpecFuzzyFind is partially an out parameter, maybe put it at the end of the 
parameter list?



Comment at: clangd/tool/ClangdMain.cpp:179
+should be effective for a number of code completions.)"),
+llvm::cl::init(true));
+

ioeric wrote:
> ilya-biryukov wrote:
> > Maybe remove this option?
> > Doing a speculative request seems like the right thing to do in all cases. 
> > It never hurts completion latency. Any reasons to avoid it?
> Sure, was trying to be conservative :)
> 
> Removed the command-line option but still keeping the code completion option. 
> As async call has some overhead, I think we should only enable it when static 
> index is provided.
Enabling only when static index is there LG. OTOH, it should always take less 
than code completion in case there's no static index, so it would've probably 
been fine either way



Comment at: unittests/clangd/CodeCompleteTests.cpp:928
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;

Calling any methods on moved-from objects is a red flag.
It probably works on vectors, but maybe reassign `Requests = {}` or use a 
swap-with-empty-vector trick instead to avoid that?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D51164: [Tooling] Add a isSingleProcess() helper to ToolExecutor

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340599: [Tooling] Add a isSingleProcess() helper to 
ToolExecutor (authored by ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51164?vs=162182&id=162345#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51164

Files:
  include/clang/Tooling/AllTUsExecution.h
  include/clang/Tooling/Execution.h
  include/clang/Tooling/StandaloneExecution.h
  unittests/Tooling/ExecutionTest.cpp


Index: unittests/Tooling/ExecutionTest.cpp
===
--- unittests/Tooling/ExecutionTest.cpp
+++ unittests/Tooling/ExecutionTest.cpp
@@ -96,6 +96,8 @@
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   llvm::Error
   execute(llvm::ArrayRef,
ArgumentsAdjuster>>) override {
Index: include/clang/Tooling/StandaloneExecution.h
===
--- include/clang/Tooling/StandaloneExecution.h
+++ include/clang/Tooling/StandaloneExecution.h
@@ -52,6 +52,8 @@
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   using ToolExecutor::execute;
 
   llvm::Error
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -114,6 +114,13 @@
   /// Returns the name of a specific executor.
   virtual StringRef getExecutorName() const = 0;
 
+  /// Should return true iff executor runs all actions in a single process.
+  /// Clients can use this signal to find out if they can collect results
+  /// in-memory (e.g. to avoid serialization costs of using ToolResults).
+  /// The single-process executors can still run multiple threads, but all
+  /// executions are guaranteed to share the same memory.
+  virtual bool isSingleProcess() const = 0;
+
   /// Executes each action with a corresponding arguments adjuster.
   virtual llvm::Error
   execute(llvm::ArrayRef<
Index: include/clang/Tooling/AllTUsExecution.h
===
--- include/clang/Tooling/AllTUsExecution.h
+++ include/clang/Tooling/AllTUsExecution.h
@@ -45,6 +45,8 @@
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   using ToolExecutor::execute;
 
   llvm::Error


Index: unittests/Tooling/ExecutionTest.cpp
===
--- unittests/Tooling/ExecutionTest.cpp
+++ unittests/Tooling/ExecutionTest.cpp
@@ -96,6 +96,8 @@
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   llvm::Error
   execute(llvm::ArrayRef,
ArgumentsAdjuster>>) override {
Index: include/clang/Tooling/StandaloneExecution.h
===
--- include/clang/Tooling/StandaloneExecution.h
+++ include/clang/Tooling/StandaloneExecution.h
@@ -52,6 +52,8 @@
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   using ToolExecutor::execute;
 
   llvm::Error
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -114,6 +114,13 @@
   /// Returns the name of a specific executor.
   virtual StringRef getExecutorName() const = 0;
 
+  /// Should return true iff executor runs all actions in a single process.
+  /// Clients can use this signal to find out if they can collect results
+  /// in-memory (e.g. to avoid serialization costs of using ToolResults).
+  /// The single-process executors can still run multiple threads, but all
+  /// executions are guaranteed to share the same memory.
+  virtual bool isSingleProcess() const = 0;
+
   /// Executes each action with a corresponding arguments adjuster.
   virtual llvm::Error
   execute(llvm::ArrayRef<
Index: include/clang/Tooling/AllTUsExecution.h
===
--- include/clang/Tooling/AllTUsExecution.h
+++ include/clang/Tooling/AllTUsExecution.h
@@ -45,6 +45,8 @@
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   using ToolExecutor::execute;
 
   llvm::Error
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r340599 - [Tooling] Add a isSingleProcess() helper to ToolExecutor

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Fri Aug 24 02:03:29 2018
New Revision: 340599

URL: http://llvm.org/viewvc/llvm-project?rev=340599&view=rev
Log:
[Tooling] Add a isSingleProcess() helper to ToolExecutor

Summary:
Used in clangd's symbol builder to optimize for the common
shared-memory executor case.

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: kadircet, cfe-commits

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

Modified:
cfe/trunk/include/clang/Tooling/AllTUsExecution.h
cfe/trunk/include/clang/Tooling/Execution.h
cfe/trunk/include/clang/Tooling/StandaloneExecution.h
cfe/trunk/unittests/Tooling/ExecutionTest.cpp

Modified: cfe/trunk/include/clang/Tooling/AllTUsExecution.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/AllTUsExecution.h?rev=340599&r1=340598&r2=340599&view=diff
==
--- cfe/trunk/include/clang/Tooling/AllTUsExecution.h (original)
+++ cfe/trunk/include/clang/Tooling/AllTUsExecution.h Fri Aug 24 02:03:29 2018
@@ -45,6 +45,8 @@ public:
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   using ToolExecutor::execute;
 
   llvm::Error

Modified: cfe/trunk/include/clang/Tooling/Execution.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Execution.h?rev=340599&r1=340598&r2=340599&view=diff
==
--- cfe/trunk/include/clang/Tooling/Execution.h (original)
+++ cfe/trunk/include/clang/Tooling/Execution.h Fri Aug 24 02:03:29 2018
@@ -114,6 +114,13 @@ public:
   /// Returns the name of a specific executor.
   virtual StringRef getExecutorName() const = 0;
 
+  /// Should return true iff executor runs all actions in a single process.
+  /// Clients can use this signal to find out if they can collect results
+  /// in-memory (e.g. to avoid serialization costs of using ToolResults).
+  /// The single-process executors can still run multiple threads, but all
+  /// executions are guaranteed to share the same memory.
+  virtual bool isSingleProcess() const = 0;
+
   /// Executes each action with a corresponding arguments adjuster.
   virtual llvm::Error
   execute(llvm::ArrayRef<

Modified: cfe/trunk/include/clang/Tooling/StandaloneExecution.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/StandaloneExecution.h?rev=340599&r1=340598&r2=340599&view=diff
==
--- cfe/trunk/include/clang/Tooling/StandaloneExecution.h (original)
+++ cfe/trunk/include/clang/Tooling/StandaloneExecution.h Fri Aug 24 02:03:29 
2018
@@ -52,6 +52,8 @@ public:
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   using ToolExecutor::execute;
 
   llvm::Error

Modified: cfe/trunk/unittests/Tooling/ExecutionTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/ExecutionTest.cpp?rev=340599&r1=340598&r2=340599&view=diff
==
--- cfe/trunk/unittests/Tooling/ExecutionTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/ExecutionTest.cpp Fri Aug 24 02:03:29 2018
@@ -96,6 +96,8 @@ public:
 
   StringRef getExecutorName() const override { return ExecutorName; }
 
+  bool isSingleProcess() const override { return true; }
+
   llvm::Error
   execute(llvm::ArrayRef,
ArgumentsAdjuster>>) override {


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

[clang-tools-extra] r340600 - [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Fri Aug 24 02:03:54 2018
New Revision: 340600

URL: http://llvm.org/viewvc/llvm-project?rev=340600&view=rev
Log:
[clangd] Allow to merge symbols on-the-fly in global-symbol-builder

Summary:
The new mode avoids serializing and deserializing YAML.
This results in better performance and less memory usage. Reduce phase
is now almost instant.

The default is to use the old mode going through YAML serialization to
allow migrating MapReduce clients that require the old mode to operate
properly. After we migrate the clients, we can switch the default to
the new mode.

Reviewers: hokein, ioeric, kbobyrev, sammccall

Reviewed By: ioeric

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

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

Modified:

clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

Modified: 
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp?rev=340600&r1=340599&r2=340600&view=diff
==
--- 
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
 (original)
+++ 
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
 Fri Aug 24 02:03:54 2018
@@ -49,9 +49,33 @@ static llvm::cl::opt Assume
"not given, such headers will have relative paths."),
 llvm::cl::init(""));
 
+static llvm::cl::opt MergeOnTheFly(
+"merge-on-the-fly",
+llvm::cl::desc(
+"Merges symbols for each processed translation unit as soon "
+"they become available. This results in a smaller memory "
+"usage and an almost instant reduce stage. Optimal for running as a "
+"standalone tool, but cannot be used with multi-process executors like 
"
+"MapReduce."),
+llvm::cl::init(true), llvm::cl::Hidden);
+
+/// Responsible for aggregating symbols from each processed file and producing
+/// the final results. All methods in this class must be thread-safe,
+/// 'consumeSymbols' may be called from multiple threads.
+class SymbolsConsumer {
+public:
+  virtual ~SymbolsConsumer() = default;
+
+  /// Consume a SymbolSlab build for a file.
+  virtual void consumeSymbols(SymbolSlab Symbols) = 0;
+  /// Produce a resulting symbol slab, by combining  occurrences of the same
+  /// symbols across translation units.
+  virtual SymbolSlab mergeResults() = 0;
+};
+
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
+  SymbolIndexActionFactory(SymbolsConsumer &Consumer) : Consumer(Consumer) {}
 
   clang::FrontendAction *create() override {
 // Wraps the index action and reports collected symbols to the execution
@@ -61,10 +85,10 @@ public:
   WrappedIndexAction(std::shared_ptr C,
  std::unique_ptr Includes,
  const index::IndexingOptions &Opts,
- tooling::ExecutionContext *Ctx)
+ SymbolsConsumer &Consumer)
   : WrapperFrontendAction(
 index::createIndexingAction(C, Opts, nullptr)),
-Ctx(Ctx), Collector(C), Includes(std::move(Includes)),
+Consumer(Consumer), Collector(C), Includes(std::move(Includes)),
 PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {}
 
   std::unique_ptr
@@ -91,14 +115,11 @@ public:
   return;
 }
 
-auto Symbols = Collector->takeSymbols();
-for (const auto &Sym : Symbols) {
-  Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));
-}
+Consumer.consumeSymbols(Collector->takeSymbols());
   }
 
 private:
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
   std::shared_ptr Collector;
   std::unique_ptr Includes;
   std::unique_ptr PragmaHandler;
@@ -118,30 +139,72 @@ public:
 CollectorOpts.Includes = Includes.get();
 return new WrappedIndexAction(
 std::make_shared(std::move(CollectorOpts)),
-std::move(Includes), IndexOpts, Ctx);
+std::move(Includes), IndexOpts, Consumer);
+  }
+
+  SymbolsConsumer &Consumer;
+};
+
+/// Stashes per-file results inside ExecutionContext, merges all of them at the
+/// end. Useful for running on MapReduce infrastructure to avoid keeping 
symbols
+/// from multiple files in memory.
+class ToolExecutorConsumer : public SymbolsConsumer {
+public:
+  ToolExecutorConsumer(ToolExecutor &Executor) : Executor(Executor) {}
+
+  void consumeSymbols(SymbolSlab Symbols) override {
+for (const auto &Sym : Symbols)
+  Executor.getExecutionContext()->reportResult(Sym.ID.str(),
+   SymbolToYAML(Sym));
+  }
+
+  SymbolSlab mergeResults() overrid

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340600: [clangd] Allow to merge symbols on-the-fly in 
global-symbol-builder (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D51155

Files:
  
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

Index: clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -49,9 +49,33 @@
"not given, such headers will have relative paths."),
 llvm::cl::init(""));
 
+static llvm::cl::opt MergeOnTheFly(
+"merge-on-the-fly",
+llvm::cl::desc(
+"Merges symbols for each processed translation unit as soon "
+"they become available. This results in a smaller memory "
+"usage and an almost instant reduce stage. Optimal for running as a "
+"standalone tool, but cannot be used with multi-process executors like "
+"MapReduce."),
+llvm::cl::init(true), llvm::cl::Hidden);
+
+/// Responsible for aggregating symbols from each processed file and producing
+/// the final results. All methods in this class must be thread-safe,
+/// 'consumeSymbols' may be called from multiple threads.
+class SymbolsConsumer {
+public:
+  virtual ~SymbolsConsumer() = default;
+
+  /// Consume a SymbolSlab build for a file.
+  virtual void consumeSymbols(SymbolSlab Symbols) = 0;
+  /// Produce a resulting symbol slab, by combining  occurrences of the same
+  /// symbols across translation units.
+  virtual SymbolSlab mergeResults() = 0;
+};
+
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
+  SymbolIndexActionFactory(SymbolsConsumer &Consumer) : Consumer(Consumer) {}
 
   clang::FrontendAction *create() override {
 // Wraps the index action and reports collected symbols to the execution
@@ -61,10 +85,10 @@
   WrappedIndexAction(std::shared_ptr C,
  std::unique_ptr Includes,
  const index::IndexingOptions &Opts,
- tooling::ExecutionContext *Ctx)
+ SymbolsConsumer &Consumer)
   : WrapperFrontendAction(
 index::createIndexingAction(C, Opts, nullptr)),
-Ctx(Ctx), Collector(C), Includes(std::move(Includes)),
+Consumer(Consumer), Collector(C), Includes(std::move(Includes)),
 PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {}
 
   std::unique_ptr
@@ -91,14 +115,11 @@
   return;
 }
 
-auto Symbols = Collector->takeSymbols();
-for (const auto &Sym : Symbols) {
-  Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));
-}
+Consumer.consumeSymbols(Collector->takeSymbols());
   }
 
 private:
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
   std::shared_ptr Collector;
   std::unique_ptr Includes;
   std::unique_ptr PragmaHandler;
@@ -118,30 +139,72 @@
 CollectorOpts.Includes = Includes.get();
 return new WrappedIndexAction(
 std::make_shared(std::move(CollectorOpts)),
-std::move(Includes), IndexOpts, Ctx);
+std::move(Includes), IndexOpts, Consumer);
   }
 
-  tooling::ExecutionContext *Ctx;
+  SymbolsConsumer &Consumer;
 };
 
-// Combine occurrences of the same symbol across translation units.
-SymbolSlab mergeSymbols(tooling::ToolResults *Results) {
-  SymbolSlab::Builder UniqueSymbols;
-  llvm::BumpPtrAllocator Arena;
-  Symbol::Details Scratch;
-  Results->forEachResult([&](llvm::StringRef Key, llvm::StringRef Value) {
-Arena.Reset();
-llvm::yaml::Input Yin(Value, &Arena);
-auto Sym = clang::clangd::SymbolFromYAML(Yin, Arena);
-clang::clangd::SymbolID ID;
-Key >> ID;
-if (const auto *Existing = UniqueSymbols.find(ID))
-  UniqueSymbols.insert(mergeSymbol(*Existing, Sym, &Scratch));
-else
-  UniqueSymbols.insert(Sym);
-  });
-  return std::move(UniqueSymbols).build();
-}
+/// Stashes per-file results inside ExecutionContext, merges all of them at the
+/// end. Useful for running on MapReduce infrastructure to avoid keeping symbols
+/// from multiple files in memory.
+class ToolExecutorConsumer : public SymbolsConsumer {
+public:
+  ToolExecutorConsumer(ToolExecutor &Executor) : Executor(Executor) {}
+
+  void consumeSymbols(SymbolSlab Symbols) override {
+for (const auto &Sym : Symbols)
+  Executor.getExecutionContext()->reportResult(Sym.ID.str(),
+   SymbolToYAML(Sym));
+  }
+
+  SymbolSlab mergeResults() override {
+SymbolSlab::

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@Dmitry.Kozhevnikov, do you need us to land the patch?
Any last changes you'd like to make?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



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

[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340601: [clangd] Log memory usage of DexIndex and MemIndex 
(authored by omtcyfz, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51154?vs=162334&id=162347#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51154

Files:
  clang-tools-extra/trunk/clangd/index/FileIndex.cpp
  clang-tools-extra/trunk/clangd/index/FileIndex.h
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/MemIndex.cpp
  clang-tools-extra/trunk/clangd/index/MemIndex.h
  clang-tools-extra/trunk/clangd/index/Merge.cpp
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Index: clang-tools-extra/trunk/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/trunk/clangd/index/FileIndex.cpp
+++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp
@@ -118,5 +118,9 @@
   log("findOccurrences is not implemented.");
 }
 
+size_t FileIndex::estimateMemoryUsage() const {
+  return Index.estimateMemoryUsage();
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/index/MemIndex.cpp
===
--- clang-tools-extra/trunk/clangd/index/MemIndex.cpp
+++ clang-tools-extra/trunk/clangd/index/MemIndex.cpp
@@ -26,6 +26,9 @@
 Index = std::move(TempIndex);
 Symbols = std::move(Syms); // Relase old symbols.
   }
+
+  vlog("Built MemIndex with estimated memory usage {0} bytes.",
+   estimateMemoryUsage());
 }
 
 std::unique_ptr MemIndex::build(SymbolSlab Slab) {
@@ -98,5 +101,10 @@
   &Snap->Pointers);
 }
 
+size_t MemIndex::estimateMemoryUsage() const {
+  std::lock_guard Lock(Mutex);
+  return Index.getMemorySize();
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
@@ -67,6 +67,9 @@
 InvertedIndex = std::move(TempInvertedIndex);
 SymbolQuality = std::move(TempSymbolQuality);
   }
+
+  vlog("Built DexIndex with estimated memory usage {0} bytes.",
+   estimateMemoryUsage());
 }
 
 std::unique_ptr DexIndex::build(SymbolSlab Slab) {
@@ -171,6 +174,20 @@
   log("findOccurrences is not implemented.");
 }
 
+size_t DexIndex::estimateMemoryUsage() const {
+  std::lock_guard Lock(Mutex);
+
+  size_t Bytes =
+  LookupTable.size() * sizeof(std::pair);
+  Bytes += SymbolQuality.size() * sizeof(std::pair);
+  Bytes += InvertedIndex.size() * sizeof(Token);
+
+  for (const auto &P : InvertedIndex) {
+Bytes += P.second.size() * sizeof(DocID);
+  }
+  return Bytes;
+}
+
 } // namespace dex
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
@@ -57,7 +57,10 @@
llvm::function_ref
Callback) const override;
 
+  size_t estimateMemoryUsage() const override;
+
 private:
+
   mutable std::mutex Mutex;
 
   std::shared_ptr> Symbols /*GUARDED_BY(Mutex)*/;
Index: clang-tools-extra/trunk/clangd/index/Merge.cpp
===
--- clang-tools-extra/trunk/clangd/index/Merge.cpp
+++ clang-tools-extra/trunk/clangd/index/Merge.cpp
@@ -84,6 +84,10 @@
 log("findOccurrences is not implemented.");
   }
 
+  size_t estimateMemoryUsage() const override {
+return Dynamic->estimateMemoryUsage() + Static->estimateMemoryUsage();
+  }
+
 private:
   const SymbolIndex *Dynamic, *Static;
 };
Index: clang-tools-extra/trunk/clangd/index/Index.h
===
--- clang-tools-extra/trunk/clangd/index/Index.h
+++ clang-tools-extra/trunk/clangd/index/Index.h
@@ -385,6 +385,12 @@
   virtual void findOccurrences(
   const OccurrencesRequest &Req,
   llvm::function_ref Callback) const = 0;
+
+  /// Returns estimated size of index (in bytes).
+  // FIXME(kbobyrev): Currently, this only returns the size of index itself
+  // excluding the size of actual symbol slab index refers to. We should include
+  // both.
+  virtual size_t estimateMemoryUsage() const = 0;
 };
 
 } // namespace clangd
Index: clang-tools-extra/trunk/clangd/index/FileIndex.h
===
--- clang-tools-extra/trunk/clangd/index/FileIndex.h
+++ clang-tools-extra/trunk/clangd/index/FileIndex.h
@@ -81,6 +81,9 @@
   vo

[clang-tools-extra] r340601 - [clangd] Log memory usage of DexIndex and MemIndex

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Fri Aug 24 02:12:54 2018
New Revision: 340601

URL: http://llvm.org/viewvc/llvm-project?rev=340601&view=rev
Log:
[clangd] Log memory usage of DexIndex and MemIndex

This patch prints information about built index size estimation to
verbose logs. This is useful for optimizing memory usage of DexIndex and
comparisons with MemIndex.

Reviewed by: sammccall

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

Modified:
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/MemIndex.cpp
clang-tools-extra/trunk/clangd/index/MemIndex.h
clang-tools-extra/trunk/clangd/index/Merge.cpp
clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=340601&r1=340600&r2=340601&view=diff
==
--- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Fri Aug 24 02:12:54 2018
@@ -118,5 +118,9 @@ void FileIndex::findOccurrences(
   log("findOccurrences is not implemented.");
 }
 
+size_t FileIndex::estimateMemoryUsage() const {
+  return Index.estimateMemoryUsage();
+}
+
 } // namespace clangd
 } // namespace clang

Modified: clang-tools-extra/trunk/clangd/index/FileIndex.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.h?rev=340601&r1=340600&r2=340601&view=diff
==
--- clang-tools-extra/trunk/clangd/index/FileIndex.h (original)
+++ clang-tools-extra/trunk/clangd/index/FileIndex.h Fri Aug 24 02:12:54 2018
@@ -81,6 +81,9 @@ public:
   void findOccurrences(const OccurrencesRequest &Req,
llvm::function_ref
Callback) const override;
+
+  size_t estimateMemoryUsage() const override;
+
 private:
   FileSymbols FSymbols;
   MemIndex Index;

Modified: clang-tools-extra/trunk/clangd/index/Index.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=340601&r1=340600&r2=340601&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Index.h (original)
+++ clang-tools-extra/trunk/clangd/index/Index.h Fri Aug 24 02:12:54 2018
@@ -385,6 +385,12 @@ public:
   virtual void findOccurrences(
   const OccurrencesRequest &Req,
   llvm::function_ref Callback) const = 0;
+
+  /// Returns estimated size of index (in bytes).
+  // FIXME(kbobyrev): Currently, this only returns the size of index itself
+  // excluding the size of actual symbol slab index refers to. We should 
include
+  // both.
+  virtual size_t estimateMemoryUsage() const = 0;
 };
 
 } // namespace clangd

Modified: clang-tools-extra/trunk/clangd/index/MemIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/MemIndex.cpp?rev=340601&r1=340600&r2=340601&view=diff
==
--- clang-tools-extra/trunk/clangd/index/MemIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/MemIndex.cpp Fri Aug 24 02:12:54 2018
@@ -26,6 +26,9 @@ void MemIndex::build(std::shared_ptr MemIndex::build(SymbolSlab Slab) {
@@ -98,5 +101,10 @@ getSymbolsFromSlab(SymbolSlab Slab) {
   &Snap->Pointers);
 }
 
+size_t MemIndex::estimateMemoryUsage() const {
+  std::lock_guard Lock(Mutex);
+  return Index.getMemorySize();
+}
+
 } // namespace clangd
 } // namespace clang

Modified: clang-tools-extra/trunk/clangd/index/MemIndex.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/MemIndex.h?rev=340601&r1=340600&r2=340601&view=diff
==
--- clang-tools-extra/trunk/clangd/index/MemIndex.h (original)
+++ clang-tools-extra/trunk/clangd/index/MemIndex.h Fri Aug 24 02:12:54 2018
@@ -39,7 +39,10 @@ public:
llvm::function_ref
Callback) const override;
 
+  size_t estimateMemoryUsage() const override;
+
 private:
+
   std::shared_ptr> Symbols;
   // Index is a set of symbols that are deduplicated by symbol IDs.
   // FIXME: build smarter index structure.

Modified: clang-tools-extra/trunk/clangd/index/Merge.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Merge.cpp?rev=340601&r1=340600&r2=340601&view=diff
==
--- clang-tools-extra/trunk/clangd/in

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162348.
ioeric marked 5 inline comments as done.
ioeric added a comment.

- Merge remote-tracking branch 'origin/master' into speculate-index
- address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest &Req) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
=

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.h:187
+/// A speculative and asynchronous fuzzy find index request (based on cached
+/// request) that can be before parsing sema. This would reduce completion
+/// latency if the speculation succeeds.

s/can be before/can be send
(or similar?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.h:187
+/// A speculative and asynchronous fuzzy find index request (based on cached
+/// request) that can be before parsing sema. This would reduce completion
+/// latency if the speculation succeeds.

ilya-biryukov wrote:
> s/can be before/can be send
> (or similar?)
or rather 'can be sent'


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D51209: AMDGPU: Default to hidden visibility

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: t-tye, kzhuravl, yaxunl.
Herald added subscribers: sunfish, aheejin, tpr, dstuttard, nhaehnle, wdng, 
jvesely, dschuff.

Object linking isn't supported, so it's not useful
to emit default visibility. Default visibility requires
relocations we don't yet support for functions compiled
in another translation unit.

  

WebAssembly already does this, although they insert these
arguments in a different place for some reason.


https://reviews.llvm.org/D51209

Files:
  lib/Driver/ToolChains/AMDGPU.cpp
  lib/Driver/ToolChains/AMDGPU.h
  test/Driver/amdgpu-visibility.cl


Index: test/Driver/amdgpu-visibility.cl
===
--- /dev/null
+++ test/Driver/amdgpu-visibility.cl
@@ -0,0 +1,7 @@
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm %s 2>&1 | 
FileCheck -check-prefix=DEFAULT %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm 
-fvisibility=protected  %s 2>&1 | FileCheck -check-prefix=OVERRIDE-PROTECTED  %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm 
-fvisibility-ms-compat  %s 2>&1 | FileCheck -check-prefix=OVERRIDE-MS  %s
+
+// DEFAULT: "-fvisibility" "hidden"
+// OVERRIDE-PROTECTED: "-fvisibility" "protected"
+// OVERRIDE-MS:  "-fvisibility" "hidden" "-ftype-visibility" "default"
Index: lib/Driver/ToolChains/AMDGPU.h
===
--- lib/Driver/ToolChains/AMDGPU.h
+++ lib/Driver/ToolChains/AMDGPU.h
@@ -61,6 +61,10 @@
   llvm::opt::DerivedArgList *
   TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch,
 Action::OffloadKind DeviceOffloadKind) const override;
+
+  void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args,
+ Action::OffloadKind DeviceOffloadKind) const 
override;
 };
 
 } // end namespace toolchains
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -98,3 +98,16 @@
 
   return DAL;
 }
+
+void AMDGPUToolChain::addClangTargetOptions(
+const llvm::opt::ArgList &DriverArgs,
+llvm::opt::ArgStringList &CC1Args,
+Action::OffloadKind DeviceOffloadingKind) const {
+  // Default to "hidden" visibility, as object level linking will not be
+  // supported for the forseeable future.
+  if (!DriverArgs.hasArg(options::OPT_fvisibility_EQ,
+ options::OPT_fvisibility_ms_compat)) {
+CC1Args.push_back("-fvisibility");
+CC1Args.push_back("hidden");
+  }
+}


Index: test/Driver/amdgpu-visibility.cl
===
--- /dev/null
+++ test/Driver/amdgpu-visibility.cl
@@ -0,0 +1,7 @@
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm %s 2>&1 | FileCheck -check-prefix=DEFAULT %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility=protected  %s 2>&1 | FileCheck -check-prefix=OVERRIDE-PROTECTED  %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility-ms-compat  %s 2>&1 | FileCheck -check-prefix=OVERRIDE-MS  %s
+
+// DEFAULT: "-fvisibility" "hidden"
+// OVERRIDE-PROTECTED: "-fvisibility" "protected"
+// OVERRIDE-MS:  "-fvisibility" "hidden" "-ftype-visibility" "default"
Index: lib/Driver/ToolChains/AMDGPU.h
===
--- lib/Driver/ToolChains/AMDGPU.h
+++ lib/Driver/ToolChains/AMDGPU.h
@@ -61,6 +61,10 @@
   llvm::opt::DerivedArgList *
   TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch,
 Action::OffloadKind DeviceOffloadKind) const override;
+
+  void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args,
+ Action::OffloadKind DeviceOffloadKind) const override;
 };
 
 } // end namespace toolchains
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -98,3 +98,16 @@
 
   return DAL;
 }
+
+void AMDGPUToolChain::addClangTargetOptions(
+const llvm::opt::ArgList &DriverArgs,
+llvm::opt::ArgStringList &CC1Args,
+Action::OffloadKind DeviceOffloadingKind) const {
+  // Default to "hidden" visibility, as object level linking will not be
+  // supported for the forseeable future.
+  if (!DriverArgs.hasArg(options::OPT_fvisibility_EQ,
+ options::OPT_fvisibility_ms_compat)) {
+CC1Args.push_back("-fvisibility");
+CC1Args.push_back("hidden");
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162351.
ioeric added a comment.

- fix doc


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest &Req) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50993#1212130, @ilya-biryukov wrote:

> @Dmitry.Kozhevnikov, do you need us to land the patch?
>  Any last changes you'd like to make?


It’s blocked by the llvm review, which has some valid comments I’ll address 
today. After that, when both are accepted, I’ll need someone to commit it, yes 
:)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



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

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly just reviewed the `references()` implementation.

For the infrastructure I have some high level questions:

- why are we combining SymbolSlab and occurrences?
- what's the motivation for the separate preamble/main-file indexes
- why put more functionality into SymbolCollector?

Let's discuss offline a bit if that works?




Comment at: clangd/TUScheduler.h:54
 
+class ParsingCallbacks {
+public:

(we're replacing a std::function, why not just a struct with two 
std::functions? saves boilerplate in a bunch of places...)



Comment at: clangd/XRefs.cpp:666
+std::vector references(ParsedAST &AST, Position Pos,
+ bool IncludeDeclaration,
+ const SymbolIndex *Index) {

I'm not sure unpacking the reference options into individual bools is going to 
scale well. I'd suggest passing the whole struct instead.



Comment at: clangd/XRefs.cpp:675
+  llvm::DenseSet IDs;
+  llvm::DenseSet NonLocalIDs;
+  for (const auto *D : Symbols.Decls) {

"local" is ambiguous here (this function cares both about file-local references 
and function-local symbols).
Consider `ExternallyVisibleIDs`



Comment at: clangd/XRefs.cpp:681
+   : "Non-local\n");
+  if (!clang::index::isFunctionLocalSymbol(D))
+NonLocalIDs.insert(*ID);

(This saves us hitting the index to look up references for one symbol, not sure 
if it's worth it at all).

I wouldn't particularly trust the helpers in clang::index::. Indeed the 
implementation looks wrong here (e.g. it would treat lambda captures as global, 
I think?)

I'd prefer D->getParentFunctionOrMethod() here.





Comment at: clangd/XRefs.cpp:688
+  SymbolOccurrenceKind::Reference | SymbolOccurrenceKind::Definition;
+  if (IncludeDeclaration)
+Filter |= SymbolOccurrenceKind::Declaration;

I'm not sure this is the best interpretation of LSP `includeDeclaration`.

The LSP spec is very vague here, and we can usually assume it's written with 
typescript in mind :-) where the declaration/definition distinction doesn't 
really exist.
It appears the distinction they're trying to draw is declaration vs "only" 
reference, rather than definition vs "only" declaration. So I think if we're 
going to respect this flag, we should *exclude* definitions when the flag is 
false.

Alternatively we could punt on this and just ignore the flag for now, and add 
it in a followup.



Comment at: clangd/XRefs.cpp:694
+
+  SymbolCollector Collector({nullptr, &Opts}, {});
+  index::IndexingOptions IndexOpts;

Reusing symbolcollector here seems odd.

It forces us to go through SymbolID rather than just using the Decl*. This is 
certainly reliable for global symbols, but I've got no idea how robust USRs are 
for local symbols. It also ends up building the Symbol objects, which we then 
throw away.

How much of SymbolCollector are we really reusing, vs a custom 
IndexDataConsumer? How is this different from document-highlights? Maybe we can 
reuse the consumer.



Comment at: clangd/XRefs.cpp:699
+  IndexOpts.IndexFunctionLocals = true;
+  // Only find references for the current main file.
+  indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),

This comment is surprising as it stands. Maybe 
```// Look in the AST for references from the current file.```
(and hoist the commentto the top of the code that deals with AST, currently 
line 686)



Comment at: clangd/XRefs.cpp:713
+
+  // Query index for non-local symbols.
+  if (Index && !NonLocalIDs.empty()) {

This is also a bit confusing - the symbols are non-local (in the function-scope 
sense) and the references are non-local (in the file sense).
Consider:
```
// Consult the index for references in other files.
// We only need to consider symbols visible to other files.
```



Comment at: clangd/XRefs.cpp:722
+  if (auto LSPLoc = toLSPLocation(O.Location, "")) {
+if (!llvm::is_contained(SeenURIs, O.Location.FileURI))
+  Results.push_back(*LSPLoc);

in the usual case, SeenURIs is the current file (if there were any local 
usages), or empty (if there weren't).

This means if there are refs from the current file in the index but not locally 
(they were deleted), you'll return them when it would be better not to.

Why not just obtain the URI for the main file directly and compare that, 
instead of extracting it from the local references found?



Comment at: clangd/index/Index.h:368
   std::vector Symbols;  // Sorted by SymbolID to allow lookup.
-};
-
-// Describes the kind of a symbol occurrence.
-//
-// This is a bitfield which can be combined from different kinds.
-enum class Symb

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: lib/Format/Format.cpp:1312
   auto &Line = *AnnotatedLines[i];
   if (Line.startsWith(tok::kw_namespace) ||
+  Line.startsWith(tok::kw_inline, tok::kw_namespace) ||

sammccall wrote:
> these 3 startswith checks appear 4 times now, you could pull out a helper 
> function `startsWithNamespace` in FormatToken.h or something like that.
> Up to you.
I missed it. Good catch!


Repository:
  rC Clang

https://reviews.llvm.org/D51036



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

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Parts of the patch contain were landed as https://reviews.llvm.org/D50847 and 
https://reviews.llvm.org/D50889, happy to discuss the design decisions 
("merged" dynamic index, two callbacks), but that's probably out of scope of 
this patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/Protocol.h:883
+/// converts it into a string.
+bool parseNumberOrString(const llvm::json::Value &Params, std::string &Parsed,
+ const std::string &Field);

ilya-biryukov wrote:
> Maybe simplify the signature to: `Optional parseNumberOrString(const 
> llvm::json::Value&);`
> And call as `parseNumberOrString(obj["id"])` instead of 
> `parseNumberOrString(obj, "id")`?
> 
> Also, maybe call it `normalizeRequestId`? 
LSP has a few more places that can have paremeters of type number | string so I 
believe this one could be used with them as well. Therefore I think it is 
better to keep the name that way. The normalizeRequestID is a sepcial case and 
handled within ClangdLSPServer.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 162354.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Change helper for normalizing Number | String.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502

Files:
  clangd/CMakeLists.txt
  clangd/Cancellation.cpp
  clangd/Cancellation.h
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/CancellationTests.cpp

Index: unittests/clangd/CancellationTests.cpp
===
--- /dev/null
+++ unittests/clangd/CancellationTests.cpp
@@ -0,0 +1,74 @@
+#include "Cancellation.h"
+#include "Context.h"
+#include "Threading.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(CancellationTest, CancellationTest) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  EXPECT_FALSE(isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskTestHandleDiesContextLives) {
+  llvm::Optional ContextWithCancellation;
+  {
+TaskHandle TH = Task::createHandle();
+ContextWithCancellation.emplace(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskContextDiesHandleLives) {
+  TaskHandle TH = Task::createHandle();
+  {
+WithContext ContextWithCancellation(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  // Still should be able to cancel without any problems.
+  TH->cancel();
+}
+
+TEST(CancellationTest, CancellationToken) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  const auto &CT = getCurrentTask();
+  EXPECT_FALSE(CT.isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(CT.isCancelled());
+}
+
+TEST(CancellationTest, AsynCancellationTest) {
+  std::atomic HasCancelled(false);
+  Notification Cancelled;
+  auto TaskToBeCancelled = [&](ConstTaskHandle CT) {
+WithContext ContextGuard(setCurrentTask(std::move(CT)));
+Cancelled.wait();
+HasCancelled = isCancelled();
+  };
+  TaskHandle TH = Task::createHandle();
+  std::thread AsyncTask(TaskToBeCancelled, TH);
+  TH->cancel();
+  Cancelled.notify();
+  AsyncTask.join();
+
+  EXPECT_TRUE(HasCancelled);
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  CancellationTests.cpp
   ClangdTests.cpp
   ClangdUnitTests.cpp
   CodeCompleteTests.cpp
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,6 +55,7 @@
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
+  virtual void onCancelRequest(CancelParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -75,4 +75,5 @@
   Register("workspace/didChangeConfiguration",
&ProtocolCallbacks::onChangeConfiguration);
   Register("workspace/symbol", &ProtocolCallbacks::onWorkspaceSymbol);
+  Register("$/cancelRequest", &ProtocolCallbacks::onCancelRequest);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -867,6 +867,20 @@
 llvm::json::Value toJSON(const DocumentHighlight &DH);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
+struct CancelParams {
+  /// The request id to cancel.
+  /// This can be either a number or string, if it is a number simply print it
+  /// out and always use a string.
+  std::string ID;
+};
+llvm::json::Value toJSON(const CancelParams &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const CancelParams &);
+bool fromJSON(const llvm::json::Value &, CancelParams &);
+
+/// Param can be either of type string or number. Returns the result as a
+/// string.
+llvm::Optional parseNumberOrString(const llvm::json::Value *Param);
+
 } // namespace clangd
 } // namespace clang
 
Index

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: lib/Format/UnwrappedLineFormatter.cpp:533-535
+ !(Line.startsWith(tok::kw_namespace) ||
+   Line.startsWith(tok::kw_inline, tok::kw_namespace) ||
+   Line.startsWith(tok::kw_export, tok::kw_namespace)) &&

Maybe add a test case (or modify an existing one) for this fix, with a C/C++ 
style comment before `namespace`, `inline`, and/or `export`?


Repository:
  rC Clang

https://reviews.llvm.org/D51036



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

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:241
 
-SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
+class SymbolCollector::CollectOccurrence {
+public:

I don't see a strong reason for the separation of `CollectOccurrence` and 
`CollectSymbol`. There are some pieceis that are only used by one of them, but 
they seem cheap enough to ignore? Intuitively, it seems to me reference 
collection could just be a member function of `SymbolCollector`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50993#1212162, @Dmitry.Kozhevnikov wrote:

> In https://reviews.llvm.org/D50993#1212130, @ilya-biryukov wrote:
>
> > @Dmitry.Kozhevnikov, do you need us to land the patch?
> >  Any last changes you'd like to make?
>
>
> It’s blocked by the llvm review, which has some valid comments I’ll address 
> today. After that, when both are accepted, I’ll need someone to commit it, 
> yes :)


Ping us when the prerequisites are ready this is ready to get merged.
And make sure to request commit access for yourself after someone lands a few 
patches for you ;-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



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

r340602 - Fix build bot after r340598.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Aug 24 02:53:44 2018
New Revision: 340602

URL: http://llvm.org/viewvc/llvm-project?rev=340602&view=rev
Log:
Fix build bot after r340598.

Revert to the original behavior: only calculate real file path when
file is opened and avoid using InterndPath for real path calculation.

Modified:
cfe/trunk/lib/Basic/FileManager.cpp

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=340602&r1=340601&r2=340602&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Fri Aug 24 02:53:44 2018
@@ -316,14 +316,18 @@ const FileEntry *FileManager::getFile(St
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  llvm::SmallString<128> AbsPath(InterndFileName);
-  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
-  // can be very expensive on real file systems.
-  // FIXME: the semantic of RealPathName is unclear, and the name might be
-  // misleading. We need to clean up the interface here.
-  makeAbsolutePath(AbsPath);
-  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
-  UFE.RealPathName = AbsPath.str();
+  if (UFE.File) {
+if (auto PathName = UFE.File->getName()) {
+  llvm::SmallString<128> AbsPath(*PathName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+  // but can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}
+  }
   return &UFE;
 }
 


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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162355.
ioeric added a comment.

- moved SpecReq into CodeComplete.cpp


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest &Req) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[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.

LG, thanks. And two small NITs.




Comment at: clangd/CodeComplete.h:184
+llvm::Expected
+speculateCompletionFilter(llvm::StringRef Content, Position Pos);
+

Is it exposed only for tests?
Maybe add a comment that it's a private API that should be avoided and put it 
to the end of the file?



Comment at: clangd/CodeComplete.h:201
+  /// The result is consumed by `codeComplete()` if speculation succeeded.
+  /// NOTE that the structure can only be destroyed after the async call
+  /// finishes.

A comment does not mention the destructor will wait for the async call to 
finish.
Maybe add that, it looks like an important detail?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162356.
ioeric added a comment.

- add doc


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest &Req) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:34
+consumeIDs(std::unique_ptr It,
+   size_t Limit = std::numeric_limits::max()) {
+  auto Root = createLimit(move(It), Limit);

remove limit param here too?
Where you're *testing* limit, the tests will be clearer if you do it explicitly.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:279
   DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre(4, 7, 8, 20, 42, 100));
+  EXPECT_THAT(consumeIDs(move(DocIterator)), ElementsAre(3, 6, 7, 20, 42, 
100));
 

this doesn't seem to test the limit iterator (at least not more than it's 
tested elsewhere)


https://reviews.llvm.org/D51029



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

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D50958#1212179, @ilya-biryukov wrote:

> Parts of this patch were landed as https://reviews.llvm.org/D50847 and 
> https://reviews.llvm.org/D50889, happy to discuss the design decisions 
> ("merged" dynamic index, two callbacks), but that's probably out of scope of 
> this patch.


Ah ok, that explains some of the unrelated changes.
I do have some questions there that would be good to discuss. Meanwhile, 
@hokein can you rebase this patch against head?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958



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

[clang-tools-extra] r340604 - [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Aug 24 04:23:56 2018
New Revision: 340604

URL: http://llvm.org/viewvc/llvm-project?rev=340604&view=rev
Log:
[clangd] Speculative code completion index request before Sema is run.

Summary:
For index-based code completion, send an asynchronous speculative index
request, based on the index request for the last code completion on the same
file and the filter text typed before the cursor, before sema code completion
is invoked. This can reduce the code completion latency (by roughly latency of
sema code completion) if the speculative request is the same as the one
generated for the ongoing code completion from sema. As a sequence of code
completions often have the same scopes and proximity paths etc, this should be
effective for a number of code completions.

Trace with speculative index request:{F6997544}

Reviewers: hokein, ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: javed.absar, jfb, MaskRay, jkorous, arphaman, kadircet, cfe-commits

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

Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=340604&r1=340603&r2=340604&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Fri Aug 24 04:23:56 2018
@@ -12,6 +12,7 @@
 #include "FindSymbols.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "Trace.h"
 #include "XRefs.h"
 #include "index/Merge.h"
 #include "clang/Format/Format.h"
@@ -22,6 +23,7 @@
 #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
@@ -29,6 +31,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
@@ -181,21 +184,44 @@ void ClangdServer::codeComplete(PathRef
   // Copy PCHs to avoid accessing this->PCHs concurrently
   std::shared_ptr PCHs = this->PCHs;
   auto FS = FSProvider.getFileSystem();
-  auto Task = [PCHs, Pos, FS,
-   CodeCompleteOpts](Path File, Callback CB,
- llvm::Expected IP) {
+
+  auto Task = [PCHs, Pos, FS, CodeCompleteOpts,
+   this](Path File, Callback CB,
+ llvm::Expected IP) {
 if (!IP)
   return CB(IP.takeError());
 
 auto PreambleData = IP->Preamble;
 
+llvm::Optional SpecFuzzyFind;
+if (CodeCompleteOpts.Index && CodeCompleteOpts.SpeculativeIndexRequest) {
+  SpecFuzzyFind.emplace();
+  {
+std::lock_guard 
Lock(CachedCompletionFuzzyFindRequestMutex);
+SpecFuzzyFind->CachedReq = 
CachedCompletionFuzzyFindRequestByFile[File];
+  }
+}
+
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
 // both the old and the new version in case only one of them matches.
 CodeCompleteResult Result = clangd::codeComplete(
 File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr,
 PreambleData ? PreambleData->Includes : IncludeStructure(),
-IP->Contents, Pos, FS, PCHs, CodeCompleteOpts);
-CB(std::move(Result));
+IP->Contents, Pos, FS, PCHs, CodeCompleteOpts,
+SpecFuzzyFind ? SpecFuzzyFind.getPointer() : nullptr);
+{
+  clang::clangd::trace::Span Tracer("Completion results callback");
+  CB(std::move(Result));
+}
+if (SpecFuzzyFind && SpecFuzzyFind->NewReq.hasValue()) {
+  std::lock_guard Lock(CachedCompletionFuzzyFindRequestMutex);
+  CachedCompletionFuzzyFindRequestByFile[File] =
+  SpecFuzzyFind->NewReq.getValue();
+}
+// SpecFuzzyFind is only destroyed after speculative fuzzy find finishes.
+// We don't want `codeComplete` to wait for the async call if it doesn't 
use
+// the result (e.g. non-index completion, speculation fails), so that `CB`
+// is called as soon as results are available.
   };
 
   WorkScheduler.runWithPreamble("CodeComplete", File,

Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=340604&r1=340603&r2=340604&view=diff
==
--- clang-t

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE340604: [clangd] Speculative code completion index request 
before Sema is run. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50962?vs=162356&id=162358#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 #include "llvm/ADT/StringMap.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -167,6 +168,18 @@
   std::chrono::steady_clock::duration UpdateDebounce;
 };
 
+/// Runs \p Action asynchronously with a new std::thread. The context will be
+/// propogated.
+template 
+std::future runAsync(llvm::unique_function Action) {
+  return std::async(std::launch::async,
+[](llvm::unique_function &&Action, Context Ctx) {
+  WithContext WithCtx(std::move(Ctx));
+  return Action();
+},
+std::move(Action), Context::current().clone());
+}
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest &Req) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -12,6 +12,7 @@
 #include "FindSymbols.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "Trace.h"
 #include "XRefs.h"
 #include "index/Merge.h"
 #include "clang/Format/Format.h"
@@ -22,13 +23,15 @@
 #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
@@ -181,21 +184,44 @@
   // Copy PCHs to avoid accessing this->PCHs concurrently
   std::shared_ptr PCHs = this->PCHs;
   auto FS = FSProvider.getFileSystem();
-  auto Task = [PCHs, Pos, FS,
-   CodeCompleteOpts](Path File, Callback CB,
- llvm::Expected IP) {
+
+  auto Task = [PCHs, Pos, FS, CodeCompleteOpts,
+   this](Path File, Callback CB,
+ llvm::Expected IP) {
 if (!IP)
   return CB(IP.takeError());
 
 auto PreambleData = IP->Preamble;
 
+llvm::Optional SpecFuzzyFind;
+if (CodeCompleteOpts.Index && CodeCompleteOpts.SpeculativeIndexRequest) {
+  SpecFuzzyFind.emplace();
+  {
+std::lock_guard Lock(CachedCompletionFuzzyFindRequestMutex);
+SpecFuzzyFind->CachedReq = CachedCompletionFuzzyFindRequestByFile[File];
+  }
+}
+
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
 // both the old and the new version in case only one of them matches.
 CodeCompleteResult Result = clangd::codeComplete(
 File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr,
 PreambleData ? PreambleData->Includes : IncludeStructure(),
-IP->Contents, Pos, FS, PCHs, CodeCompleteOpts);
-CB(std::move(Result));
+IP->Contents, Pos, FS, PCHs, CodeCompleteOpts,
+SpecFuzzyFind ? S

[clang-tools-extra] r340605 - [clangd] Implement LIMIT iterator

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Fri Aug 24 04:25:43 2018
New Revision: 340605

URL: http://llvm.org/viewvc/llvm-project?rev=340605&view=rev
Log:
[clangd] Implement LIMIT iterator

This patch introduces LIMIT iterator, which is very important for
improving the quality of search query. LIMIT iterators can be applied on
top of BOOST iterators to prevent populating query request with a huge
number of low-quality symbols.

Reviewed by: sammccall

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

Modified:
clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
clang-tools-extra/trunk/clangd/index/dex/Iterator.h
clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp?rev=340605&r1=340604&r2=340605&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp Fri Aug 24 04:25:43 
2018
@@ -128,10 +128,10 @@ bool DexIndex::fuzzyFind(
 // using 100x of the requested number might not be good in practice, e.g.
 // when the requested number of items is small.
 const unsigned ItemsToRetrieve = 100 * Req.MaxCandidateCount;
+auto Root = createLimit(move(QueryIterator), ItemsToRetrieve);
 // FIXME(kbobyrev): Add boosting to the query and utilize retrieved
 // boosting scores.
-std::vector> SymbolDocIDs =
-consume(*QueryIterator, ItemsToRetrieve);
+std::vector> SymbolDocIDs = consume(*Root);
 
 // Retrieve top Req.MaxCandidateCount items.
 std::priority_queue> Top;

Modified: clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp?rev=340605&r1=340604&r2=340605&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp Fri Aug 24 04:25:43 
2018
@@ -46,7 +46,7 @@ public:
 return *Index;
   }
 
-  float consume(DocID ID) override { return DEFAULT_BOOST_SCORE; }
+  float consume() override { return DEFAULT_BOOST_SCORE; }
 
 private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
@@ -105,16 +105,12 @@ public:
 
   DocID peek() const override { return Children.front()->peek(); }
 
-  // If not exhausted and points to the given item, consume() returns the
-  // product of Children->consume(ID). Otherwise, DEFAULT_BOOST_SCORE is
-  // returned.
-  float consume(DocID ID) override {
-if (reachedEnd() || peek() != ID)
-  return DEFAULT_BOOST_SCORE;
+  float consume() override {
+assert(!reachedEnd() && "AndIterator can't consume() at the end.");
 return std::accumulate(
 begin(Children), end(Children), DEFAULT_BOOST_SCORE,
 [&](float Current, const std::unique_ptr &Child) {
-  return Current * Child->consume(ID);
+  return Current * Child->consume();
 });
   }
 
@@ -226,15 +222,16 @@ public:
 
   // Returns the maximum boosting score among all Children when iterator is not
   // exhausted and points to the given ID, DEFAULT_BOOST_SCORE otherwise.
-  float consume(DocID ID) override {
-if (reachedEnd() || peek() != ID)
-  return DEFAULT_BOOST_SCORE;
+  float consume() override {
+assert(!reachedEnd() &&
+   "OrIterator can't consume() after it reached the end.");
+const DocID ID = peek();
 return std::accumulate(
 begin(Children), end(Children), DEFAULT_BOOST_SCORE,
-[&](float Current, const std::unique_ptr &Child) {
+[&](float Boost, const std::unique_ptr &Child) {
   return (!Child->reachedEnd() && Child->peek() == ID)
- ? std::max(Current, Child->consume(ID))
- : Current;
+ ? std::max(Boost, Child->consume())
+ : Boost;
 });
   }
 
@@ -278,7 +275,7 @@ public:
 return Index;
   }
 
-  float consume(DocID) override { return DEFAULT_BOOST_SCORE; }
+  float consume() override { return DEFAULT_BOOST_SCORE; }
 
 private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
@@ -306,7 +303,7 @@ public:
 
   DocID peek() const override { return Child->peek(); }
 
-  float consume(DocID ID) override { return Child->consume(ID) * Factor; }
+  float consume() override { return Child->consume() * Factor; }
 
 private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
@@ -318,15 +315,50 @@ private:
   float Factor;
 };
 
+/// This iterator limits the number of items retrieved from the child iterator
+/// on top of the query tree. To ensure that query tree with LIMIT iterators
+/// inside works correctly, users have to 

[PATCH] D51029: [clangd] Implement LIMIT iterator

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340605: [clangd] Implement LIMIT iterator (authored by 
omtcyfz, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51029?vs=162341&id=162359#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51029

Files:
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
  clang-tools-extra/trunk/clangd/index/dex/Iterator.h
  clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
@@ -29,9 +29,8 @@
 namespace dex {
 namespace {
 
-std::vector
-consumeIDs(Iterator &It, size_t Limit = std::numeric_limits::max()) {
-  auto IDAndScore = consume(It, Limit);
+std::vector consumeIDs(Iterator &It) {
+  auto IDAndScore = consume(It);
   std::vector IDs(IDAndScore.size());
   for (size_t I = 0; I < IDAndScore.size(); ++I)
 IDs[I] = IDAndScore[I].first;
@@ -234,13 +233,13 @@
   Root->advanceTo(1);
   Root->advanceTo(0);
   EXPECT_EQ(Root->peek(), 1U);
-  auto ElementBoost = Root->consume(Root->peek());
+  auto ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 6);
   Root->advance();
   EXPECT_EQ(Root->peek(), 5U);
   Root->advanceTo(5);
   EXPECT_EQ(Root->peek(), 5U);
-  ElementBoost = Root->consume(Root->peek());
+  ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 8);
   Root->advanceTo(9000);
   EXPECT_TRUE(Root->reachedEnd());
@@ -265,24 +264,23 @@
 }
 
 TEST(DexIndexIterators, Limit) {
-  const PostingList L0 = {4, 7, 8, 20, 42, 100};
-  const PostingList L1 = {1, 3, 5, 8, 9};
-  const PostingList L2 = {1, 5, 7, 9};
-  const PostingList L3 = {0, 5};
-  const PostingList L4 = {0, 1, 5};
-  const PostingList L5;
-
-  auto DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator, 42), ElementsAre(4, 7, 8, 20, 42, 100));
-
-  DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre(4, 7, 8, 20, 42, 100));
-
-  DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator, 3), ElementsAre(4, 7, 8));
-
-  DocIterator = create(L0);
-  EXPECT_THAT(consumeIDs(*DocIterator, 0), ElementsAre());
+  const PostingList L0 = {3, 6, 7, 20, 42, 100};
+  const PostingList L1 = {1, 3, 5, 6, 7, 30, 100};
+  const PostingList L2 = {0, 3, 5, 7, 8, 100};
+
+  auto DocIterator = createLimit(create(L0), 42);
+  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre(3, 6, 7, 20, 42, 100));
+
+  DocIterator = createLimit(create(L0), 3);
+  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre(3, 6, 7));
+
+  DocIterator = createLimit(create(L0), 0);
+  EXPECT_THAT(consumeIDs(*DocIterator), ElementsAre());
+
+  auto AndIterator =
+  createAnd(createLimit(createTrue(9000), 343), createLimit(create(L0), 2),
+createLimit(create(L1), 3), createLimit(create(L2), 42));
+  EXPECT_THAT(consumeIDs(*AndIterator), ElementsAre(3, 7));
 }
 
 TEST(DexIndexIterators, True) {
@@ -301,28 +299,28 @@
 TEST(DexIndexIterators, Boost) {
   auto BoostIterator = createBoost(createTrue(5U), 42U);
   EXPECT_FALSE(BoostIterator->reachedEnd());
-  auto ElementBoost = BoostIterator->consume(BoostIterator->peek());
+  auto ElementBoost = BoostIterator->consume();
   EXPECT_THAT(ElementBoost, 42U);
 
   const PostingList L0 = {2, 4};
   const PostingList L1 = {1, 4};
   auto Root = createOr(createTrue(5U), createBoost(create(L0), 2U),
createBoost(create(L1), 3U));
 
-  ElementBoost = Root->consume(Root->peek());
+  ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, Iterator::DEFAULT_BOOST_SCORE);
   Root->advance();
   EXPECT_THAT(Root->peek(), 1U);
-  ElementBoost = Root->consume(Root->peek());
+  ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 3);
 
   Root->advance();
   EXPECT_THAT(Root->peek(), 2U);
-  ElementBoost = Root->consume(Root->peek());
+  ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 2);
 
   Root->advanceTo(4);
-  ElementBoost = Root->consume(Root->peek());
+  ElementBoost = Root->consume();
   EXPECT_THAT(ElementBoost, 3);
 }
 
Index: clang-tools-extra/trunk/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.h
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.h
@@ -87,13 +87,14 @@
   ///
   /// Note: reachedEnd() must be false.
   virtual DocID peek() const = 0;
-  /// Retrieves boosting score. Query tree root should pass Root->peek() to this
-  /// function, the parameter is needed to propagate through the tree. Given ID
-  /// should be compared against BOOST iterator peek()s: some of the iterators
-  /// would not point to the item which was propagated to the top

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: krememek, rsmith.
Herald added a subscriber: cfe-commits.

The integer promotions apply to bitfields as well, but
rather oddly. If you have a bitfield with a lower width
than int, then the type of the member expression will
be int regardless of the type of the bitfield member.
This means that you can effectively get 'type demotion'
in a bitfield member expression.

However, when analyzing format string types, we
would look through argument promotions like integer
promotion. Looking through bitfield demotion means
that we would get the wrong type when analyzing,
hiding -Wformat issues.

This patch fixes this so that we only explicitly look
through integer and floating point promotion where
the result type is actually a promotion.


Repository:
  rC Clang

https://reviews.llvm.org/D51211

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings.c


Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -692,3 +692,17 @@
   // This caused crashes due to invalid casts.
   printf(1 > 0); // expected-warning{{format string is not a string literal}} 
expected-warning{{incompatible integer to pointer conversion}} 
expected-note@format-strings.c:*{{passing argument to parameter here}} 
expected-note{{to avoid this}}
 }
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'int'}}
+  printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7582,6 +7582,26 @@
   return std::make_pair(QualType(), StringRef());
 }
 
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
+  QualType From = ICE->getSubExpr()->getType();
+  QualType To = ICE->getType();
+  // It's a floating promotion if the source type is a lower rank.
+  if (ICE->getCastKind() == CK_FloatingCast &&
+  S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  if (ICE->getCastKind() == CK_IntegralCast &&
+  From->isPromotableIntegerType() &&
+  S.Context.getPromotedIntegerType(From) == To)
+return true;
+  return false;
+}
+
 bool
 CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
 const char *StartSpecifier,
@@ -7609,11 +7629,11 @@
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
-  // and function pointer decay; seeing that an argument intended to be a
-  // string has type 'char [6]' is probably more confusing than 'char *'.
+  // and function pointer decay (seeing that an argument intended to be a
+  // string has type 'char [6]' is probably more confusing than 'char *') and
+  // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
   if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
   E = ICE->getSubExpr();
   ExprTy = E->getType();
 


Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -692,3 +692,17 @@
   // This caused crashes due to invalid casts.
   printf(1 > 0); // expected-warning{{format string is not a string literal}} expected-warning{{incompatible integer to pointer conversion}} expected-note@format-strings.c:*{{passing argument to parameter here}} expected-note{{to avoid this}}
 }
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned long' b

[PATCH] D51212: [OpenCL][Docs] Release notes for OpenCL in Clang

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: hans, bader, yaxunl.

https://reviews.llvm.org/D51212

Files:
  docs/ReleaseNotes.rst


Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -222,10 +222,31 @@
 
 ...
 
-OpenCL C Language Changes in Clang
---
+OpenCL C/C++ Language Changes in Clang
+--
 
-...
+Miscellaneous changes in OpenCL C:
+
+- Added ``cles_khr_int64`` extension.
+
+- Added bug fixes and simplifications to Clang blocks in OpenCL mode.
+
+- Added compiler flag ``-cl-uniform-work-group-size`` to allow extra compile 
time optimisation.
+
+- Propagate ``denorms-are-zero`` attribute to IR if ``-cl-denorms-are-zero`` 
is passed to the compiler.
+
+- Separated ``read_only`` and ``write_only`` pipe IR types.
+
+- Fixed address space for the ``__func__`` predefined macro.
+
+- Improved diagnostics of kernel argument types.
+
+
+Started OpenCL C++ support:
+
+- Added ``-std/-cl-std=c++``.
+
+- Added support for keywords.
 
 OpenMP Support in Clang
 --


Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -222,10 +222,31 @@
 
 ...
 
-OpenCL C Language Changes in Clang
---
+OpenCL C/C++ Language Changes in Clang
+--
 
-...
+Miscellaneous changes in OpenCL C:
+
+- Added ``cles_khr_int64`` extension.
+
+- Added bug fixes and simplifications to Clang blocks in OpenCL mode.
+
+- Added compiler flag ``-cl-uniform-work-group-size`` to allow extra compile time optimisation.
+
+- Propagate ``denorms-are-zero`` attribute to IR if ``-cl-denorms-are-zero`` is passed to the compiler.
+
+- Separated ``read_only`` and ``write_only`` pipe IR types.
+
+- Fixed address space for the ``__func__`` predefined macro.
+
+- Improved diagnostics of kernel argument types.
+
+
+Started OpenCL C++ support:
+
+- Added ``-std/-cl-std=c++``.
+
+- Added support for keywords.
 
 OpenMP Support in Clang
 --
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r340595 - [RISCV] RISC-V using -fuse-init-array by default

[Kito Cheng via cfe-commits]

Author: kito
Date: Thu Aug 23 20:05:08 2018
New Revision: 340595

URL: http://llvm.org/viewvc/llvm-project?rev=340595&view=rev
Log:
[RISCV] RISC-V using -fuse-init-array by default

Reviewers: asb, apazos, mgrang

Reviewed By: asb

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

Modified:
cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
cfe/trunk/test/Driver/riscv32-toolchain.c

Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Gnu.cpp?rev=340595&r1=340594&r2=340595&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Thu Aug 23 20:05:08 2018
@@ -2554,7 +2554,9 @@ void Generic_ELF::addClangTargetOptions(
   getTriple().getOS() == llvm::Triple::NaCl ||
   (getTriple().getVendor() == llvm::Triple::MipsTechnologies &&
!getTriple().hasEnvironment()) ||
-  getTriple().getOS() == llvm::Triple::Solaris;
+  getTriple().getOS() == llvm::Triple::Solaris ||
+  getTriple().getArch() == llvm::Triple::riscv32 ||
+  getTriple().getArch() == llvm::Triple::riscv64;
 
   if (DriverArgs.hasFlag(options::OPT_fuse_init_array,
  options::OPT_fno_use_init_array, UseInitArrayDefault))

Modified: cfe/trunk/test/Driver/riscv32-toolchain.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/riscv32-toolchain.c?rev=340595&r1=340594&r2=340595&view=diff
==
--- cfe/trunk/test/Driver/riscv32-toolchain.c (original)
+++ cfe/trunk/test/Driver/riscv32-toolchain.c Thu Aug 23 20:05:08 2018
@@ -9,6 +9,7 @@
 // RUN:   --sysroot=%S/Inputs/basic_riscv32_tree/riscv32-unknown-elf 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV32-BAREMETAL-ILP32 %s
 
+// C-RV32-BAREMETAL-ILP32: "-fuse-init-array"
 // C-RV32-BAREMETAL-ILP32: 
"{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../bin{{/|}}riscv32-unknown-elf-ld"
 // C-RV32-BAREMETAL-ILP32: 
"--sysroot={{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf"
 // C-RV32-BAREMETAL-ILP32: 
"{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib{{/|}}crt0.o"
@@ -24,6 +25,7 @@
 // RUN:   --sysroot=%S/Inputs/basic_riscv32_tree/riscv32-unknown-elf 2>&1 \
 // RUN:   | FileCheck -check-prefix=CXX-RV32-BAREMETAL-ILP32 %s
 
+// CXX-RV32-BAREMETAL-ILP32: "-fuse-init-array"
 // CXX-RV32-BAREMETAL-ILP32: "-internal-isystem" 
"{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf/include/c++{{/|}}8.0.1"
 // CXX-RV32-BAREMETAL-ILP32: 
"{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../bin{{/|}}riscv32-unknown-elf-ld"
 // CXX-RV32-BAREMETAL-ILP32: 
"--sysroot={{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf"
@@ -40,6 +42,7 @@
 // RUN:   --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV32-LINUX-MULTI-ILP32 %s
 
+// C-RV32-LINUX-MULTI-ILP32: "-fuse-init-array"
 // C-RV32-LINUX-MULTI-ILP32: 
"{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|}}ld"
 // C-RV32-LINUX-MULTI-ILP32: 
"--sysroot={{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot"
 // C-RV32-LINUX-MULTI-ILP32: "-m" "elf32lriscv"
@@ -55,6 +58,7 @@
 // RUN:   --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV32-LINUX-MULTI-ILP32D %s
 
+// C-RV32-LINUX-MULTI-ILP32D: "-fuse-init-array"
 // C-RV32-LINUX-MULTI-ILP32D: 
"{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|}}ld"
 // C-RV32-LINUX-MULTI-ILP32D: 
"--sysroot={{.*}}/Inputs/multilib_riscv_linux_sdk/sysroot"
 // C-RV32-LINUX-MULTI-ILP32D: "-m" "elf32lriscv"


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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:17
+
+  absl::strings_internal::foo();
+  class foo {

I think this line is also triggered the warning?



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:8
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:23: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil 
[abseil-no-internal-deps]
+

Does the test get passed on the first run `RUN: %check_clang_tidy %s 
abseil-no-internal-deps %t,  -- -- -I %S/Inputs` of the test? It will suppress 
clang-tidy warnings from the header, and the warning here should not appear.


https://reviews.llvm.org/D50542



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[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.

LG with a few small comments




Comment at: clangd/Protocol.cpp:635
+  if(const auto AsNumber = Params->getAsInteger())
+return utostr(AsNumber.getValue());
+

Maybe use itostr?



Comment at: clangd/Protocol.cpp:641
+bool fromJSON(const json::Value &Params, CancelParams &CP) {
+  if(const auto Parsed = parseNumberOrString(Params.getAsObject()->get("id"))) 
{
+CP.ID = *Parsed;

What is Params is not an object?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 162365.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Resolve comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502

Files:
  clangd/CMakeLists.txt
  clangd/Cancellation.cpp
  clangd/Cancellation.h
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/CancellationTests.cpp

Index: unittests/clangd/CancellationTests.cpp
===
--- /dev/null
+++ unittests/clangd/CancellationTests.cpp
@@ -0,0 +1,74 @@
+#include "Cancellation.h"
+#include "Context.h"
+#include "Threading.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(CancellationTest, CancellationTest) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  EXPECT_FALSE(isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskTestHandleDiesContextLives) {
+  llvm::Optional ContextWithCancellation;
+  {
+TaskHandle TH = Task::createHandle();
+ContextWithCancellation.emplace(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskContextDiesHandleLives) {
+  TaskHandle TH = Task::createHandle();
+  {
+WithContext ContextWithCancellation(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  // Still should be able to cancel without any problems.
+  TH->cancel();
+}
+
+TEST(CancellationTest, CancellationToken) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  const auto &CT = getCurrentTask();
+  EXPECT_FALSE(CT.isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(CT.isCancelled());
+}
+
+TEST(CancellationTest, AsynCancellationTest) {
+  std::atomic HasCancelled(false);
+  Notification Cancelled;
+  auto TaskToBeCancelled = [&](ConstTaskHandle CT) {
+WithContext ContextGuard(setCurrentTask(std::move(CT)));
+Cancelled.wait();
+HasCancelled = isCancelled();
+  };
+  TaskHandle TH = Task::createHandle();
+  std::thread AsyncTask(TaskToBeCancelled, TH);
+  TH->cancel();
+  Cancelled.notify();
+  AsyncTask.join();
+
+  EXPECT_TRUE(HasCancelled);
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  CancellationTests.cpp
   ClangdTests.cpp
   ClangdUnitTests.cpp
   CodeCompleteTests.cpp
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -55,6 +55,7 @@
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
   virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
+  virtual void onCancelRequest(CancelParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -75,4 +75,5 @@
   Register("workspace/didChangeConfiguration",
&ProtocolCallbacks::onChangeConfiguration);
   Register("workspace/symbol", &ProtocolCallbacks::onWorkspaceSymbol);
+  Register("$/cancelRequest", &ProtocolCallbacks::onCancelRequest);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -867,6 +867,20 @@
 llvm::json::Value toJSON(const DocumentHighlight &DH);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
+struct CancelParams {
+  /// The request id to cancel.
+  /// This can be either a number or string, if it is a number simply print it
+  /// out and always use a string.
+  std::string ID;
+};
+llvm::json::Value toJSON(const CancelParams &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const CancelParams &);
+bool fromJSON(const llvm::json::Value &, CancelParams &);
+
+/// Param can be either of type string or number. Returns the result as a
+/// string.
+llvm::Optional parseNumberOrString(const llvm::json::Value *Param);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Protocol.cpp
===

Re: r314872 - We allow implicit function declarations as an extension in all C dialects. Remove OpenCL special case.

[Anastasia Stulova via cfe-commits]

Thanks! Will do!



From: Richard Smith 
Sent: 23 August 2018 21:15
To: Anastasia Stulova
Cc: nd; cfe-commits
Subject: Re: r314872 - We allow implicit function declarations as an extension 
in all C dialects. Remove OpenCL special case.

On Thu, 23 Aug 2018 at 02:52, Anastasia Stulova via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:

Hi Richard,


There was a change in the spec to disallow unprototyped functions, which was 
made this year. Unfortunately it seems it didn't make into the Khronos registry 
yet to appear publicly. I will chase it up with Khronos.

Thanks!

Reiterating what I said below, I think it is reasonable and appropriate to 
disallow implicit function declarations in languages that don't have 
unprototyped functions -- implicitly declaring something that could not be 
declared explicitly doesn't seem appropriate. So feel free to revert r314872, 
but please update the comment to explain that we don't allow this as an 
extension in OpenCL because OpenCL does not permit unprototyped functions. 
Apologies for the round-trip time here.


I would like to highlight that OpenCL C was based on C99 originally and 
therefore in contrast to C doesn't have backwards compatibility with the old C 
standards. I don't think it's common to either write or port old C code to 
OpenCL simply because it's not practical to run regular C code on OpenCL 
targets efficiently but also in many cases it won't even compile due to many 
restrictions.


Anastasia



From: Richard Smith mailto:rich...@metafoo.co.uk>>
Sent: 22 August 2018 21:16
To: Anastasia Stulova
Cc: nd; cfe-commits
Subject: Re: r314872 - We allow implicit function declarations as an extension 
in all C dialects. Remove OpenCL special case.

On Wed, 22 Aug 2018 at 06:55, Anastasia Stulova via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:
Hi Richard,

> This is incorrect. Implicit function declarations declare unprototyped 
> functions, which are *not* variadic, and are in fact supported by Clang's 
> OpenCL language mode.

We have removed the support for the unprototyped functions from the OpenCL as 
well. See commit: https://reviews.llvm.org/D33681. This is the reason why in 
the OpenCL mode we now generated empty parameter list instead of unprototyped 
function like for C in the examples I provided before (that is not governed now 
by any standard or any compiler extension).

That's interesting. Do I understand from that review thread that we're 
diverging from the OpenCL specification in treating () as (void) rather than as 
an unprototyped function declaration? If so, that seems like a surprising and 
concerning decision, unless we're confident that the OpenCL language really did 
mean to change this aspect of the C semantics and omitted the wording change by 
oversight. (And I've checked, and can find nothing in the OpenCL specification 
that justifies this: it looks like a Clang bug that we reject

  int f();
  void g() { f(1, 2, 3); }
  int f(int a, int b, int c) { return 0; }

... for example, unless Khronos really did mean to use the C++ rule.)

If it is indeed the intent of the OpenCL specification to treat () as (void) 
like in C++ and not have unprototyped functions, then I think it does make 
sense to also disable our implicitly declared functions extension in OpenCL. 
But, conversely, if the OpenCL specification instead intends to follow C here 
and allow unprototyped functions, then it is a bug in our OpenCL support that 
we don't follow that intent, and when that bug is fixed it makes sense to 
continue to accept implicit function declarations as an extension.

> I would have sympathy for your position if we did not produce an extension 
> warning on this construct by default. But we do, and it says the construct is 
> invalid in OpenCL; moreover, in our strict conformance mode 
> (-pedantic-errors), we reject the code.

I understand the motivation for C to maintain the compatibility with the 
previous standards and other compilers (like gcc) to be able to support the 
legacy code. However, for OpenCL we don't have this requirement wrt older C 
standards. And therefore it is desirable to take advantage of this and remove 
problematic features that are generally confusing for developers or that can't 
be efficiently supported by the targets (especially if there is a little cost 
to that).

For this "can't be efficiently supported by the target" claim, I think you're 
conflating the target and the language mode. If some target can't reasonably 
support variadic functions, we should disable variadic functions for that 
target. That has *nothing* to do with the language mode; targets and language 
modes can be combined arbitrarily. [If I want to use Clang to compile OpenCL 
code for my PDP-11, then I should be allowed to do that (assuming I have a 
suitable LLVM backend), and that target presumably would support variadic 
functions just fine.] Likewise, if

[clang-tools-extra] r340607 - [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Fri Aug 24 06:09:41 2018
New Revision: 340607

URL: http://llvm.org/viewvc/llvm-project?rev=340607&view=rev
Log:
[clangd] Initial cancellation mechanism for LSP requests.

Reviewers: ilya-biryukov, ioeric, hokein

Reviewed By: ilya-biryukov

Subscribers: mgorny, ioeric, MaskRay, jkorous, arphaman, jfb, cfe-commits

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

Added:
clang-tools-extra/trunk/clangd/Cancellation.cpp
clang-tools-extra/trunk/clangd/Cancellation.h
clang-tools-extra/trunk/unittests/clangd/CancellationTests.cpp
Modified:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/ProtocolHandlers.h
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

Modified: clang-tools-extra/trunk/clangd/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CMakeLists.txt?rev=340607&r1=340606&r2=340607&view=diff
==
--- clang-tools-extra/trunk/clangd/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/CMakeLists.txt Fri Aug 24 06:09:41 2018
@@ -9,6 +9,7 @@ endif()
 
 add_clang_library(clangDaemon
   AST.cpp
+  Cancellation.cpp
   ClangdLSPServer.cpp
   ClangdServer.cpp
   ClangdUnit.cpp

Added: clang-tools-extra/trunk/clangd/Cancellation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Cancellation.cpp?rev=340607&view=auto
==
--- clang-tools-extra/trunk/clangd/Cancellation.cpp (added)
+++ clang-tools-extra/trunk/clangd/Cancellation.cpp Fri Aug 24 06:09:41 2018
@@ -0,0 +1,34 @@
+//===--- Cancellation.cpp 
-*-C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Cancellation.h"
+#include 
+
+namespace clang {
+namespace clangd {
+
+namespace {
+static Key TaskKey;
+} // namespace
+
+char CancelledError::ID = 0;
+
+const Task &getCurrentTask() {
+  const auto TH = Context::current().getExisting(TaskKey);
+  assert(TH && "Fetched a nullptr for TaskHandle from context.");
+  return *TH;
+}
+
+Context setCurrentTask(ConstTaskHandle TH) {
+  assert(TH && "Trying to stash a nullptr as TaskHandle into context.");
+  return Context::current().derive(TaskKey, std::move(TH));
+}
+
+} // namespace clangd
+} // namespace clang

Added: clang-tools-extra/trunk/clangd/Cancellation.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Cancellation.h?rev=340607&view=auto
==
--- clang-tools-extra/trunk/clangd/Cancellation.h (added)
+++ clang-tools-extra/trunk/clangd/Cancellation.h Fri Aug 24 06:09:41 2018
@@ -0,0 +1,142 @@
+//===--- Cancellation.h 
---*-C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Cancellation mechanism for async tasks. Roughly all the clients of this code
+// can be classified into three categories:
+// 1. The code that creates and schedules async tasks, e.g. TUScheduler.
+// 2. The callers of the async method that can cancel some of the running 
tasks,
+// e.g. `ClangdLSPServer`
+// 3. The code running inside the async task itself, i.e. code completion or
+// find definition implementation that run clang, etc.
+//
+// For (1), the guideline is to accept a callback for the result of async
+// operation and return a `TaskHandle` to allow cancelling the request.
+//
+//  TaskHandle someAsyncMethod(Runnable T,
+//  function)> Callback) {
+//   auto TH = Task::createHandle();
+//   WithContext ContextWithCancellationToken(TH);
+//   auto run = [](){
+// Callback(T());
+//   }
+//   // Start run() in a new async thread, and make sure to propagate Context.
+//   return TH;
+// }
+//
+// The callers of async methods (2) can issue cancellations and should be
+// prepared to handle `TaskCancelledError` result:
+//
+// void Caller() {
+//   // You should store this handle if you wanna cancel the task later on.
+//   Task

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340607: [clangd] Initial cancellation mechanism for LSP 
requests. (authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50502?vs=162365&id=162366#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50502

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/Cancellation.cpp
  clang-tools-extra/trunk/clangd/Cancellation.h
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/CancellationTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
===
--- clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
+++ clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  CancellationTests.cpp
   ClangdTests.cpp
   ClangdUnitTests.cpp
   CodeCompleteTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/CancellationTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CancellationTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CancellationTests.cpp
@@ -0,0 +1,74 @@
+#include "Cancellation.h"
+#include "Context.h"
+#include "Threading.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(CancellationTest, CancellationTest) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  EXPECT_FALSE(isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskTestHandleDiesContextLives) {
+  llvm::Optional ContextWithCancellation;
+  {
+TaskHandle TH = Task::createHandle();
+ContextWithCancellation.emplace(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskContextDiesHandleLives) {
+  TaskHandle TH = Task::createHandle();
+  {
+WithContext ContextWithCancellation(setCurrentTask(TH));
+EXPECT_FALSE(isCancelled());
+TH->cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  // Still should be able to cancel without any problems.
+  TH->cancel();
+}
+
+TEST(CancellationTest, CancellationToken) {
+  TaskHandle TH = Task::createHandle();
+  WithContext ContextWithCancellation(setCurrentTask(TH));
+  const auto &CT = getCurrentTask();
+  EXPECT_FALSE(CT.isCancelled());
+  TH->cancel();
+  EXPECT_TRUE(CT.isCancelled());
+}
+
+TEST(CancellationTest, AsynCancellationTest) {
+  std::atomic HasCancelled(false);
+  Notification Cancelled;
+  auto TaskToBeCancelled = [&](ConstTaskHandle CT) {
+WithContext ContextGuard(setCurrentTask(std::move(CT)));
+Cancelled.wait();
+HasCancelled = isCancelled();
+  };
+  TaskHandle TH = Task::createHandle();
+  std::thread AsyncTask(TaskToBeCancelled, TH);
+  TH->cancel();
+  Cancelled.notify();
+  AsyncTask.join();
+
+  EXPECT_TRUE(HasCancelled);
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -616,5 +616,38 @@
  O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
+json::Value toJSON(const CancelParams &CP) {
+  return json::Object{{"id", CP.ID}};
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &O, const CancelParams &CP) {
+  O << toJSON(CP);
+  return O;
+}
+
+llvm::Optional parseNumberOrString(const json::Value *Params) {
+  if (!Params)
+return llvm::None;
+  // ID is either a number or a string, check for both.
+  if(const auto AsString = Params->getAsString())
+return AsString->str();
+
+  if(const auto AsNumber = Params->getAsInteger())
+return itostr(AsNumber.getValue());
+
+  return llvm::None;
+}
+
+bool fromJSON(const json::Value &Params, CancelParams &CP) {
+  const auto ParamsAsObject = Params.getAsObject();
+  if (!ParamsAsObject)
+return false;
+  if (auto Parsed = parseNumberOrString(ParamsAsObject->get("id"))) {
+CP.ID = std::move(*Parsed);
+return true;

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Currently LSP clients cannot directly change IncludeFixIts or
EnableFunctionArgSnippets parameters. This patch is to provide them with a way
to enable/disable that functionality.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214

Files:
  clangd/tool/ClangdMain.cpp


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -194,6 +194,19 @@
 "'compile_commands.json' files")),
 llvm::cl::init(FilesystemCompileArgs), llvm::cl::Hidden);
 
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
+llvm::cl::desc(
+"Enables suggestion of completion items that needs additional changes. 
"
+"Like changing an arrow(->) member access to dot(.) member access."),
+llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+
+static llvm::cl::opt EnableFunctionArgSnippets(
+"enable-function-arg-snippets",
+llvm::cl::desc("Gives snippets for function arguments, when disabled only "
+   "gives parantheses for the call."),
+llvm::cl::init(clangd::CodeCompleteOptions().EnableFunctionArgSnippets));
+
 int main(int argc, char *argv[]) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
@@ -309,6 +322,8 @@
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
+  CCOpts.IncludeFixIts = IncludeFixIts;
+  CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -194,6 +194,19 @@
 "'compile_commands.json' files")),
 llvm::cl::init(FilesystemCompileArgs), llvm::cl::Hidden);
 
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
+llvm::cl::desc(
+"Enables suggestion of completion items that needs additional changes. "
+"Like changing an arrow(->) member access to dot(.) member access."),
+llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+
+static llvm::cl::opt EnableFunctionArgSnippets(
+"enable-function-arg-snippets",
+llvm::cl::desc("Gives snippets for function arguments, when disabled only "
+   "gives parantheses for the call."),
+llvm::cl::init(clangd::CodeCompleteOptions().EnableFunctionArgSnippets));
+
 int main(int argc, char *argv[]) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
@@ -309,6 +322,8 @@
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
+  CCOpts.IncludeFixIts = IncludeFixIts;
+  CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51170: [libc++] Remove race condition in std::async

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/include/future:556
 bool __has_value() const
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 

jfb wrote:
> I'm not auditing everything, but it seems like code above can still access 
> __state_ without holding __mut_? Like in the dtor.
> 
> Generally this patch lgtm because it's a step forward, but maybe we should 
> separately refactor the code to make it so that accesses to __state_ require 
> passing in a reference to lock_guard to show we actually hold __mut_. It 
> would ignore that reference, but that's a way to enforce, in the type system, 
> that __state_ is only touched when the lock is held.
> 
> WDYT?
I think you're right, and I filed this bug to keep track of the issue: 
https://bugs.llvm.org/show_bug.cgi?id=38688

Not all of them need a lock (some are in the constructor where only one thread 
has a reference to the data, for example), but most of them probably do.


Repository:
  rCXX libc++

https://reviews.llvm.org/D51170



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

[libcxx] r340608 - [libc++] Remove race condition in std::async

[Louis Dionne via cfe-commits]

Author: ldionne
Date: Fri Aug 24 07:00:59 2018
New Revision: 340608

URL: http://llvm.org/viewvc/llvm-project?rev=340608&view=rev
Log:
[libc++] Remove race condition in std::async

Summary:
The state associated to the future was set in one thread (with synchronization)
but read in another thread without synchronization, which led to a data race.

https://bugs.llvm.org/show_bug.cgi?id=38181
rdar://problem/42548261

Reviewers: mclow.lists, EricWF

Subscribers: christof, dexonsmith, cfe-commits

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

Added:
libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
Modified:
libcxx/trunk/include/future
libcxx/trunk/src/future.cpp

Modified: libcxx/trunk/include/future
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/future?rev=340608&r1=340607&r2=340608&view=diff
==
--- libcxx/trunk/include/future (original)
+++ libcxx/trunk/include/future Fri Aug 24 07:00:59 2018
@@ -556,13 +556,14 @@ public:
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 
 _LIBCPP_INLINE_VISIBILITY
-void __set_future_attached()
-{
+void __attach_future() {
 lock_guard __lk(__mut_);
+bool __has_future_attached = (__state_ & __future_attached) != 0;
+if (__has_future_attached)
+__throw_future_error(future_errc::future_already_retrieved);
+this->__add_shared();
 __state_ |= __future_attached;
 }
-_LIBCPP_INLINE_VISIBILITY
-bool __has_future_attached() const {return (__state_ & __future_attached) 
!= 0;}
 
 _LIBCPP_INLINE_VISIBILITY
 void __set_deferred() {__state_ |= deferred;}
@@ -1154,10 +1155,7 @@ template 
 future<_Rp>::future(__assoc_state<_Rp>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 struct __release_shared_count
@@ -1257,10 +1255,7 @@ template 
 future<_Rp&>::future(__assoc_state<_Rp&>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 template 

Modified: libcxx/trunk/src/future.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/future.cpp?rev=340608&r1=340607&r2=340608&view=diff
==
--- libcxx/trunk/src/future.cpp (original)
+++ libcxx/trunk/src/future.cpp Fri Aug 24 07:00:59 2018
@@ -179,10 +179,7 @@ __assoc_sub_state::__execute()
 future::future(__assoc_sub_state* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 future::~future()

Added: 
libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp?rev=340608&view=auto
==
--- 
libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp 
(added)
+++ 
libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp 
Fri Aug 24 07:00:59 2018
@@ -0,0 +1,58 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++98, c++03
+
+// This test is designed to cause and allow TSAN to detect a race condition
+// in std::async, as reported in https://bugs.llvm.org/show_bug.cgi?id=38682.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+static int worker(std::vector const& data) {
+  return std::accumulate(data.begin(), data.end(), 0);
+}
+
+static int& worker_ref(int& i) { return i; }
+
+static void worker_void() { }
+
+int main() {
+  // future
+  {
+std::vector const v{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker, v);
+  int answer = fut.get();
+  assert(answer == 55);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_ref, 
std::ref(i));
+  int& answer = fut.get();
+  assert(answer == i);
+

[PATCH] D51170: [libc++] Remove race condition in std::async

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340608: [libc++] Remove race condition in std::async 
(authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51170?vs=162200&id=162376#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51170

Files:
  libcxx/trunk/include/future
  libcxx/trunk/src/future.cpp
  libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp

Index: libcxx/trunk/src/future.cpp
===
--- libcxx/trunk/src/future.cpp
+++ libcxx/trunk/src/future.cpp
@@ -179,10 +179,7 @@
 future::future(__assoc_sub_state* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 future::~future()
Index: libcxx/trunk/include/future
===
--- libcxx/trunk/include/future
+++ libcxx/trunk/include/future
@@ -556,13 +556,14 @@
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 
 _LIBCPP_INLINE_VISIBILITY
-void __set_future_attached()
-{
+void __attach_future() {
 lock_guard __lk(__mut_);
+bool __has_future_attached = (__state_ & __future_attached) != 0;
+if (__has_future_attached)
+__throw_future_error(future_errc::future_already_retrieved);
+this->__add_shared();
 __state_ |= __future_attached;
 }
-_LIBCPP_INLINE_VISIBILITY
-bool __has_future_attached() const {return (__state_ & __future_attached) != 0;}
 
 _LIBCPP_INLINE_VISIBILITY
 void __set_deferred() {__state_ |= deferred;}
@@ -1154,10 +1155,7 @@
 future<_Rp>::future(__assoc_state<_Rp>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 struct __release_shared_count
@@ -1257,10 +1255,7 @@
 future<_Rp&>::future(__assoc_state<_Rp&>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 template 
Index: libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
===
--- libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
+++ libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
@@ -0,0 +1,58 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++98, c++03
+
+// This test is designed to cause and allow TSAN to detect a race condition
+// in std::async, as reported in https://bugs.llvm.org/show_bug.cgi?id=38682.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+static int worker(std::vector const& data) {
+  return std::accumulate(data.begin(), data.end(), 0);
+}
+
+static int& worker_ref(int& i) { return i; }
+
+static void worker_void() { }
+
+int main() {
+  // future
+  {
+std::vector const v{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker, v);
+  int answer = fut.get();
+  assert(answer == 55);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_ref, std::ref(i));
+  int& answer = fut.get();
+  assert(answer == i);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_void);
+  fut.get();
+}
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added inline comments.



Comment at: clangd/tool/ClangdMain.cpp:197
 
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",

I wonder if we should make the `IncludeFixIts` option hidden?
It currently only works for our YCM integration, VSCode and other clients break.



Comment at: clangd/tool/ClangdMain.cpp:198
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
+llvm::cl::desc(

This is a bit too generic name for the binary. Maybe `-completion-fixits`?



Comment at: clangd/tool/ClangdMain.cpp:202
+"Like changing an arrow(->) member access to dot(.) member access."),
+llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+

Maybe specify the value here explicitly?
The defaults for users of the clangd binary (what the option is for) is not 
necessarily the best default for the `ClangdServer` clients (what the default 
in the struct is for).
More importantly, it's useful to be more straightforward about the defaults we 
have for the options.



Comment at: clangd/tool/ClangdMain.cpp:205
+static llvm::cl::opt EnableFunctionArgSnippets(
+"enable-function-arg-snippets",
+llvm::cl::desc("Gives snippets for function arguments, when disabled only "

I wonder if we can infer this setting from the `-completion-style' (`=bundled` 
implies `enable-function-arg-snippets == false`)
@sammccall, WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214



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

[libcxx] r340609 - [libc++] Fix handling of negated character classes in regex

[Louis Dionne via cfe-commits]

Author: ldionne
Date: Fri Aug 24 07:10:28 2018
New Revision: 340609

URL: http://llvm.org/viewvc/llvm-project?rev=340609&view=rev
Log:
[libc++] Fix handling of negated character classes in regex

Summary:
This commit fixes a regression introduced in r316095, where we don't match
inverted character classes when there's no negated characrers in the []'s.

rdar://problem/43060054

Reviewers: mclow.lists, timshen, EricWF

Subscribers: christof, dexonsmith, cfe-commits

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

Added:

libcxx/trunk/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
Modified:
libcxx/trunk/include/regex

libcxx/trunk/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp

Modified: libcxx/trunk/include/regex
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/regex?rev=340609&r1=340608&r2=340609&view=diff
==
--- libcxx/trunk/include/regex (original)
+++ libcxx/trunk/include/regex Fri Aug 24 07:10:28 2018
@@ -2414,20 +2414,17 @@ __bracket_expression<_CharT, _Traits>::_
 goto __exit;
 }
 }
-// set of "__found" chars =
+// When there's at least one of __neg_chars_ and __neg_mask_, the set
+// of "__found" chars is
 //   union(complement(union(__neg_chars_, __neg_mask_)),
 // other cases...)
 //
-// __neg_chars_ and __neg_mask_'d better be handled together, as there
-// are no short circuit opportunities.
-//
-// In addition, when __neg_mask_/__neg_chars_ is empty, they should be
-// treated as all ones/all chars.
+// It doesn't make sense to check this when there are no __neg_chars_
+// and no __neg_mask_.
+if (!(__neg_mask_ == 0 && __neg_chars_.empty()))
 {
-  const bool __in_neg_mask = (__neg_mask_ == 0) ||
-  __traits_.isctype(__ch, __neg_mask_);
+const bool __in_neg_mask = __traits_.isctype(__ch, __neg_mask_);
   const bool __in_neg_chars =
-  __neg_chars_.empty() ||
   std::find(__neg_chars_.begin(), __neg_chars_.end(), __ch) !=
   __neg_chars_.end();
   if (!(__in_neg_mask || __in_neg_chars))

Added: 
libcxx/trunk/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp?rev=340609&view=auto
==
--- 
libcxx/trunk/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
 (added)
+++ 
libcxx/trunk/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
 Fri Aug 24 07:10:28 2018
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+// UNSUPPORTED: c++98, c++03
+
+// Make sure that we correctly match inverted character classes.
+
+#include 
+#include 
+
+
+int main() {
+assert(std::regex_match("X", std::regex("[X]")));
+assert(std::regex_match("X", std::regex("[XY]")));
+assert(!std::regex_match("X", std::regex("[^X]")));
+assert(!std::regex_match("X", std::regex("[^XY]")));
+
+assert(std::regex_match("X", std::regex("[\\S]")));
+assert(!std::regex_match("X", std::regex("[^\\S]")));
+
+assert(!std::regex_match("X", std::regex("[\\s]")));
+assert(std::regex_match("X", std::regex("[^\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\s\\S]")));
+assert(std::regex_match("X", std::regex("[^Y\\s]")));
+assert(!std::regex_match("X", std::regex("[^X\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\w]")));
+assert(std::regex_match("_", std::regex("[\\w]")));
+assert(!std::regex_match("X", std::regex("[^\\w]")));
+assert(!std::regex_match("_", std::regex("[^\\w]")));
+
+assert(!std::regex_match("X", std::regex("[\\W]")));
+assert(!std::regex_match("_", std::regex("[\\W]")));
+assert(std::regex_match("X", std::regex("[^\\W]")));
+assert(std::regex_match("_", std::regex("[^\\W]")));
+}

Modified: 
libcxx/trunk/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp?rev=340609&r1=340608&r2=340609&view=diff
==
--- 
libcxx/trunk/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp 
(original)
+++ 
libcxx/trunk/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp 
F

[PATCH] D50534: [libc++] Fix handling of negated character classes in regex

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX340609: [libc++] Fix handling of negated character classes 
in regex (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50534?vs=160169&id=162377#toc

Repository:
  rCXX libc++

https://reviews.llvm.org/D50534

Files:
  include/regex
  test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
  test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp


Index: include/regex
===
--- include/regex
+++ include/regex
@@ -2414,20 +2414,17 @@
 goto __exit;
 }
 }
-// set of "__found" chars =
+// When there's at least one of __neg_chars_ and __neg_mask_, the set
+// of "__found" chars is
 //   union(complement(union(__neg_chars_, __neg_mask_)),
 // other cases...)
 //
-// __neg_chars_ and __neg_mask_'d better be handled together, as there
-// are no short circuit opportunities.
-//
-// In addition, when __neg_mask_/__neg_chars_ is empty, they should be
-// treated as all ones/all chars.
+// It doesn't make sense to check this when there are no __neg_chars_
+// and no __neg_mask_.
+if (!(__neg_mask_ == 0 && __neg_chars_.empty()))
 {
-  const bool __in_neg_mask = (__neg_mask_ == 0) ||
-  __traits_.isctype(__ch, __neg_mask_);
+const bool __in_neg_mask = __traits_.isctype(__ch, __neg_mask_);
   const bool __in_neg_chars =
-  __neg_chars_.empty() ||
   std::find(__neg_chars_.begin(), __neg_chars_.end(), __ch) !=
   __neg_chars_.end();
   if (!(__in_neg_mask || __in_neg_chars))
Index: test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
===
--- test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
+++ test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
@@ -18,7 +18,7 @@
 
 #include 
 #include 
-#include "test_macros.h"
+
 
 // PR34310
 int main()
Index: test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
===
--- test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
+++ test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+// UNSUPPORTED: c++98, c++03
+
+// Make sure that we correctly match inverted character classes.
+
+#include 
+#include 
+
+
+int main() {
+assert(std::regex_match("X", std::regex("[X]")));
+assert(std::regex_match("X", std::regex("[XY]")));
+assert(!std::regex_match("X", std::regex("[^X]")));
+assert(!std::regex_match("X", std::regex("[^XY]")));
+
+assert(std::regex_match("X", std::regex("[\\S]")));
+assert(!std::regex_match("X", std::regex("[^\\S]")));
+
+assert(!std::regex_match("X", std::regex("[\\s]")));
+assert(std::regex_match("X", std::regex("[^\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\s\\S]")));
+assert(std::regex_match("X", std::regex("[^Y\\s]")));
+assert(!std::regex_match("X", std::regex("[^X\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\w]")));
+assert(std::regex_match("_", std::regex("[\\w]")));
+assert(!std::regex_match("X", std::regex("[^\\w]")));
+assert(!std::regex_match("_", std::regex("[^\\w]")));
+
+assert(!std::regex_match("X", std::regex("[\\W]")));
+assert(!std::regex_match("_", std::regex("[\\W]")));
+assert(std::regex_match("X", std::regex("[^\\W]")));
+assert(std::regex_match("_", std::regex("[^\\W]")));
+}


Index: include/regex
===
--- include/regex
+++ include/regex
@@ -2414,20 +2414,17 @@
 goto __exit;
 }
 }
-// set of "__found" chars =
+// When there's at least one of __neg_chars_ and __neg_mask_, the set
+// of "__found" chars is
 //   union(complement(union(__neg_chars_, __neg_mask_)),
 // other cases...)
 //
-// __neg_chars_ and __neg_mask_'d better be handled together, as there
-// are no short circuit opportunities.
-//
-// In addition, when __neg_mask_/__neg_chars_ is empty, they should be
-// treated as all ones/all chars.
+// It doesn't make sense to check this when there are no __neg_chars_
+// and no __neg_mask_.

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:28
+
+void RedundantStrcatCallsCheck::registerMatchers(MatchFinder* Finder) {
+  if (!getLangOpts().CPlusPlus) 

The formatting here looks off -- you should run the patch through clang-format, 
if you haven't already.



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:37-38
+  // Those are handled on the ancestor call.
+  const auto CallToEither = callExpr(
+  callee(functionDecl(hasAnyName("::absl::StrCat", "::absl::StrAppend";
+  Finder->addMatcher(

Can this be replaced by `anyOf(CallToStrcat, CallToStrappend)`?



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:50
+  int NumCalls = 0;
+  std::vector hints;
+};

s/hints/Hints



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:96
+int StartArg = CallExpr == RootCall && IsAppend;
+for (const auto Arg : CallExpr->arguments()) {
+  if (StartArg-- > 0) 

`const auto *` please (so it's obvious the type is a pointer).



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:99
+   continue;
+  if (const clang::CallExpr* sub =
+  ProcessArgument(Arg, Result, &CheckResult)) {

s/sub/Sub



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:113-119
+  if ((RootCall = Result.Nodes.getNodeAs("StrCat"))) {
+IsAppend = false;
+  } else if ((RootCall = Result.Nodes.getNodeAs("StrAppend"))) {
+IsAppend = true;
+  } else {
+return;
+  }

Elide braces



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:136
+
+  diag(RootCall->getBeginLoc(), "redundant StrCat calls")
+  << CheckResult.hints;

This text doesn't really help the user to understand what they've done wrong 
with their code. Perhaps `multiple calls to 'StrCat' can be flattened into a 
single call` or something along those lines?


https://reviews.llvm.org/D51132



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

[PATCH] D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Owen, shall i commit for you?


Repository:
  rC Clang

https://reviews.llvm.org/D50699



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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Hugo Gonzalez via Phabricator via cfe-commits]

hugoeg updated this revision to Diff 162379.
hugoeg marked 2 inline comments as done.
hugoeg added a comment.

fixed comments


https://reviews.llvm.org/D50542

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/NoInternalDepsCheck.cpp
  clang-tidy/abseil/NoInternalDepsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-no-internal-deps.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/Inputs/absl/external-file.h
  test/clang-tidy/Inputs/absl/strings/internal-file.h
  test/clang-tidy/abseil-no-internal-deps.cpp

Index: test/clang-tidy/abseil-no-internal-deps.cpp
===
--- test/clang-tidy/abseil-no-internal-deps.cpp
+++ test/clang-tidy/abseil-no-internal-deps.cpp
@@ -0,0 +1,39 @@
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t,  -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-internal-deps' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:23: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil [abseil-no-internal-deps]
+
+void DirectAcess() {
+  absl::strings_internal::InternalFunction();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+  absl::strings_internal::InternalTemplateFunction("a");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+}
+
+class FriendUsage {
+  friend struct absl::container_internal::InternalStruct;
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+};
+
+namespace absl {
+void OpeningNamespace() {
+  strings_internal::InternalFunction();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+}
+} // namespace absl
+
+// should not trigger warnings
+void CorrectUsage() {
+  std::string Str = absl::StringsFunction("a");
+  absl::SomeContainer b;
+}
+
+namespace absl {
+SomeContainer b;
+std::string Str = absl::StringsFunction("a");
+} // namespace absl
Index: test/clang-tidy/Inputs/absl/strings/internal-file.h
===
--- test/clang-tidy/Inputs/absl/strings/internal-file.h
+++ test/clang-tidy/Inputs/absl/strings/internal-file.h
@@ -0,0 +1,33 @@
+namespace std {
+struct string {
+  string(const char *);
+  ~string();
+};
+} // namespace std
+
+namespace absl {
+std::string StringsFunction(std::string s1) { return s1; }
+class SomeContainer {};
+namespace strings_internal {
+void InternalFunction() {}
+template  P InternalTemplateFunction(P a) {}
+} // namespace strings_internal
+
+namespace container_internal {
+struct InternalStruct {};
+} // namespace container_internal
+} // namespace absl
+
+// should not trigger warnings because inside Abseil files
+void DirectAcessInternal() {
+  absl::strings_internal::InternalFunction();
+  absl::strings_internal::InternalTemplateFunction("a");
+}
+
+class FriendUsageInternal {
+  friend struct absl::container_internal::InternalStruct;
+};
+
+namespace absl {
+void OpeningNamespaceInternally() { strings_internal::InternalFunction(); }
+} // namespace absl
Index: test/clang-tidy/Inputs/absl/external-file.h
===
--- test/clang-tidy/Inputs/absl/external-file.h
+++ test/clang-tidy/Inputs/absl/external-file.h
@@ -0,0 +1 @@
+void DirectAcess2() { absl::strings_internal::InternalFunction(); }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
 .. toctree::
abseil-duration-division
abseil-faster-strsplit-delimiter
+   abseil-no-internal-deps
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-internal-deps.rst
===
--- docs/clang-tidy/checks/abseil-no-internal-deps.rst
+++ docs/clang-tidy/checks/abseil-no-internal-deps.rst
@@ -0,0 +1,24 @@
+subl.. title:: clang-tidy - abseil-no-internal-deps
+
+abseil-no-internal-deps
+===
+
+Warns if code using Abseil depends on internal details. If something is in a
+namespace that includes the word “internal”, code is not allowed to depend upon
+it beaucse it’s an implementation detail. They cannot friend it, include it,
+you mention it or refer to it in any way. Doing so violates Abseil's
+compatibility guidelines and may result in breakage. See
+https://abseil.io/about/com

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Hugo Gonzalez via Phabricator via cfe-commits]

hugoeg added inline comments.



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:8
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:23: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil 
[abseil-no-internal-deps]
+

hokein wrote:
> Does the test get passed on the first run `RUN: %check_clang_tidy %s 
> abseil-no-internal-deps %t,  -- -- -I %S/Inputs` of the test? It will 
> suppress clang-tidy warnings from the header, and the warning here should not 
> appear.
Yes, the test passes cleanly on both runs, I just re ran it a couple times to 
make sure. 


https://reviews.llvm.org/D50542



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

[PATCH] D51090: [clangd] Add index benchmarks

[Kirill Bobyrev via Phabricator via cfe-commits]

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

Rebase on top of parent patch.


https://reviews.llvm.org/D51090

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/benchmarks/CMakeLists.txt
  clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
  clang-tools-extra/clangd/index/SymbolYAML.cpp
  clang-tools-extra/clangd/index/SymbolYAML.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -38,24 +38,6 @@
 
 enum class PCHStorageFlag { Disk, Memory };
 
-// Build an in-memory static index for global symbols from a YAML-format file.
-// The size of global symbols should be relatively small, so that all symbols
-// can be managed in memory.
-std::unique_ptr buildStaticIndex(llvm::StringRef YamlSymbolFile) {
-  auto Buffer = llvm::MemoryBuffer::getFile(YamlSymbolFile);
-  if (!Buffer) {
-llvm::errs() << "Can't open " << YamlSymbolFile << "\n";
-return nullptr;
-  }
-  auto Slab = symbolsFromYAML(Buffer.get()->getBuffer());
-  SymbolSlab::Builder SymsBuilder;
-  for (auto Sym : Slab)
-SymsBuilder.insert(Sym);
-
-  return UseDex ? dex::DexIndex::build(std::move(SymsBuilder).build())
-: MemIndex::build(std::move(SymsBuilder).build());
-}
-
 } // namespace
 
 static llvm::cl::opt CompileCommandsDir(
@@ -294,7 +276,7 @@
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   std::unique_ptr StaticIdx;
   if (EnableIndex && !YamlSymbolFile.empty()) {
-StaticIdx = buildStaticIndex(YamlSymbolFile);
+StaticIdx = buildStaticIndex(YamlSymbolFile, UseDex);
 Opts.StaticIndex = StaticIdx.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
Index: clang-tools-extra/clangd/index/SymbolYAML.h
===
--- clang-tools-extra/clangd/index/SymbolYAML.h
+++ clang-tools-extra/clangd/index/SymbolYAML.h
@@ -42,6 +42,12 @@
 // The YAML result is safe to concatenate if you have multiple symbol slabs.
 void SymbolsToYAML(const SymbolSlab &Symbols, llvm::raw_ostream &OS);
 
+// Build an in-memory static index for global symbols from a YAML-format file.
+// The size of global symbols should be relatively small, so that all symbols
+// can be managed in memory.
+std::unique_ptr buildStaticIndex(llvm::StringRef YamlSymbolFile,
+  bool UseDex);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/index/SymbolYAML.cpp
===
--- clang-tools-extra/clangd/index/SymbolYAML.cpp
+++ clang-tools-extra/clangd/index/SymbolYAML.cpp
@@ -9,6 +9,7 @@
 
 #include "SymbolYAML.h"
 #include "Index.h"
+#include "dex/DexIndex.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -203,5 +204,21 @@
   return OS.str();
 }
 
+std::unique_ptr buildStaticIndex(llvm::StringRef YamlSymbolFile,
+  bool UseDex) {
+  auto Buffer = llvm::MemoryBuffer::getFile(YamlSymbolFile);
+  if (!Buffer) {
+llvm::errs() << "Can't open " << YamlSymbolFile << "\n";
+return nullptr;
+  }
+  auto Slab = symbolsFromYAML(Buffer.get()->getBuffer());
+  SymbolSlab::Builder SymsBuilder;
+  for (auto Sym : Slab)
+SymsBuilder.insert(Sym);
+
+  return UseDex ? dex::DexIndex::build(std::move(SymsBuilder).build())
+: MemIndex::build(std::move(SymsBuilder).build());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
@@ -0,0 +1,148 @@
+//===--- DexBenchmark.cpp - DexIndex benchmarks -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../index/SymbolYAML.h"
+#include "../index/dex/DexIndex.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Regex.h"
+#include "benchmark/benchmark.h"
+#include 
+#include 
+#include 
+
+// FIXME(kbobyrev): Get rid of global variables and put them into
+// benchmark::State?
+std::unique_ptr Dex;
+std::unique_ptr Mem;
+
+std::vector Requests;
+
+std::vector RealRequests;
+
+std::string IndexFilename;
+std::string LogFilename;
+
+void readLogFile() {
+  llvm::Regex RequestMatcher("fuzzyFind\\(\"([a-zA-Z]*)\", scopes=\\[(.*)\\]");
+  llvm::SmallVector Matches;
+  std::ifstream InputStream(LogFilename);
+  std::string Log((std::istreambuf_iterator(InputStream)),
+  std::istreambu

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/Index.h:310
+
+  llvm::ArrayRef findOccurrences(const SymbolID &ID) const {
+auto It = SymbolOccurrences.find(ID);

As discussed offline: the merge of occurrences into SymbolSlab seems 
problematic to me.
On the consumer side, we have a separation between Symbol APIs and 
SymbolOccurrence APIs - they don't really interact.
The Symbol type can often only be used with SymbolSlab, and so including 
occurrences drags them into the mess for consumers that don't care about them.

For producers (index implementations), they will usually have both and they may 
want to share arena storage. But this probably doesn't matter much, and if it 
does we can use another mechanism (like allowing SymbolSlabBuilder and 
SymbolOccurrenceSlab to share UniqueStringSaver)



Comment at: clangd/index/SymbolCollector.cpp:439
   SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 

note that here we've done basically all the work needed to record the 
occurrence.

If you add a DenseMap then you'll have 
enough info at the end to fill in the occurrences, like we do with 
referenceddecls -> references.



Comment at: clangd/index/SymbolCollector.h:59
+  // If none, all symbol occurrences will be collected.
+  llvm::Optional> IDs = llvm::None;
+};

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Could you elaborate on what this option will be used for? How do we know 
> > > in advance which symbols we're interested in?
> > This is used for finding references in the AST as a part of the xref 
> > implementation, basically the workflow would be:
> > 
> > 1. find SymbolIDs of the symbol under the cursor, using 
> > `DeclarationAndMacrosFinder`
> > 2. run symbol collector to find all occurrences in the main AST with all 
> > SymbolIDs in #1
> > 3. query the index, to get more occurrences
> > 4. merge them  
> Can we instead find all the occurences in  `DeclarationAndMacrosFinder` 
> directly?
> Extra run of `SymbolCollector` means another AST traversal, which is slow by 
> itself, and SymbolCollector s designed for a much more hairy problem, its 
> interface is just not nicely suited for things like only occurrences. The 
> latter seems to be a simpler problem, and we can have a simpler interface to 
> solve it (possibly shared between SymbolCollector and 
> DeclarationAndMacrosFinder). WDYT?
> 
> 
Yeah, I don't think we need this.
For "find references in the AST" we have an implementation in XRefs for 
highlights which we don't need to share.



Comment at: clangd/index/SymbolCollector.h:40
   struct Options {
+struct CollectSymbolOptions {
+  bool CollectIncludePath = false;

Not sure this split is justified.
if IDs goes away (see below), all that's left can be represented in a 
SymbolOccurenceKind filter (which is 0 to collect no occurrences)



Comment at: clangd/index/SymbolCollector.h:76
+// If not null, SymbolCollector will collect symbol occurrences.
+const CollectOccurrenceOptions *OccurrenceOpts;
   };

collecting symbols doesn't actually need to be optional I think - it's the core 
responsibility of this class, and "find occurrences of a decl in an ast" can be 
implemented more easily in other ways


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Hugo Gonzalez via Phabricator via cfe-commits]

hugoeg updated this revision to Diff 162382.
hugoeg marked 7 inline comments as done.
hugoeg added a comment.

applied corrections form comments


https://reviews.llvm.org/D51132

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/RedundantStrcatCallsCheck.cpp
  clang-tidy/abseil/RedundantStrcatCallsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-redundant-strcat-calls.cpp

Index: test/clang-tidy/abseil-redundant-strcat-calls.cpp
===
--- test/clang-tidy/abseil-redundant-strcat-calls.cpp
+++ test/clang-tidy/abseil-redundant-strcat-calls.cpp
@@ -0,0 +1,188 @@
+// RUN: %check_clang_tidy %s abseil-redundant-strcat-calls %t
+
+int strlen(const char *);
+
+// Here we mimic the hierarchy of ::string.
+// We need to do so because we are matching on the fully qualified name of the
+// methods.
+struct __sso_string_base {};
+namespace __gnu_cxx {
+template 
+class __versa_string {
+ public:
+  const char *c_str() const;
+  const char *data() const;
+  int size() const;
+  int capacity() const;
+  int length() const;
+  bool empty() const;
+  char &operator[](int);
+  void clear();
+  void resize(int);
+  int compare(const __versa_string &) const;
+};
+}  // namespace __gnu_cxx
+
+namespace std {
+template 
+class char_traits {};
+template 
+class allocator {};
+}  // namespace std
+
+template ,
+  typename C = std::allocator>
+class basic_string : public __gnu_cxx::__versa_string {
+ public:
+  basic_string();
+  basic_string(const basic_string &);
+  basic_string(const char *, C = C());
+  basic_string(const char *, int, C = C());
+  basic_string(const basic_string &, int, int, C = C());
+  ~basic_string();
+
+  basic_string &operator+=(const basic_string &);
+};
+
+template 
+basic_string operator+(const basic_string &,
+const basic_string &);
+template 
+basic_string operator+(const basic_string &, const char *);
+
+typedef basic_string string;
+
+bool operator==(const string &, const string &);
+bool operator==(const string &, const char *);
+bool operator==(const char *, const string &);
+
+bool operator!=(const string &, const string &);
+bool operator<(const string &, const string &);
+bool operator>(const string &, const string &);
+bool operator<=(const string &, const string &);
+bool operator>=(const string &, const string &);
+
+namespace std {
+template ,
+  typename _Alloc = allocator<_CharT>>
+class basic_string;
+
+template 
+class basic_string {
+ public:
+  basic_string();
+  basic_string(const basic_string &);
+  basic_string(const char *, const _Alloc & = _Alloc());
+  basic_string(const char *, int, const _Alloc & = _Alloc());
+  basic_string(const basic_string &, int, int, const _Alloc & = _Alloc());
+  ~basic_string();
+
+  basic_string &operator+=(const basic_string &);
+
+  unsigned size() const;
+  unsigned length() const;
+  bool empty() const;
+};
+
+typedef basic_string string;
+}  // namespace std
+
+namespace absl {
+
+class string_view {
+ public:
+  typedef std::char_traits traits_type;
+
+  string_view();
+  string_view(const char *);
+  string_view(const string &);
+  string_view(const char *, int);
+  string_view(string_view, int);
+
+  template 
+  explicit operator ::basic_string() const;
+
+  const char *data() const;
+  int size() const;
+  int length() const;
+};
+
+bool operator==(string_view A, string_view B);
+
+struct AlphaNum {
+  AlphaNum(int i);
+  AlphaNum(double f);
+  AlphaNum(const char *c_str);
+  AlphaNum(const string &str);
+  AlphaNum(const string_view &pc);
+
+ private:
+  AlphaNum(const AlphaNum &);
+  AlphaNum &operator=(const AlphaNum &);
+};
+
+string StrCat(const AlphaNum &A);
+string StrCat(const AlphaNum &A, const AlphaNum &B);
+string StrCat(const AlphaNum &A, const AlphaNum &B, const AlphaNum &C);
+string StrCat(const AlphaNum &A, const AlphaNum &B, const AlphaNum &C,
+  const AlphaNum &D);
+
+// Support 5 or more arguments
+template 
+string StrCat(const AlphaNum &A, const AlphaNum &B, const AlphaNum &C,
+  const AlphaNum &D, const AlphaNum &E, const AV &... args);
+
+void StrAppend(string *Dest, const AlphaNum &A);
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B);
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B,
+   const AlphaNum &C);
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B,
+   const AlphaNum &C, const AlphaNum &D);
+
+// Support 5 or more arguments
+template 
+void StrAppend(string *Dest, const AlphaNum &A, const AlphaNum &B,
+   const AlphaNum &C, const AlphaNum &D, const AlphaNum &E,
+   const AV &... args);
+
+}  // namespace absl
+
+using absl::AlphaNum;
+using absl::StrAppend;
+using absl::StrCat;
+
+void Positives() {
+  string S = StrCat(1, Str

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:82
+  if (Call->getNumArgs() == 1) {
+diag(Op->getBeginLoc(), "call to absl::StrCat does nothing");
+return;

JonasToth wrote:
> please use 'absl::StrCat' in the diagnostics (same below) to signify that it 
> is code.
"does nothing" is not quite correct either; it does something, just not what 
the user expects. How about `call to 'absl::StrCat' has no effect`?



Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:91-92
+  diag(Op->getBeginLoc(),
+   "please use absl::StrAppend instead of absl::StrCat when appending to " 
   
+   "an existing string")
+  << FixItHint::CreateReplacement(

This wins the award for "kindest diagnostic message". :-D How about: `call 
'absl::StrAppend' instead of 'absl::StrCat' when appending to a string`?

That said, the diagnostic doesn't say *why* the code is wrong in the first 
place. The documentation doesn't really go into it either. Is it a correctness 
issue, a performance issue, something else?


https://reviews.llvm.org/D51061



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

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The description says "LSP clients", i.e. editors.
For editors (e.g. whether editor can handle snippets), LSP capability 
negotiation rather than flags is the right thing. I suspect this is true for 
including completion fixes.
For users (e.g. I like behavior X), flags are the right mechanism. But we don't 
need to expose every option, just the important ones.




Comment at: clangd/tool/ClangdMain.cpp:197
 
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",

ilya-biryukov wrote:
> I wonder if we should make the `IncludeFixIts` option hidden?
> It currently only works for our YCM integration, VSCode and other clients 
> break.
why would a user want to turn this on or off?



Comment at: clangd/tool/ClangdMain.cpp:198
+static llvm::cl::opt IncludeFixIts(
+"include-fixits",
+llvm::cl::desc(

ilya-biryukov wrote:
> This is a bit too generic name for the binary. Maybe `-completion-fixits`?
clang calls changes attached to diagnostics "fix-it hints", clangd calls them 
"fixes".

This isn't either of those, maybe it should have a different name?
Or if clang is going to consider them as another type of fixit, we should 
probably call them fixes.
Maybe `-completions-with-fixes`, which hints that if the flag is off you're 
going to lose these completions entirely?



Comment at: clangd/tool/ClangdMain.cpp:202
+"Like changing an arrow(->) member access to dot(.) member access."),
+llvm::cl::init(clangd::CodeCompleteOptions().IncludeFixIts));
+

ilya-biryukov wrote:
> Maybe specify the value here explicitly?
> The defaults for users of the clangd binary (what the option is for) is not 
> necessarily the best default for the `ClangdServer` clients (what the default 
> in the struct is for).
> More importantly, it's useful to be more straightforward about the defaults 
> we have for the options.
Not sure I agree here, this is consistent with the other options above. It's 
the other `ClangdServer` clients that are the weirdos, they should have to be 
explicit :-)

The defaults are clear when running with `-help`, which is how users will see 
them.



Comment at: clangd/tool/ClangdMain.cpp:205
+static llvm::cl::opt EnableFunctionArgSnippets(
+"enable-function-arg-snippets",
+llvm::cl::desc("Gives snippets for function arguments, when disabled only "

ilya-biryukov wrote:
> I wonder if we can infer this setting from the `-completion-style' 
> (`=bundled` implies `enable-function-arg-snippets == false`)
> @sammccall, WDYT?
They're not inextricably linked, the main connection is "these options both 
need good signaturehelp support to be really useful".
But we might want to link them just to cut down on number of options.

I don't have a strong opinion, I don't use `-completion-style=bundled`. Can we 
find a few people who do and ask if they like this setting?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51214



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

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D49851#1203676, @JonasToth wrote:

> In https://reviews.llvm.org/D49851#1202764, @Abpostelnicu wrote:
>
> > Strangely I haven't had those kind of issues but since you propose those 
> > modifications I would do one more modification, let's output everything to 
> > stdout and not stderr by doing:
> >
> >   if err:
> > sys.stdout.write(str(err) + '\n')
> >   
>
>
> You can make this a new revision, fixing the `byte` and `str` issues would be 
> more important now.
>
> The `byte` and `str` thingie is since the whole python3 releases or did it 
> change?


I'm not sure this is the fix for this, I think we should specify the encoding 
type when building the string from the byte like:

  if err:
sys.stdout.write(str(err, 'utf-8') + '\n')
   

And of course this must be done only on python 3+ since on python 2 the str 
ctor doesn't accept the encoding parameter.


Repository:
  rL LLVM

https://reviews.llvm.org/D49851



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

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm not certain how I feel about this as it seems to be removing functionality 
users may be relying on that I think is still technically correct. Can you 
describe more about why you think this should change?


Repository:
  rC Clang

https://reviews.llvm.org/D51187



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

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 162387.

https://reviews.llvm.org/D49851

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,10 +166,18 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
+
+if is_py2:
+  output_string = output
+  err_string = err
+else:
+  output_string = str(output, 'utf-8')
+  err_string = str(err, 'utf-8')
+
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  if err:
+sys.stderr.write(err_string + '\n')
 queue.task_done()
 
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,10 +166,18 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
+
+if is_py2:
+  output_string = output
+  err_string = err
+else:
+  output_string = str(output, 'utf-8')
+  err_string = str(err, 'utf-8')
+
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  if err:
+sys.stderr.write(err_string + '\n')
 queue.task_done()
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please dont work in this revision but create a new one (as this one is
already committed)!

Am 24.08.2018 um 17:34 schrieb Andi via Phabricator:

> Abpostelnicu updated this revision to Diff 162387.
> 
> https://reviews.llvm.org/D49851
> 
> Files:
> 
>   clang-tidy/tool/run-clang-tidy.py
> 
> Index: clang-tidy/tool/run-clang-tidy.py
>  ===
> 
>   - clang-tidy/tool/run-clang-tidy.py +++ clang-tidy/tool/run-clang-tidy.py 
> @@ -166,10 +166,18 @@ output, err = proc.communicate() if proc.returncode != 
> 0: failed_files.append(name) + +if is_py2: +  output_string = output 
> +  err_string = err +else: +  output_string = str(output, 
> 'utf-8') +  err_string = str(err, 'utf-8') + with lock:
> - sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
> - if err > 0:
> - sys.stderr.write(err + '\n') +  sys.stdout.write(' '.join(invocation) + 
> '\n' + output_string + '\n') +  if err: +
> sys.stderr.write(err_string + '\n') queue.task_done()


https://reviews.llvm.org/D49851



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

[PATCH] D51212: [OpenCL][Docs] Release notes for OpenCL in Clang

[Hans Wennborg via Phabricator via cfe-commits]

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

lgtm, thanks


https://reviews.llvm.org/D51212



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

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Hugo Gonzalez via Phabricator via cfe-commits]

hugoeg added inline comments.



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:37-38
+  // Those are handled on the ancestor call.
+  const auto CallToEither = callExpr(
+  callee(functionDecl(hasAnyName("::absl::StrCat", "::absl::StrAppend";
+  Finder->addMatcher(

aaron.ballman wrote:
> Can this be replaced by `anyOf(CallToStrcat, CallToStrappend)`?
I just tried it, it doesn't work, so we should probably keep it as is. 


https://reviews.llvm.org/D51132



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please create the patch with full context 
(https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51220



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

[PATCH] D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

@JonasToth Yes, please. Also https://reviews.llvm.org/D50697 :)
Thanks again for getting me started!


Repository:
  rC Clang

https://reviews.llvm.org/D50699



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I don't know a lot about how to write portable code!

But wouldn't this be out usecase? 
http://python-future.org/compatible_idioms.html#file-io-with-open
Using the `decode` is supposed to work in both pythons


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51220



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

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

One extra question: do you think this deserves a separate RFC?


Repository:
  rC Clang

https://reviews.llvm.org/D51200



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

[PATCH] D51212: [OpenCL][Docs] Release notes for OpenCL in Clang

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl accepted this revision.
yaxunl added a comment.

LGTM. Thanks.


https://reviews.llvm.org/D51212



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

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Hugo Gonzalez via Phabricator via cfe-commits]

hugoeg updated this revision to Diff 162390.
hugoeg marked 9 inline comments as done.
hugoeg added a comment.

applied corrections from comments


https://reviews.llvm.org/D51061

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/StrCatAppendCheck.cpp
  clang-tidy/abseil/StrCatAppendCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-str-cat-append.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-str-cat-append.cpp

Index: test/clang-tidy/abseil-str-cat-append.cpp
===
--- test/clang-tidy/abseil-str-cat-append.cpp
+++ test/clang-tidy/abseil-str-cat-append.cpp
@@ -0,0 +1,129 @@
+// RUN: %check_clang_tidy %s abseil-str-cat-append %t -- -- -I%S -std=c++11
+
+typedef unsigned __INT16_TYPE__ char16;
+typedef unsigned __INT32_TYPE__ char32;
+typedef __SIZE_TYPE__ size;
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template 
+struct basic_string {
+  typedef basic_string _Type;
+  basic_string();
+  basic_string(const C* p, const A& a = A());
+
+  const C* c_str() const;
+  const C* data() const;
+
+  _Type& append(const C* s);
+  _Type& append(const C* s, size n);
+  _Type& assign(const C* s);
+  _Type& assign(const C* s, size n);
+
+  int compare(const _Type&) const;
+  int compare(const C* s) const;
+  int compare(size pos, size len, const _Type&) const;
+  int compare(size pos, size len, const C* s) const;
+
+  size find(const _Type& str, size pos = 0) const;
+  size find(const C* s, size pos = 0) const;
+  size find(const C* s, size pos, size n) const;
+
+  _Type& insert(size pos, const _Type& str);
+  _Type& insert(size pos, const C* s);
+  _Type& insert(size pos, const C* s, size n);
+
+  _Type& operator+=(const _Type& str);
+  _Type& operator+=(const C* s);
+  _Type& operator=(const _Type& str);
+  _Type& operator=(const C* s);
+};
+
+typedef basic_string, std::allocator> string;
+typedef basic_string,
+ std::allocator>
+wstring;
+typedef basic_string, std::allocator>
+u16string;
+typedef basic_string, std::allocator>
+u32string;
+}  // namespace std
+
+std::string operator+(const std::string&, const std::string&);
+std::string operator+(const std::string&, const char*);
+std::string operator+(const char*, const std::string&);
+
+bool operator==(const std::string&, const std::string&);
+bool operator==(const std::string&, const char*);
+bool operator==(const char*, const std::string&);
+
+namespace llvm {
+struct StringRef {
+  StringRef(const char* p);
+  StringRef(const std::string&);
+};
+}  // namespace llvm
+
+namespace absl {
+
+struct AlphaNum {
+  AlphaNum(int i);
+  AlphaNum(double f);
+  AlphaNum(const char* c_str);
+  AlphaNum(const std::string& str);
+
+ private:
+  AlphaNum(const AlphaNum&);
+  AlphaNum& operator=(const AlphaNum&);
+};
+
+std::string StrCat(const AlphaNum& A);
+std::string StrCat(const AlphaNum& A, const AlphaNum& B);
+
+template 
+void Foo(A& a) {
+  a = StrCat(a);
+}
+
+void Bar() {
+  std::string A, B;
+  Foo(A);
+
+  std::string C = StrCat(A);
+  A = StrCat(A);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: call to 'absl::StrCat' has no effect
+  A = StrCat(A, B);
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: {{^}}  StrAppend(&A, B);
+  B = StrCat(A, B);
+
+#define M(X) X = StrCat(X, A)
+  M(B);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: #define M(X) X = StrCat(X, A)
+}
+
+void Regression_SelfAppend() {
+  std::string A;
+  A = StrCat(A, A);
+}
+
+}  // namespace absl
+
+void OutsideAbsl() {
+  std::string A, B;
+  A = absl::StrCat(A, B);
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: {{^}}  StrAppend(&A, B);
+}
+
+void OutisdeUsingAbsl() {
+  std::string A, B;
+  using absl::StrCat;
+  A = StrCat(A, B);
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: {{^}}  StrAppend(&A, B);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -5,6 +5,7 @@
 
 .. toctree::
abseil-duration-division
+   abseil-str-cat-append
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-str-cat-append.rst
===
--- docs/clang-tidy/checks/abseil-str-cat-append.rst
+++ docs/clang-tidy/checks/abseil-str-cat-append.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - abseil-str-cat-append
+
+abseil-str-cat-append
+=
+
+Flags uses of ``absl::StrCat()`` to appen

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 162391.

https://reviews.llvm.org/D51220

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,18 +166,10 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
-
-if is_py2:
-  output_string = output
-  err_string = err
-else:
-  output_string = str(output, 'utf-8')
-  err_string = str(err, 'utf-8')
-
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + 
'\n')
   if err:
-sys.stderr.write(err_string + '\n')
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,18 +166,10 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
-
-if is_py2:
-  output_string = output
-  err_string = err
-else:
-  output_string = str(output, 'utf-8')
-  err_string = str(err, 'utf-8')
-
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
   if err:
-sys.stderr.write(err_string + '\n')
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D51220#1212463, @JonasToth wrote:

> I don't know a lot about how to write portable code!
>
> But wouldn't this be out usecase? 
> http://python-future.org/compatible_idioms.html#file-io-with-open
>  Using the `decode` is supposed to work in both pythons


Yes, indeed, this is more elegant solution!


https://reviews.llvm.org/D51220



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could you please verify it is actually working?

Am 24.08.2018 um 18:05 schrieb Andi via Phabricator:

> Abpostelnicu added a comment.
> 
> In https://reviews.llvm.org/D51220#1212463, @JonasToth wrote:
> 
>> I don't know a lot about how to write portable code!
>> 
>> But wouldn't this be out usecase? 
>> http://python-future.org/compatible_idioms.html#file-io-with-open
>> 
>>   Using the `decode` is supposed to work in both pythons
> 
> Yes, indeed, this is more elegant solution!
> 
> https://reviews.llvm.org/D51220


https://reviews.llvm.org/D51220



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

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I can confirm tested on:
2.7.15
3.7.0

On both it worked.

- F7047650: signature.asc 


https://reviews.llvm.org/D51220



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

[PATCH] D43783: [OpenCL] Remove block invoke function from emitted block literal struct

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.
Herald added a subscriber: jvesely.

In https://reviews.llvm.org/D43783#1204353, @svenvh wrote:

> Sorry for digging up an old commit...
>
> Apparently this broke block arguments, e.g. the following test case:
>
>   int foo(int (^ bl)(void)) {
> return bl();
>   }
>  
>   int get21() {
> return foo(^{return 21;});
>   }
>  
>   int get42() {
> return foo(^{return 42;});
>   }
>
>
> In particular, the VarDecl that `getBlockExpr()` sees doesn't have an 
> initializer when the called block comes from an argument (causing clang to 
> crash).


Sorry for the delay. I think block should not be allowed as function argument 
since this generally leads indirect function calls therefore requires support 
of function pointer. It will rely on optimizations to get rid of indirect 
function calls.


Repository:
  rC Clang

https://reviews.llvm.org/D43783



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

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Hugo Gonzalez via Phabricator via cfe-commits]

hugoeg updated this revision to Diff 162395.
hugoeg added a comment.

ran svn update


https://reviews.llvm.org/D51061

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/StrCatAppendCheck.cpp
  clang-tidy/abseil/StrCatAppendCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-str-cat-append.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-str-cat-append.cpp

Index: test/clang-tidy/abseil-str-cat-append.cpp
===
--- test/clang-tidy/abseil-str-cat-append.cpp
+++ test/clang-tidy/abseil-str-cat-append.cpp
@@ -0,0 +1,129 @@
+// RUN: %check_clang_tidy %s abseil-str-cat-append %t -- -- -I%S -std=c++11
+
+typedef unsigned __INT16_TYPE__ char16;
+typedef unsigned __INT32_TYPE__ char32;
+typedef __SIZE_TYPE__ size;
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template 
+struct basic_string {
+  typedef basic_string _Type;
+  basic_string();
+  basic_string(const C* p, const A& a = A());
+
+  const C* c_str() const;
+  const C* data() const;
+
+  _Type& append(const C* s);
+  _Type& append(const C* s, size n);
+  _Type& assign(const C* s);
+  _Type& assign(const C* s, size n);
+
+  int compare(const _Type&) const;
+  int compare(const C* s) const;
+  int compare(size pos, size len, const _Type&) const;
+  int compare(size pos, size len, const C* s) const;
+
+  size find(const _Type& str, size pos = 0) const;
+  size find(const C* s, size pos = 0) const;
+  size find(const C* s, size pos, size n) const;
+
+  _Type& insert(size pos, const _Type& str);
+  _Type& insert(size pos, const C* s);
+  _Type& insert(size pos, const C* s, size n);
+
+  _Type& operator+=(const _Type& str);
+  _Type& operator+=(const C* s);
+  _Type& operator=(const _Type& str);
+  _Type& operator=(const C* s);
+};
+
+typedef basic_string, std::allocator> string;
+typedef basic_string,
+ std::allocator>
+wstring;
+typedef basic_string, std::allocator>
+u16string;
+typedef basic_string, std::allocator>
+u32string;
+}  // namespace std
+
+std::string operator+(const std::string&, const std::string&);
+std::string operator+(const std::string&, const char*);
+std::string operator+(const char*, const std::string&);
+
+bool operator==(const std::string&, const std::string&);
+bool operator==(const std::string&, const char*);
+bool operator==(const char*, const std::string&);
+
+namespace llvm {
+struct StringRef {
+  StringRef(const char* p);
+  StringRef(const std::string&);
+};
+}  // namespace llvm
+
+namespace absl {
+
+struct AlphaNum {
+  AlphaNum(int i);
+  AlphaNum(double f);
+  AlphaNum(const char* c_str);
+  AlphaNum(const std::string& str);
+
+ private:
+  AlphaNum(const AlphaNum&);
+  AlphaNum& operator=(const AlphaNum&);
+};
+
+std::string StrCat(const AlphaNum& A);
+std::string StrCat(const AlphaNum& A, const AlphaNum& B);
+
+template 
+void Foo(A& a) {
+  a = StrCat(a);
+}
+
+void Bar() {
+  std::string A, B;
+  Foo(A);
+
+  std::string C = StrCat(A);
+  A = StrCat(A);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: call to 'absl::StrCat' has no effect
+  A = StrCat(A, B);
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: {{^}}  StrAppend(&A, B);
+  B = StrCat(A, B);
+
+#define M(X) X = StrCat(X, A)
+  M(B);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: #define M(X) X = StrCat(X, A)
+}
+
+void Regression_SelfAppend() {
+  std::string A;
+  A = StrCat(A, A);
+}
+
+}  // namespace absl
+
+void OutsideAbsl() {
+  std::string A, B;
+  A = absl::StrCat(A, B);
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: {{^}}  StrAppend(&A, B);
+}
+
+void OutisdeUsingAbsl() {
+  std::string A, B;
+  using absl::StrCat;
+  A = StrCat(A, B);
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: call 'absl::StrAppend' instead of 'absl::StrCat' when appending to a string
+// CHECK-FIXES: {{^}}  StrAppend(&A, B);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -7,6 +7,7 @@
abseil-duration-division
abseil-faster-strsplit-delimiter
abseil-string-find-startswith
+   abseil-str-cat-append
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-str-cat-append.rst
===
--- docs/clang-tidy/checks/abseil-str-cat-append.rst
+++ docs/clang-tidy/checks/abseil-str-cat-append.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - abseil-str-cat-append
+
+abseil-str-cat-append
+=
+
+Flags uses of ``absl::StrCat()`` to append to a ``std:

[clang-tools-extra] r340620 - [clang-doc] Fix memory leaks

[Julie Hockett via cfe-commits]

Author: juliehockett
Date: Fri Aug 24 09:43:46 2018
New Revision: 340620

URL: http://llvm.org/viewvc/llvm-project?rev=340620&view=rev
Log:
[clang-doc] Fix memory leaks

Adds a virtual destructor to the base Info class.

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

Modified:
clang-tools-extra/trunk/clang-doc/Representation.h
clang-tools-extra/trunk/clang-doc/Serialize.cpp

Modified: clang-tools-extra/trunk/clang-doc/Representation.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/Representation.h?rev=340620&r1=340619&r2=340620&view=diff
==
--- clang-tools-extra/trunk/clang-doc/Representation.h (original)
+++ clang-tools-extra/trunk/clang-doc/Representation.h Fri Aug 24 09:43:46 2018
@@ -162,6 +162,8 @@ struct Info {
   Info(const Info &Other) = delete;
   Info(Info &&Other) = default;
 
+  virtual ~Info() = default;
+
   SymbolID USR =
   SymbolID(); // Unique identifier for the decl described by this Info.
   const InfoType IT = InfoType::IT_default; // InfoType of this particular 
Info.

Modified: clang-tools-extra/trunk/clang-doc/Serialize.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/Serialize.cpp?rev=340620&r1=340619&r2=340620&view=diff
==
--- clang-tools-extra/trunk/clang-doc/Serialize.cpp (original)
+++ clang-tools-extra/trunk/clang-doc/Serialize.cpp Fri Aug 24 09:43:46 2018
@@ -361,7 +361,7 @@ std::unique_ptr emitInfo(const Fun
 I->USR = Func.Namespace[0].USR;
   else
 I->USR = SymbolID();
-  I->ChildFunctions.push_back(std::move(Func));
+  I->ChildFunctions.emplace_back(std::move(Func));
   return std::unique_ptr{std::move(I)};
 }
 
@@ -382,7 +382,7 @@ std::unique_ptr emitInfo(const CXX
   // Wrap in enclosing scope
   auto I = llvm::make_unique();
   I->USR = ParentUSR;
-  I->ChildFunctions.push_back(std::move(Func));
+  I->ChildFunctions.emplace_back(std::move(Func));
   return std::unique_ptr{std::move(I)};
 }
 
@@ -402,13 +402,13 @@ std::unique_ptr emitInfo(const Enu
 case InfoType::IT_namespace: {
   auto I = llvm::make_unique();
   I->USR = Enum.Namespace[0].USR;
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 case InfoType::IT_record: {
   auto I = llvm::make_unique();
   I->USR = Enum.Namespace[0].USR;
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 default:
@@ -419,7 +419,7 @@ std::unique_ptr emitInfo(const Enu
   // Put in global namespace
   auto I = llvm::make_unique();
   I->USR = SymbolID();
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 


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

[PATCH] D51137: [clang-doc] Fix memory leaks

[Julie Hockett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340620: [clang-doc] Fix memory leaks (authored by 
juliehockett, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51137?vs=162081&id=162399#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51137

Files:
  clang-tools-extra/trunk/clang-doc/Representation.h
  clang-tools-extra/trunk/clang-doc/Serialize.cpp


Index: clang-tools-extra/trunk/clang-doc/Representation.h
===
--- clang-tools-extra/trunk/clang-doc/Representation.h
+++ clang-tools-extra/trunk/clang-doc/Representation.h
@@ -162,6 +162,8 @@
   Info(const Info &Other) = delete;
   Info(Info &&Other) = default;
 
+  virtual ~Info() = default;
+
   SymbolID USR =
   SymbolID(); // Unique identifier for the decl described by this Info.
   const InfoType IT = InfoType::IT_default; // InfoType of this particular 
Info.
Index: clang-tools-extra/trunk/clang-doc/Serialize.cpp
===
--- clang-tools-extra/trunk/clang-doc/Serialize.cpp
+++ clang-tools-extra/trunk/clang-doc/Serialize.cpp
@@ -361,7 +361,7 @@
 I->USR = Func.Namespace[0].USR;
   else
 I->USR = SymbolID();
-  I->ChildFunctions.push_back(std::move(Func));
+  I->ChildFunctions.emplace_back(std::move(Func));
   return std::unique_ptr{std::move(I)};
 }
 
@@ -382,7 +382,7 @@
   // Wrap in enclosing scope
   auto I = llvm::make_unique();
   I->USR = ParentUSR;
-  I->ChildFunctions.push_back(std::move(Func));
+  I->ChildFunctions.emplace_back(std::move(Func));
   return std::unique_ptr{std::move(I)};
 }
 
@@ -402,13 +402,13 @@
 case InfoType::IT_namespace: {
   auto I = llvm::make_unique();
   I->USR = Enum.Namespace[0].USR;
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 case InfoType::IT_record: {
   auto I = llvm::make_unique();
   I->USR = Enum.Namespace[0].USR;
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 default:
@@ -419,7 +419,7 @@
   // Put in global namespace
   auto I = llvm::make_unique();
   I->USR = SymbolID();
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 


Index: clang-tools-extra/trunk/clang-doc/Representation.h
===
--- clang-tools-extra/trunk/clang-doc/Representation.h
+++ clang-tools-extra/trunk/clang-doc/Representation.h
@@ -162,6 +162,8 @@
   Info(const Info &Other) = delete;
   Info(Info &&Other) = default;
 
+  virtual ~Info() = default;
+
   SymbolID USR =
   SymbolID(); // Unique identifier for the decl described by this Info.
   const InfoType IT = InfoType::IT_default; // InfoType of this particular Info.
Index: clang-tools-extra/trunk/clang-doc/Serialize.cpp
===
--- clang-tools-extra/trunk/clang-doc/Serialize.cpp
+++ clang-tools-extra/trunk/clang-doc/Serialize.cpp
@@ -361,7 +361,7 @@
 I->USR = Func.Namespace[0].USR;
   else
 I->USR = SymbolID();
-  I->ChildFunctions.push_back(std::move(Func));
+  I->ChildFunctions.emplace_back(std::move(Func));
   return std::unique_ptr{std::move(I)};
 }
 
@@ -382,7 +382,7 @@
   // Wrap in enclosing scope
   auto I = llvm::make_unique();
   I->USR = ParentUSR;
-  I->ChildFunctions.push_back(std::move(Func));
+  I->ChildFunctions.emplace_back(std::move(Func));
   return std::unique_ptr{std::move(I)};
 }
 
@@ -402,13 +402,13 @@
 case InfoType::IT_namespace: {
   auto I = llvm::make_unique();
   I->USR = Enum.Namespace[0].USR;
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 case InfoType::IT_record: {
   auto I = llvm::make_unique();
   I->USR = Enum.Namespace[0].USR;
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 default:
@@ -419,7 +419,7 @@
   // Put in global namespace
   auto I = llvm::make_unique();
   I->USR = SymbolID();
-  I->ChildEnums.push_back(std::move(Enum));
+  I->ChildEnums.emplace_back(std::move(Enum));
   return std::unique_ptr{std::move(I)};
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51132: [clang-tidy] abseil-redundant-strcat-calls-check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:28
+
+void RedundantStrcatCallsCheck::registerMatchers(MatchFinder* Finder) {
+  if (!getLangOpts().CPlusPlus) 

aaron.ballman wrote:
> The formatting here looks off -- you should run the patch through 
> clang-format, if you haven't already.
This comment is marked done but the formatting in the patch still doesn't match 
the LLVM coding style. For instance, pointers bind to the right, not to the 
left in our style.



Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:135
+  diag(RootCall->getBeginLoc(), 
+  "multiple calls to 'StrCat' can be flattened into a single call")
+  << CheckResult.Hints;

One more change, based on comments in other reviews, this should probably be 
`'absl::StrCat'` instead of just `'StrCat'`.


https://reviews.llvm.org/D51132



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