[PATCH] D50805: Don't warn on returning the address of a label

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

> I think that, ultimately, this warning isn't worth the code complexity in 
> clang to support it.

The code complexity is //at most// 11 lines (the 11 lines removed by this 
patch), which really isn't much.  But there's an interface-complexity argument 
to be made, for sure.  If you added a new option `-Wret-addr-label` as 
suggested above (for a total patch of +2 lines), then is it accurate to say:

- if `-Wret-addr-label` was enabled by default, we know of at least one 
codebase that would pass `-Wno-ret-addr-label` to their build
- if `-Wret-addr-label` was disabled by default, we don't know of any codebases 
that would voluntarily enable it

And if nobody would enable it voluntarily... might as well eliminate it, right?




Comment at: clang/test/Analysis/stack-addr-ps.cpp:81
 label:
-void *const &x = &&label; // expected-note {{binding reference variable 
'x' here}}
-return x; // expected-warning {{returning address of label, which is 
local}}
+void *const &x = &&label;
+return x;

Is it just me, or is this test case relying on lifetime extension for no good 
reason? Why is this not `void *const x = &&label;`?


https://reviews.llvm.org/D50805



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I see, thank you for the clarification :)

Am 15.08.2018 um 19:25 schrieb Shuai Wang via Phabricator:

> shuaiwang added inline comments.
> 
> 
>  Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:410
>  +  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
>  +  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
>  +}
> 
>  
> 
> JonasToth wrote:
> 
>> Out of curiosity: Why is the result with `y` different from the result for 
>> `x`? Both time `x` is mutated and `g()` mutates them.
> 
> This is ultimately caused by not handling pointers yet.
>  As soon as the address of an object is taken we assume the object is mutated.
>  e.g.
> 
>   void f(const int*);
>   void g() {
> int x;
> f(&x); // <-- address of x taken, assume mutation
> int y[10];
> f(y); // <-- address of y taken, assume mutation
>   }
> 
> 
> And in all such cases the "mutated by" expression is the expression that 
> takes the address.
> 
> So back in this case, `g(x)` mutates `x` because we're assuming `g` mutates 
> its argument through non-const reference. Note that the declared `g` might 
> not be the one actually being called because of overload resolution, there 
> could be another `void g(char(&)[8])`
>  While for `g(y)` we know it's calling the `void g(char*)` so there's an 
> array to pointer decay, and the decay is the point we assumed mutation not 
> the function call.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50619


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

You are totally right.

Am 16.08.2018 um 02:41 schrieb Shuai Wang via Phabricator:

> shuaiwang added inline comments.
> 
> 
>  Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:309
> 
> +TEST(ExprMutationAnalyzerTest, CallUnresolved) {
>  +  auto AST =
> 
>  
> 
> JonasToth wrote:
> 
>> I think we are missing tests for non-type template paramters (`template 
>> `). They should behave the same. But the following case would not 
>> be a mutation:
>> 
>>   void non_mutating(const char* Array, int size) { /* Foo */ }
>>   template 
>>   struct ArrayLike {
>> char* data[N]; // Initialized to something
>> void SomeFunction() {
>>   non_mutating(data, N);
>> }
>>   };
>> 
>> 
>> The difference between the 'normal' and non-type templates would be, that 
>> `N` is not mutatable at all and the semantics is clear (builtin integer-like 
>> type).
>> 
>> If the current implementation would not figure that out, you can just add a 
>> test for it and assume a mutation. Handling non-type templates later is 
>> absolutly ok.
> 
> We have to assume `data` is mutated here as well. I'll add a test case for 
> this.
> 
>   void g(const char*, int); // <-- doesn't mutate
>   void g(char(&)[8], int); // <-- do mutate
>   
>   template 
>   void f() {
>   char data[N];
>   g(data, N); // <-- we don't know which `g` will be called yet
>   }
>   
>   void h() {
>   f<8>(); // <-- f calls g(char(&)[8], int) internally
>   f<9>(); // <-- f calls g(const char*, int) internally
>   }
> 
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D50619


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50792: [ASTImporter] Add test for member pointer types.

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50792



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

[PATCH] D50793: [ASTImporter] Add test for importing CompoundAssignOperators

[Gabor Marton via Phabricator via cfe-commits]

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

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D50793



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

[PATCH] D50810: [ASTImporter] Add test for DoStmt

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50810



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

r339845 - [X86] Remove masking from the 512-bit paddus/psubus builtins. Use a select builtin instead.

[Craig Topper via cfe-commits]

Author: ctopper
Date: Thu Aug 16 00:28:06 2018
New Revision: 339845

URL: http://llvm.org/viewvc/llvm-project?rev=339845&view=rev
Log:
[X86] Remove masking from the 512-bit paddus/psubus builtins. Use a select 
builtin instead.

Modified:
cfe/trunk/include/clang/Basic/BuiltinsX86.def
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/Headers/avx512bwintrin.h

Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsX86.def?rev=339845&r1=339844&r2=339845&view=diff
==
--- cfe/trunk/include/clang/Basic/BuiltinsX86.def (original)
+++ cfe/trunk/include/clang/Basic/BuiltinsX86.def Thu Aug 16 00:28:06 2018
@@ -1040,8 +1040,8 @@ TARGET_BUILTIN(__builtin_ia32_packusdw51
 TARGET_BUILTIN(__builtin_ia32_packuswb512, "V64cV32sV32s", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_paddsb512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_paddsw512, "V32sV32sV32s", "ncV:512:", 
"avx512bw")
-TARGET_BUILTIN(__builtin_ia32_paddusb512_mask, "V64cV64cV64cV64cULLi", 
"ncV:512:", "avx512bw")
-TARGET_BUILTIN(__builtin_ia32_paddusw512_mask, "V32sV32sV32sV32sUi", 
"ncV:512:", "avx512bw")
+TARGET_BUILTIN(__builtin_ia32_paddusb512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
+TARGET_BUILTIN(__builtin_ia32_paddusw512, "V32sV32sV32s", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_pmaxsb512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_pmaxsw512, "V32sV32sV32s", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_pmaxub512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
@@ -1053,8 +1053,8 @@ TARGET_BUILTIN(__builtin_ia32_pminuw512,
 TARGET_BUILTIN(__builtin_ia32_pshufb512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_psubsb512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
 TARGET_BUILTIN(__builtin_ia32_psubsw512, "V32sV32sV32s", "ncV:512:", 
"avx512bw")
-TARGET_BUILTIN(__builtin_ia32_psubusb512_mask, "V64cV64cV64cV64cULLi", 
"ncV:512:", "avx512bw")
-TARGET_BUILTIN(__builtin_ia32_psubusw512_mask, "V32sV32sV32sV32sUi", 
"ncV:512:", "avx512bw")
+TARGET_BUILTIN(__builtin_ia32_psubusb512, "V64cV64cV64c", "ncV:512:", 
"avx512bw")
+TARGET_BUILTIN(__builtin_ia32_psubusw512, "V32sV32sV32s", "ncV:512:", 
"avx512bw")
 
 TARGET_BUILTIN(__builtin_ia32_vpconflictdi_128_mask, "V2LLiV2LLiV2LLiUc", 
"ncV:128:", "avx512cd,avx512vl")
 TARGET_BUILTIN(__builtin_ia32_vpconflictdi_256_mask, "V4LLiV4LLiV4LLiUc", 
"ncV:256:", "avx512cd,avx512vl")

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=339845&r1=339844&r2=339845&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Thu Aug 16 00:28:06 2018
@@ -8931,12 +8931,6 @@ static Value *EmitX86AddSubSatExpr(CodeG
 Res = CGF.Builder.CreateSub(Select, Ops[1]);
   }
 
-  if (E->getNumArgs() == 4) { // For masked intrinsics.
-Value *VecSRC = Ops[2];
-Value *Mask = Ops[3];
-return EmitX86Select(CGF, Mask, Res, VecSRC);
-  }
-
   return Res;
 }
 
@@ -10563,15 +10557,15 @@ Value *CodeGenFunction::EmitX86BuiltinEx
 Load->setVolatile(true);
 return Load;
   }
-  case X86::BI__builtin_ia32_paddusb512_mask:
-  case X86::BI__builtin_ia32_paddusw512_mask:
+  case X86::BI__builtin_ia32_paddusb512:
+  case X86::BI__builtin_ia32_paddusw512:
   case X86::BI__builtin_ia32_paddusb256:
   case X86::BI__builtin_ia32_paddusw256:
   case X86::BI__builtin_ia32_paddusb128:
   case X86::BI__builtin_ia32_paddusw128:
 return EmitX86AddSubSatExpr(*this, E, Ops, true /* IsAddition */);
-  case X86::BI__builtin_ia32_psubusb512_mask:
-  case X86::BI__builtin_ia32_psubusw512_mask:
+  case X86::BI__builtin_ia32_psubusb512:
+  case X86::BI__builtin_ia32_psubusw512:
   case X86::BI__builtin_ia32_psubusb256:
   case X86::BI__builtin_ia32_psubusw256:
   case X86::BI__builtin_ia32_psubusb128:

Modified: cfe/trunk/lib/Headers/avx512bwintrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/avx512bwintrin.h?rev=339845&r1=339844&r2=339845&view=diff
==
--- cfe/trunk/lib/Headers/avx512bwintrin.h (original)
+++ cfe/trunk/lib/Headers/avx512bwintrin.h Thu Aug 16 00:28:06 2018
@@ -466,57 +466,45 @@ _mm512_maskz_adds_epi16 (__mmask32 __U,
 static __inline__ __m512i __DEFAULT_FN_ATTRS
 _mm512_adds_epu8 (__m512i __A, __m512i __B)
 {
-  return (__m512i) __builtin_ia32_paddusb512_mask ((__v64qi) __A,
-  (__v64qi) __B,
-  (__v64qi) _mm512_setzero_si512(),
-  (__mmask64) -1);
+  return (__m512i)__builtin_ia32_paddusb512((__v64qi) __A, (__v64qi) __B);
 }
 
 static __inline__ __m512i __DEFAULT_FN_ATTRS
-_mm512_mask_adds_epu8 (__m512i __W, __mmask64 __U, __m512i __

[PATCH] D50811: [ASTImporter] Add test for WhileStmt

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50811



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

[PATCH] D50813: [ASTImporter] Add test for IndirectGotoStmt

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50813



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

[PATCH] D49798: [ASTImporter] Adding some friend function related unittests.

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

balazske updated this revision to Diff 160972.
balazske added a comment.

- Renamed variables, corrected a variable type.


Repository:
  rC Clang

https://reviews.llvm.org/D49798

Files:
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2275,6 +2275,211 @@
   EXPECT_EQ(ImportedD1->getPreviousDecl(), ImportedD);
 }
 
+TEST_P(ImportFriendFunctions, Lookup) {
+  auto FunctionPattern = functionDecl(hasName("f"));
+  auto ClassPattern = cxxRecordDecl(hasName("X"));
+
+  TranslationUnitDecl *FromTU =
+  getTuDecl("struct X { friend void f(); };", Lang_CXX, "input0.cc");
+  auto *FromD = FirstDeclMatcher().match(FromTU, FunctionPattern);
+  ASSERT_TRUE(FromD->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  ASSERT_FALSE(FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  {
+auto FromName = FromD->getDeclName();
+auto *Class = FirstDeclMatcher().match(FromTU, ClassPattern);
+auto LookupRes = Class->noload_lookup(FromName);
+ASSERT_EQ(LookupRes.size(), 0u);
+LookupRes = FromTU->noload_lookup(FromName);
+ASSERT_EQ(LookupRes.size(), 1u);
+  }
+
+  auto *ToD = cast(Import(FromD, Lang_CXX));
+  auto ToName = ToD->getDeclName();
+
+  TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  auto *Class = FirstDeclMatcher().match(ToTU, ClassPattern);
+  auto LookupRes = Class->noload_lookup(ToName);
+  EXPECT_EQ(LookupRes.size(), 0u);
+  LookupRes = ToTU->noload_lookup(ToName);
+  EXPECT_EQ(LookupRes.size(), 1u);
+
+  EXPECT_EQ(DeclCounter().match(ToTU, FunctionPattern), 1u);
+  auto *To0 = FirstDeclMatcher().match(ToTU, FunctionPattern);
+  EXPECT_TRUE(To0->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  EXPECT_FALSE(To0->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+}
+
+TEST_P(ImportFriendFunctions, DISABLED_LookupWithProtoAfter) {
+  auto FunctionPattern = functionDecl(hasName("f"));
+  auto ClassPattern = cxxRecordDecl(hasName("X"));
+
+  TranslationUnitDecl *FromTU = getTuDecl(
+  "struct X { friend void f(); };"
+  // This proto decl makes f available to normal
+  // lookup, otherwise it is hidden.
+  // Normal C++ lookup (implemented in
+  // `clang::Sema::CppLookupName()` and in `LookupDirect()`)
+  // returns the found `NamedDecl` only if the set IDNS is matched
+  "void f();",
+  Lang_CXX, "input0.cc");
+  auto *FromFriend =
+  FirstDeclMatcher().match(FromTU, FunctionPattern);
+  auto *FromNormal =
+  LastDeclMatcher().match(FromTU, FunctionPattern);
+  ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  ASSERT_FALSE(FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  ASSERT_FALSE(FromNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  ASSERT_TRUE(FromNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+
+  auto FromName = FromFriend->getDeclName();
+  auto *FromClass =
+  FirstDeclMatcher().match(FromTU, ClassPattern);
+  auto LookupRes = FromClass->noload_lookup(FromName);
+  ASSERT_EQ(LookupRes.size(), 0u);
+  LookupRes = FromTU->noload_lookup(FromName);
+  ASSERT_EQ(LookupRes.size(), 1u);
+
+  auto *ToFriend = cast(Import(FromFriend, Lang_CXX));
+  auto ToName = ToFriend->getDeclName();
+
+  TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  auto *ToClass = FirstDeclMatcher().match(ToTU, ClassPattern);
+  LookupRes = ToClass->noload_lookup(ToName);
+  EXPECT_EQ(LookupRes.size(), 0u);
+  LookupRes = ToTU->noload_lookup(ToName);
+  // Test is disabled because this result is 2.
+  EXPECT_EQ(LookupRes.size(), 1u);
+
+  ASSERT_EQ(DeclCounter().match(ToTU, FunctionPattern), 2u);
+  ToFriend = FirstDeclMatcher().match(ToTU, FunctionPattern);
+  auto *ToNormal = LastDeclMatcher().match(ToTU, FunctionPattern);
+  EXPECT_TRUE(ToFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  EXPECT_FALSE(ToFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  EXPECT_FALSE(ToNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  EXPECT_TRUE(ToNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+}
+
+TEST_P(ImportFriendFunctions, LookupWithProtoBefore) {
+  auto FunctionPattern = functionDecl(hasName("f"));
+  auto ClassPattern = cxxRecordDecl(hasName("X"));
+
+  TranslationUnitDecl *FromTU = getTuDecl(
+  "void f();"
+  "struct X { friend void f(); };",
+  Lang_CXX, "input0.cc");
+  auto *FromNormal =
+  FirstDeclMatcher().match(FromTU, FunctionPattern);
+  auto *FromFriend =
+  LastDeclMatcher().match(FromTU, FunctionPattern);
+  ASSERT_FALSE(FromNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  ASSERT_TRUE(FromNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
+  ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl:

[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Yes, I was thinking about it too. The reason why I chose Optional was that 
> ASTImporter clients don't use the error kind. If you have any plans on 
> changing this, Expected is a preferred approach.

Yes, we plan to submit a patch to LLDB too which would apply the use of 
Expected.

> If I understand you correctly, importNonNull and importNullable should work 
> with Expected pretty well.

Yes, that's right.

> Changing import*Into return result to Error can partially help in avoiding 
> usage of an unchecked result. These functions already guarantee that their 
> parameters are changed only if the internal import was successful.

Yes I agree, that `Error` can help, and I also agree that this help is just 
partial.
If we change `importNonNullInto` to has an `Error` return value

  template 
  LLVM_NODISCARD Error importNonNullInto(DeclTy *&ToD, Decl *FromD) {
if (auto Res = Importer.Import(FromD)) {
  ToD = cast(*Res);
  return make_error();
}
return Error::success();
  }

then on the call site, on the branch where whe handle the error we may make 
mistakes like this:

  CXXRecordDecl *ToTemplated = nullptr;
  if (Err = importNonNullInto(ToTemplated, FromTemplated)) {
foo(ToTemplated->hasDefinition()); // Undefined Behaviour!
return Err;
  }

If we do not use output parameters, only `Expected`, then this could not 
happen.
`Expected.get()` aborts if there was an error.


Repository:
  rC Clang

https://reviews.llvm.org/D50672



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.

>>> Oh, or could we do
>>> 
>>>   -D_LIBCPP_HIDE_FROM_ABI=
>>> 
>>> 
>>> and just get regular odr linkage for these functions?
>> 
>> No, you do need to use `_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` because of the 
>> issue described in 
>> http://lists.llvm.org/pipermail/llvm-dev/2018-July/124549.html. But yeah, 
>> Chromium could use this workaround.
> 
> Actually, scratch that, it does work. One can either use 
> `-D_LIBCPP_HIDE_FROM_ABI=_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` to restore the 
> old behavior, or `-D_LIBCPP_HIDE_FROM_ABI=` to get odr linkage.

I tried `-D_LIBCPP_HIDE_FROM_ABI=` but got lots of link errors (see 
https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c21).

`-D_LIBCPP_HIDE_FROM_ABI=_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` works nicely 
though.

I also tried applying your patch and verified that works for Chromium. If I 
understand correctly, without setting LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT, 
it restores the old behaviour of force inlining.

I think this makes sense for Chromium since it allows us to build without any 
special flags until we can get ODR linkage, and I think it also makes sense for 
the 7.0 branch to prevent regressing binary sizes for users of the release.

lgtm


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

[David Chisnall via Phabricator via cfe-commits]

theraven added inline comments.



Comment at: lib/AST/MicrosoftMangle.cpp:1886
   case BuiltinType::ObjCId:
-mangleArtificalTagType(TTK_Struct, "objc_object");
+mangleArtificalTagType(TTK_Struct, ".objc_object");
 break;

smeenai wrote:
> theraven wrote:
> > compnerd wrote:
> > > DHowett-MSFT wrote:
> > > > I'm interested in @smeenai's take on this, as well.
> > > Hmm, doesn't this break ObjC/ObjC++ interoperability?  I don't think that 
> > > we should be changing the decoration here for the MS ABI case.
> > No, this fixes ObjC++ interop.  Throwing an exception from a `@throw` and 
> > catching it with `catch` now works and we avoid the problem that a C++ 
> > compilation unit declares a `struct` that happens to have the same name as 
> > an Objective-C class (which is very common for wrapper code) may catch 
> > things of the wrong type.
> I've seen a lot of code which does something like this in a header
> 
> ```lang=cpp
> #ifdef __OBJC__
> @class I;
> #else
> struct I;
> #endif
> 
> void f(I *);
> ```
> 
> You want `f` to be mangled consistently across C++ and Objective-C++, 
> otherwise you'll get link errors if `f` is e.g. defined in a C++ TU and 
> referenced in an Objective-C++ TU. This was the case before your change and 
> isn't the case after your change, which is what @compnerd was pointing out. 
> They are mangled consistently in the Itanium ABI, and we want them to be 
> mangled consistently in the MS ABI as well.
> 
> Not wanting the catch mix-up is completely reasonable, of course, but it can 
> be done in a more targeted way. I'll put up a patch that undoes this part of 
> the change while still preventing incorrect catching.
With a non-fragile ABI, there is not guarantee that `struct I` and `@class I` 
have the same layout.  This is why `@defs` has gone away.  Any code that does 
this and treats `struct I` as anything other than an opaque type is broken.  No 
other platform supports throwing an Objective-C object and catching it as a 
struct, and this is an intentional design decision.  I would be strongly 
opposed to a change that intentionally supported this, because it is breaking 
correct code in order to make incorrect code do something that may or may not 
work.


Repository:
  rC Clang

https://reviews.llvm.org/D50144



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

[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

+1 to having a separate option for that. More generally, wouldn't we want to 
exit on more kinds errors in the future?
JSON parsing errors is something that popped up recently, but the option only 
for that looks too narrow. We might end up wanting more options like that in 
the future, e.g. `-exit-on-json-dispatch-error`, `-exit-on-parse-error`, ...

How about a more generic option that would force clangd to have non-zero error 
code whenever even a single error was emitted via `elog()`? 
I've checked the output of `./bin/llvm-lit -avv 
../tools/clang/tools/extra/test/clangd/ | grep '\(^E\|PASS\)'` and there seems 
to be a few things that emit errors spuriously and should be fixed for that to 
work:

1. `E[10:19:11.985] Failed to get status for RootPath clangd: No such file or 
directory`.

Could be fixed by specifying a valid rootPath in all clangd tests.

2. `E[10:19:12.013] Could not build a preamble for file /clangd-test/main.cpp`.

IIUC, this happens on empty preambles, the fix is to not emit an error in that 
case. In general, whenever preamble is not built, this is probably due to 
changes in some headers and that case should be considered valid input for 
clangd, so arguably this shouldn't be an error in the first place.-

3. Some tests actually test for invalid inputs, those should probably expect an 
error code from clangd that runs into those errors.

That doesn't look like a long list, so it shouldn't be too hard. What are your 
thoughts on a more generic option like that?




Comment at: JSONRPCDispatcher.cpp:366
+if (TestMode)
+  exit(EXIT_FAILURE);
   }

Alternative would be to run input parsing to completion, propagate if any error 
was encountered to main and exit with a predefined error code in that case.
Exiting prematurely might hide some important errors that we would otherwise 
catch even before seeing an error code from clangd, e.g. infinite loops in the 
input handling on invalid inputs, etc.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50785



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think I've mentioned this before, but I would like to discuss the possibility 
of adding a target hook(s) for some of these operations (conversions, 
arithmetic when it comes). Precisely matching the emitted saturation operation 
is virtually impossible for a target.




Comment at: lib/CodeGen/CGExprScalar.cpp:331
+  SourceLocation Loc);
+
   /// Emit a conversion from the specified complex type to the specified

What's the plan for the other conversions (int<->fix, float<->fix)? Functions 
for those as well?

What about `EmitScalarConversion`? If it cannot handle conversions of 
fixed-point values it should probably be made to assert, since it will likely 
mess up.



Comment at: lib/CodeGen/CGExprScalar.cpp:1017
+// Need to extend first before scaling up
+ResultWidth = SrcWidth + DstScale - SrcScale;
+llvm::Type *UpscaledTy = Builder.getIntNTy(ResultWidth);

It is definitely simpler to always do the 'upscale+resize/downscale, 
[saturate], resize' in the constant evaluation case, but when emitting IR it is 
not as efficient. There's no need to keep the upshifted bits if you aren't 
interested in saturation, so in the non-saturating case it is better to keep 
everything in the native type sizes with '[downscale], resize, [upscale]'. This 
is why there are a bunch of 'sext i32 to i33' in the test cases.

Perhaps something like
```
if !dst.saturating
  downscale if needed
  resize
  upscale if needed
  return 

old code here, minus the 'DstFPSema.isSaturated()' which is implied
```

It gives a bit of duplication (or at least similar code) but I think it's an 
important disambiguation to make.



Comment at: lib/CodeGen/CGExprScalar.cpp:1052
+} else if (IsSigned && !DstFPSema.isSigned()) {
+  Value *Zero =
+  ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth, 0));

`ConstantInt::getNullValue`?



Comment at: lib/Sema/Sema.cpp:537
+  case Type::STK_FixedPoint:
+llvm_unreachable("Unknown cast from FixedPoint to boolean");
   }

Will we want to support this?


Repository:
  rC Clang

https://reviews.llvm.org/D50616



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

looks good, a few nits.




Comment at: clangd/CodeComplete.cpp:742
+llvm::DenseMap FetchedDocs;
+if (Index) {
+  LookupRequest IndexRequest;

nit: do we want to log anything here? It may be useful for debug.



Comment at: clangd/index/Index.h:65
 public:
+  static llvm::Optional forDecl(const Decl &D);
+

We already have this similar function in clangd/AST.h.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50695: [clangd] Always use the latest preamble

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, we were previously getting inconsistent ASTs (i.e. using different 
preambles), depending on whether they were built in `TUScheduler::update` or in 
`TUScheduler::runWithAST` handlers.
Just a few NITs.




Comment at: clangd/TUScheduler.cpp:373
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  // Always use the NewPreamble.
+  LastBuiltPreamble = NewPreamble;

Maybe remove the comment? It does not seem to add any additional information on 
top of the code.



Comment at: unittests/clangd/XRefsTests.cpp:1058
+  // stale one.
+  Server.findDefinitions(
+  FooCpp, FooWithoutHeader.point(),

Maybe use `runFindDefinitions` instead of the two `Server.findDefinitions()` + 
`Server.blockUntilIdleForTest()` calls?



Comment at: unittests/clangd/XRefsTests.cpp:1061
+  [&](llvm::Expected> Loc) {
+ASSERT_TRUE(bool(Loc));
+EXPECT_THAT(*Loc,

NIT: just use `cantFail(std::move(Loc))` to get the underlying vector and 
remove `ASSERT_TRUE`.
This gives equivalent results (crashes tests with `assert` failure internally 
in LLVM) with a simpler syntax.



Comment at: unittests/clangd/XRefsTests.cpp:1073
+  // Use the AST being built in above request.
+  Server.findDefinitions(
+  FooCpp, FooWithoutHeader.point(),

Same here: maybe use `runFindDefinitions`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695



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

[PATCH] D50691: [CMake] Fix the LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY option

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Merged in r339850.


Repository:
  rL LLVM

https://reviews.llvm.org/D50691



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

[libcxx] r339850 - Merging r339697:

[Hans Wennborg via cfe-commits]

Author: hans
Date: Thu Aug 16 02:03:32 2018
New Revision: 339850

URL: http://llvm.org/viewvc/llvm-project?rev=339850&view=rev
Log:
Merging r339697:

r339697 | mstorsjo | 2018-08-14 19:33:10 +0200 (Tue, 14 Aug 2018) | 8 lines

[CMake] Fix the LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY option

This option should be available if LIBCXX_ENABLE_SHARED is enabled,
not LIBCXX_ENABLE_STATIC.

This fixes a typo from SVN r337814.

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


Modified:
libcxx/branches/release_70/   (props changed)
libcxx/branches/release_70/CMakeLists.txt

Propchange: libcxx/branches/release_70/
--
--- svn:mergeinfo (original)
+++ svn:mergeinfo Thu Aug 16 02:03:32 2018
@@ -1,2 +1,2 @@
 /libcxx/branches/apple:136569-137939
-/libcxx/trunk:339431
+/libcxx/trunk:339431,339697

Modified: libcxx/branches/release_70/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/branches/release_70/CMakeLists.txt?rev=339850&r1=339849&r2=339850&view=diff
==
--- libcxx/branches/release_70/CMakeLists.txt (original)
+++ libcxx/branches/release_70/CMakeLists.txt Thu Aug 16 02:03:32 2018
@@ -175,7 +175,7 @@ cmake_dependent_option(LIBCXX_STATICALLY
 
 cmake_dependent_option(LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY
   "Statically link the ABI library to shared library" ON
-  "LIBCXX_ENABLE_STATIC_ABI_LIBRARY;LIBCXX_ENABLE_STATIC" OFF)
+  "LIBCXX_ENABLE_STATIC_ABI_LIBRARY;LIBCXX_ENABLE_SHARED" OFF)
 
 # Generate and install a linker script inplace of libc++.so. The linker script
 # will link libc++ to the correct ABI library. This option is on by default


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

Re: [libcxx] r339697 - [CMake] Fix the LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY option

[Hans Wennborg via cfe-commits]

Merged to 7.0 in r339850.

On Tue, Aug 14, 2018 at 7:33 PM, Martin Storsjo via cfe-commits
 wrote:
> Author: mstorsjo
> Date: Tue Aug 14 10:33:10 2018
> New Revision: 339697
>
> URL: http://llvm.org/viewvc/llvm-project?rev=339697&view=rev
> Log:
> [CMake] Fix the LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY option
>
> This option should be available if LIBCXX_ENABLE_SHARED is enabled,
> not LIBCXX_ENABLE_STATIC.
>
> This fixes a typo from SVN r337814.
>
> Differential Revision: https://reviews.llvm.org/D50691
>
> Modified:
> libcxx/trunk/CMakeLists.txt
>
> Modified: libcxx/trunk/CMakeLists.txt
> URL: 
> http://llvm.org/viewvc/llvm-project/libcxx/trunk/CMakeLists.txt?rev=339697&r1=339696&r2=339697&view=diff
> ==
> --- libcxx/trunk/CMakeLists.txt (original)
> +++ libcxx/trunk/CMakeLists.txt Tue Aug 14 10:33:10 2018
> @@ -175,7 +175,7 @@ cmake_dependent_option(LIBCXX_STATICALLY
>
>  cmake_dependent_option(LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY
>"Statically link the ABI library to shared library" ON
> -  "LIBCXX_ENABLE_STATIC_ABI_LIBRARY;LIBCXX_ENABLE_STATIC" OFF)
> +  "LIBCXX_ENABLE_STATIC_ABI_LIBRARY;LIBCXX_ENABLE_SHARED" OFF)
>
>  # Generate and install a linker script inplace of libc++.so. The linker 
> script
>  # will link libc++ to the correct ABI library. This option is on by default
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Currently we only add parantheses to the functions if snippets are
enabled, which also inserts snippets for parameters into parantheses. Adding a
new option to put only parantheses. Also it moves the cursor within parantheses
or at the end of them by looking at whether completion item has any parameters
or not. Still requires snippets support on the client side.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835

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


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1590,6 +1590,44 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(CompletionTest, RenderWithCursorRelocation) {
+  CodeCompleteOptions Opts;
+  Opts.EnableCursorRelocation = true;
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "()";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x()");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "(${0:bool})";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -81,6 +81,12 @@
   /// Include completions that require small corrections, e.g. change '.' to
   /// '->' on member access etc.
   bool IncludeFixIts = false;
+
+  /// Enables cursor to be moved around according to completions needs even 
when
+  /// snippets are disabled. For example selecting a function with parameters 
as
+  /// completion item moves the cursor within the parameters, whereas without
+  /// parameters moves to after closing parantheses.
+  bool EnableCursorRelocation = false;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ 
API.
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -1373,11 +1373,21 @@
   }
   if (Opts.EnableSnippets)
 LSP.textEdit->newText += SnippetSuffix;
+  // Check if we have cursor relocation enabled and completing a function.
+  else if (SnippetSuffix.size() && Opts.EnableCursorRelocation) {
+// Check whether function has any parameters or not.
+if (SnippetSuffix.size() > 2)
+  LSP.textEdit->newText += "(${0})";
+else
+  LSP.textEdit->newText += "()";
+  }
+
   // FIXME(kadircet): Do not even fill insertText after making sure textEdit is
   // compatible with most of the editors.
   LSP.insertText = LSP.textEdit->newText;
-  LSP.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
- : InsertTextFormat::PlainText;
+  LSP.insertTextFormat = Opts.EnableSnippets || Opts.EnableCursorRelocation
+ ? InsertTextFormat::Snippet
+ : InsertTextFormat::PlainText;
   if (HeaderInsertion)
 LSP.additionalTextEdits.push_back(*HeaderInsertion);
   return LSP;


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1590,6 +1590,44 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(CompletionTest, RenderWithCursorRelocation) {
+  CodeCompleteOptions Opts;
+  Opts.EnableCursorRelocation = true;
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "()";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x()");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "(${0:bool})";
+
+auto R = C.ren

[PATCH] D50695: [clangd] Always use the latest preamble

[Haojian Wu via Phabricator via cfe-commits]

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

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695

Files:
  clangd/TUScheduler.cpp
  unittests/clangd/XRefsTests.cpp


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,51 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the 
preamble.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+  ElementsAre(Location{FooHUri, FooHeader.range()}));
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above request.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -370,8 +370,7 @@
 bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
 {
   std::lock_guard Lock(Mutex);
-  if (NewPreamble)
-LastBuiltPreamble = NewPreamble;
+  LastBuiltPreamble = NewPreamble;
 }
 // Before doing the expensive AST reparse, we want to release our reference
 // to the old preamble, so it can be freed if there are no other references


Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,51 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double[[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+  ElementsAre(Location{FooHUri, FooHeader.range()}));
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  EXPECT_THAT(
+  cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+  ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above

Re: r339603 - [OPENMP] Fix emission of the loop doacross constructs.

[Hans Wennborg via cfe-commits]

I've gone ahead and merged it in r339851.

On Wed, Aug 15, 2018 at 3:23 PM, Alexey Bataev  wrote:
> I think it would be good to backport it. Could you do that, Jonas?
>
> -
> Best regards,
> Alexey Bataev
>
> 15.08.2018 5:02, Jonas Hahnfeld via cfe-commits пишет:
>
> Alexey, Hans,
>
> does it make sense to backport for 7.0 as it fixes PR37580?
>
> Thanks,
> Jonas
>
> On 2018-08-13 21:04, Alexey Bataev via cfe-commits wrote:
>
> Author: abataev
> Date: Mon Aug 13 12:04:24 2018
> New Revision: 339603
>
> URL: http://llvm.org/viewvc/llvm-project?rev=339603&view=rev
> Log:
> [OPENMP] Fix emission of the loop doacross constructs.
>
> The number of loops associated with the OpenMP loop constructs should
> not be considered as the number loops to collapse.
>
> Modified:
> cfe/trunk/include/clang/AST/OpenMPClause.h
> cfe/trunk/lib/AST/OpenMPClause.cpp
> cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
> cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
> cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
> cfe/trunk/lib/Sema/SemaOpenMP.cpp
> cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
> cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
> cfe/trunk/test/OpenMP/ordered_doacross_codegen.c
> cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp
> cfe/trunk/test/OpenMP/parallel_for_simd_ast_print.cpp
>
> Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=339603&r1=339602&r2=339603&view=diff
> ==
> --- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
> +++ cfe/trunk/include/clang/AST/OpenMPClause.h Mon Aug 13 12:04:24 2018
> @@ -930,8 +930,11 @@ public:
>  /// \endcode
>  /// In this example directive '#pragma omp for' has 'ordered' clause with
>  /// parameter 2.
> -class OMPOrderedClause : public OMPClause {
> +class OMPOrderedClause final
> +: public OMPClause,
> +  private llvm::TrailingObjects {
>friend class OMPClauseReader;
> +  friend TrailingObjects;
>
>/// Location of '('.
>SourceLocation LParenLoc;
> @@ -939,6 +942,26 @@ class OMPOrderedClause : public OMPClaus
>/// Number of for-loops.
>Stmt *NumForLoops = nullptr;
>
> +  /// Real number of loops.
> +  unsigned NumberOfLoops = 0;
> +
> +  /// Build 'ordered' clause.
> +  ///
> +  /// \param Num Expression, possibly associated with this clause.
> +  /// \param NumLoops Number of loops, associated with this clause.
> +  /// \param StartLoc Starting location of the clause.
> +  /// \param LParenLoc Location of '('.
> +  /// \param EndLoc Ending location of the clause.
> +  OMPOrderedClause(Expr *Num, unsigned NumLoops, SourceLocation StartLoc,
> +   SourceLocation LParenLoc, SourceLocation EndLoc)
> +  : OMPClause(OMPC_ordered, StartLoc, EndLoc), LParenLoc(LParenLoc),
> +NumForLoops(Num), NumberOfLoops(NumLoops) {}
> +
> +  /// Build an empty clause.
> +  explicit OMPOrderedClause(unsigned NumLoops)
> +  : OMPClause(OMPC_ordered, SourceLocation(), SourceLocation()),
> +NumberOfLoops(NumLoops) {}
> +
>/// Set the number of associated for-loops.
>void setNumForLoops(Expr *Num) { NumForLoops = Num; }
>
> @@ -946,17 +969,17 @@ public:
>/// Build 'ordered' clause.
>///
>/// \param Num Expression, possibly associated with this clause.
> +  /// \param NumLoops Number of loops, associated with this clause.
>/// \param StartLoc Starting location of the clause.
>/// \param LParenLoc Location of '('.
>/// \param EndLoc Ending location of the clause.
> -  OMPOrderedClause(Expr *Num, SourceLocation StartLoc,
> -SourceLocation LParenLoc, SourceLocation EndLoc)
> -  : OMPClause(OMPC_ordered, StartLoc, EndLoc), LParenLoc(LParenLoc),
> -NumForLoops(Num) {}
> +  static OMPOrderedClause *Create(const ASTContext &C, Expr *Num,
> +  unsigned NumLoops, SourceLocation
> StartLoc,
> +  SourceLocation LParenLoc,
> +  SourceLocation EndLoc);
>
>/// Build an empty clause.
> -  explicit OMPOrderedClause()
> -  : OMPClause(OMPC_ordered, SourceLocation(), SourceLocation()) {}
> +  static OMPOrderedClause* CreateEmpty(const ASTContext &C, unsigned
> NumLoops);
>
>/// Sets the location of '('.
>void setLParenLoc(SourceLocation Loc) { LParenLoc = Loc; }
> @@ -967,6 +990,17 @@ public:
>/// Return the number of associated for-loops.
>Expr *getNumForLoops() const { return cast_or_null(NumForLoops); }
>
> +  /// Set number of iterations for the specified loop.
> +  void setLoopNumIterations(unsigned NumLoop, Expr *NumIterations);
> +  /// Get number of iterations for all the loops.
> +  ArrayRef getLoopNumIterations() const;
> +
> +  /// Set loop counter for the specified loop.
> +  void setLoopCounter(unsigned NumLoop, Expr *Counter);
> +  /// Get loops co

Re: r339603 - [OPENMP] Fix emission of the loop doacross constructs.

[Jonas Hahnfeld via cfe-commits]

Thanks Hans!

On 2018-08-16 11:35, Hans Wennborg wrote:

I've gone ahead and merged it in r339851.

On Wed, Aug 15, 2018 at 3:23 PM, Alexey Bataev  
wrote:

I think it would be good to backport it. Could you do that, Jonas?

-
Best regards,
Alexey Bataev

15.08.2018 5:02, Jonas Hahnfeld via cfe-commits пишет:

Alexey, Hans,

does it make sense to backport for 7.0 as it fixes PR37580?

Thanks,
Jonas

On 2018-08-13 21:04, Alexey Bataev via cfe-commits wrote:

Author: abataev
Date: Mon Aug 13 12:04:24 2018
New Revision: 339603

URL: http://llvm.org/viewvc/llvm-project?rev=339603&view=rev
Log:
[OPENMP] Fix emission of the loop doacross constructs.

The number of loops associated with the OpenMP loop constructs should
not be considered as the number loops to collapse.

Modified:
cfe/trunk/include/clang/AST/OpenMPClause.h
cfe/trunk/lib/AST/OpenMPClause.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
cfe/trunk/test/OpenMP/ordered_doacross_codegen.c
cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp
cfe/trunk/test/OpenMP/parallel_for_simd_ast_print.cpp

Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=339603&r1=339602&r2=339603&view=diff
==
--- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
+++ cfe/trunk/include/clang/AST/OpenMPClause.h Mon Aug 13 12:04:24 
2018

@@ -930,8 +930,11 @@ public:
 /// \endcode
 /// In this example directive '#pragma omp for' has 'ordered' clause 
with

 /// parameter 2.
-class OMPOrderedClause : public OMPClause {
+class OMPOrderedClause final
+: public OMPClause,
+  private llvm::TrailingObjects {
   friend class OMPClauseReader;
+  friend TrailingObjects;

   /// Location of '('.
   SourceLocation LParenLoc;
@@ -939,6 +942,26 @@ class OMPOrderedClause : public OMPClaus
   /// Number of for-loops.
   Stmt *NumForLoops = nullptr;

+  /// Real number of loops.
+  unsigned NumberOfLoops = 0;
+
+  /// Build 'ordered' clause.
+  ///
+  /// \param Num Expression, possibly associated with this clause.
+  /// \param NumLoops Number of loops, associated with this clause.
+  /// \param StartLoc Starting location of the clause.
+  /// \param LParenLoc Location of '('.
+  /// \param EndLoc Ending location of the clause.
+  OMPOrderedClause(Expr *Num, unsigned NumLoops, SourceLocation 
StartLoc,

+   SourceLocation LParenLoc, SourceLocation EndLoc)
+  : OMPClause(OMPC_ordered, StartLoc, EndLoc), 
LParenLoc(LParenLoc),

+NumForLoops(Num), NumberOfLoops(NumLoops) {}
+
+  /// Build an empty clause.
+  explicit OMPOrderedClause(unsigned NumLoops)
+  : OMPClause(OMPC_ordered, SourceLocation(), SourceLocation()),
+NumberOfLoops(NumLoops) {}
+
   /// Set the number of associated for-loops.
   void setNumForLoops(Expr *Num) { NumForLoops = Num; }

@@ -946,17 +969,17 @@ public:
   /// Build 'ordered' clause.
   ///
   /// \param Num Expression, possibly associated with this clause.
+  /// \param NumLoops Number of loops, associated with this clause.
   /// \param StartLoc Starting location of the clause.
   /// \param LParenLoc Location of '('.
   /// \param EndLoc Ending location of the clause.
-  OMPOrderedClause(Expr *Num, SourceLocation StartLoc,
-SourceLocation LParenLoc, SourceLocation EndLoc)
-  : OMPClause(OMPC_ordered, StartLoc, EndLoc), 
LParenLoc(LParenLoc),

-NumForLoops(Num) {}
+  static OMPOrderedClause *Create(const ASTContext &C, Expr *Num,
+  unsigned NumLoops, SourceLocation
StartLoc,
+  SourceLocation LParenLoc,
+  SourceLocation EndLoc);

   /// Build an empty clause.
-  explicit OMPOrderedClause()
-  : OMPClause(OMPC_ordered, SourceLocation(), SourceLocation()) 
{}

+  static OMPOrderedClause* CreateEmpty(const ASTContext &C, unsigned
NumLoops);

   /// Sets the location of '('.
   void setLParenLoc(SourceLocation Loc) { LParenLoc = Loc; }
@@ -967,6 +990,17 @@ public:
   /// Return the number of associated for-loops.
   Expr *getNumForLoops() const { return 
cast_or_null(NumForLoops); }


+  /// Set number of iterations for the specified loop.
+  void setLoopNumIterations(unsigned NumLoop, Expr *NumIterations);
+  /// Get number of iterations for all the loops.
+  ArrayRef getLoopNumIterations() const;
+
+  /// Set loop counter for the specified loop.
+  void setLoopCounter(unsigned NumLoop, Expr *Counter);
+  /// Get loops counter for the specified loop.
+  Expr *getLoopCunter(unsigned NumLoop);
+  const Expr *getLoopCunter(unsigned NumLoop) const;
+
   child_range children() { ret

Re: r339603 - [OPENMP] Fix emission of the loop doacross constructs.

[Alexey Bataev via cfe-commits]

Hans, thanks a lot. 

Best regards,
Alexey Bataev

> 16 авг. 2018 г., в 5:36, Hans Wennborg  написал(а):
> 
> I've gone ahead and merged it in r339851.
> 
>> On Wed, Aug 15, 2018 at 3:23 PM, Alexey Bataev  wrote:
>> I think it would be good to backport it. Could you do that, Jonas?
>> 
>> -
>> Best regards,
>> Alexey Bataev
>> 
>> 15.08.2018 5:02, Jonas Hahnfeld via cfe-commits пишет:
>> 
>> Alexey, Hans,
>> 
>> does it make sense to backport for 7.0 as it fixes PR37580?
>> 
>> Thanks,
>> Jonas
>> 
>> On 2018-08-13 21:04, Alexey Bataev via cfe-commits wrote:
>> 
>> Author: abataev
>> Date: Mon Aug 13 12:04:24 2018
>> New Revision: 339603
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=339603&view=rev
>> Log:
>> [OPENMP] Fix emission of the loop doacross constructs.
>> 
>> The number of loops associated with the OpenMP loop constructs should
>> not be considered as the number loops to collapse.
>> 
>> Modified:
>>cfe/trunk/include/clang/AST/OpenMPClause.h
>>cfe/trunk/lib/AST/OpenMPClause.cpp
>>cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
>>cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
>>cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
>>cfe/trunk/lib/Sema/SemaOpenMP.cpp
>>cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
>>cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
>>cfe/trunk/test/OpenMP/ordered_doacross_codegen.c
>>cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp
>>cfe/trunk/test/OpenMP/parallel_for_simd_ast_print.cpp
>> 
>> Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=339603&r1=339602&r2=339603&view=diff
>> ==
>> --- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
>> +++ cfe/trunk/include/clang/AST/OpenMPClause.h Mon Aug 13 12:04:24 2018
>> @@ -930,8 +930,11 @@ public:
>> /// \endcode
>> /// In this example directive '#pragma omp for' has 'ordered' clause with
>> /// parameter 2.
>> -class OMPOrderedClause : public OMPClause {
>> +class OMPOrderedClause final
>> +: public OMPClause,
>> +  private llvm::TrailingObjects {
>>   friend class OMPClauseReader;
>> +  friend TrailingObjects;
>> 
>>   /// Location of '('.
>>   SourceLocation LParenLoc;
>> @@ -939,6 +942,26 @@ class OMPOrderedClause : public OMPClaus
>>   /// Number of for-loops.
>>   Stmt *NumForLoops = nullptr;
>> 
>> +  /// Real number of loops.
>> +  unsigned NumberOfLoops = 0;
>> +
>> +  /// Build 'ordered' clause.
>> +  ///
>> +  /// \param Num Expression, possibly associated with this clause.
>> +  /// \param NumLoops Number of loops, associated with this clause.
>> +  /// \param StartLoc Starting location of the clause.
>> +  /// \param LParenLoc Location of '('.
>> +  /// \param EndLoc Ending location of the clause.
>> +  OMPOrderedClause(Expr *Num, unsigned NumLoops, SourceLocation StartLoc,
>> +   SourceLocation LParenLoc, SourceLocation EndLoc)
>> +  : OMPClause(OMPC_ordered, StartLoc, EndLoc), LParenLoc(LParenLoc),
>> +NumForLoops(Num), NumberOfLoops(NumLoops) {}
>> +
>> +  /// Build an empty clause.
>> +  explicit OMPOrderedClause(unsigned NumLoops)
>> +  : OMPClause(OMPC_ordered, SourceLocation(), SourceLocation()),
>> +NumberOfLoops(NumLoops) {}
>> +
>>   /// Set the number of associated for-loops.
>>   void setNumForLoops(Expr *Num) { NumForLoops = Num; }
>> 
>> @@ -946,17 +969,17 @@ public:
>>   /// Build 'ordered' clause.
>>   ///
>>   /// \param Num Expression, possibly associated with this clause.
>> +  /// \param NumLoops Number of loops, associated with this clause.
>>   /// \param StartLoc Starting location of the clause.
>>   /// \param LParenLoc Location of '('.
>>   /// \param EndLoc Ending location of the clause.
>> -  OMPOrderedClause(Expr *Num, SourceLocation StartLoc,
>> -SourceLocation LParenLoc, SourceLocation EndLoc)
>> -  : OMPClause(OMPC_ordered, StartLoc, EndLoc), LParenLoc(LParenLoc),
>> -NumForLoops(Num) {}
>> +  static OMPOrderedClause *Create(const ASTContext &C, Expr *Num,
>> +  unsigned NumLoops, SourceLocation
>> StartLoc,
>> +  SourceLocation LParenLoc,
>> +  SourceLocation EndLoc);
>> 
>>   /// Build an empty clause.
>> -  explicit OMPOrderedClause()
>> -  : OMPClause(OMPC_ordered, SourceLocation(), SourceLocation()) {}
>> +  static OMPOrderedClause* CreateEmpty(const ASTContext &C, unsigned
>> NumLoops);
>> 
>>   /// Sets the location of '('.
>>   void setLParenLoc(SourceLocation Loc) { LParenLoc = Loc; }
>> @@ -967,6 +990,17 @@ public:
>>   /// Return the number of associated for-loops.
>>   Expr *getNumForLoops() const { return cast_or_null(NumForLoops); }
>> 
>> +  /// Set number of iterations for the specified loop.
>> +  void setLoopNumIterations(unsigned NumLoop, Expr *NumIterations);
>> +  /// Ge

[PATCH] D49793: [AArch64] - return address signing

[Luke Cheeseman via Phabricator via cfe-commits]

LukeCheeseman added a comment.

ping


https://reviews.llvm.org/D49793



___
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 160986.
kadircet added a comment.

- Made TaskHandle move-only. Since it is costly and most likely unnecessary to 
copy it other than to move it into Context.
- Provided an explicit clone method for copying.


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,68 @@
+#include "Cancellation.h"
+#include "Context.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 = TaskHandle::create();
+WithContext ContextWithCancellation(
+setCurrentCancellationToken(TH.clone()));
+EXPECT_FALSE(isCancelled());
+TH.cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_FALSE(isCancelled());
+}
+
+TEST(CancellationTest, TaskHandleTestHandleDiesContextLives) {
+  llvm::Optional ContextWithCancellation;
+  {
+auto CancellableTaskHandle = TaskHandle::create();
+ContextWithCancellation.emplace(
+setCurrentCancellationToken(CancellableTaskHandle.clone()));
+EXPECT_FALSE(isCancelled());
+CancellableTaskHandle.cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_TRUE(isCancelled());
+  ContextWithCancellation.reset();
+  EXPECT_FALSE(isCancelled());
+}
+
+TEST(CancellationTest, TaskHandleContextDiesHandleLives) {
+  {
+auto CancellableTaskHandle = TaskHandle::create();
+{
+  WithContext ContextWithCancellation(
+  setCurrentCancellationToken(CancellableTaskHandle.clone()));
+  EXPECT_FALSE(isCancelled());
+  CancellableTaskHandle.cancel();
+  EXPECT_TRUE(isCancelled());
+}
+  }
+  EXPECT_FALSE(isCancelled());
+}
+
+TEST(CancellationTest, CancellationToken) {
+  auto CancellableTaskHandle = TaskHandle::create();
+  WithContext ContextWithCancellation(
+  setCurrentCancellationToken(CancellableTaskHandle.clone()));
+  auto CT = isCancelled();
+  EXPECT_FALSE(CT);
+  CancellableTaskHandle.cancel();
+  EXPECT_TRUE(CT);
+}
+
+} // 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,13 @@
 llvm::json::Value toJSON(const DocumentHighlight &DH);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
+struct CancelParams {
+  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 &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -615,5 +615,30 @@
  O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
+json::Value toJSON(const CancelParams &CP) {
+  return json::Object{{"id", CP.ID}};
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostr

[libcxx] r339852 - Merging r339794:

[Hans Wennborg via cfe-commits]

Author: hans
Date: Thu Aug 16 02:45:10 2018
New Revision: 339852

URL: http://llvm.org/viewvc/llvm-project?rev=339852&view=rev
Log:
Merging r339794:

r339794 | dim | 2018-08-15 19:30:32 +0200 (Wed, 15 Aug 2018) | 8 lines

For FreeBSD, don't define _M in nasty_macros.hpp

Because FreeBSD uses _M in its , and it is hard to avoid
including that header, only define _M to NASTY_MACRO for other operating
systems.  This fixes almost 2000 unexpected test failures.

Discussed with Eric Fiselier.



Modified:
libcxx/branches/release_70/   (props changed)
libcxx/branches/release_70/test/support/nasty_macros.hpp

Propchange: libcxx/branches/release_70/
--
--- svn:mergeinfo (original)
+++ svn:mergeinfo Thu Aug 16 02:45:10 2018
@@ -1,2 +1,2 @@
 /libcxx/branches/apple:136569-137939
-/libcxx/trunk:339431,339697
+/libcxx/trunk:339431,339697,339794

Modified: libcxx/branches/release_70/test/support/nasty_macros.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/branches/release_70/test/support/nasty_macros.hpp?rev=339852&r1=339851&r2=339852&view=diff
==
--- libcxx/branches/release_70/test/support/nasty_macros.hpp (original)
+++ libcxx/branches/release_70/test/support/nasty_macros.hpp Thu Aug 16 
02:45:10 2018
@@ -22,7 +22,11 @@
 #define _J NASTY_MACRO
 #define _K NASTY_MACRO
 #define _L NASTY_MACRO
+// Because FreeBSD uses _M in its , and it is hard to avoid
+// including that header, only define _M for other operating systems.
+#ifndef __FreeBSD__
 #define _M NASTY_MACRO
+#endif
 #define _N NASTY_MACRO
 #define _O NASTY_MACRO
 #define _P NASTY_MACRO


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

Re: [libcxx] r339794 - For FreeBSD, don't define _M in nasty_macros.hpp

[Hans Wennborg via cfe-commits]

Merged to 7.0 in r339852.

On Wed, Aug 15, 2018 at 7:30 PM, Dimitry Andric via cfe-commits
 wrote:
> Author: dim
> Date: Wed Aug 15 10:30:32 2018
> New Revision: 339794
>
> URL: http://llvm.org/viewvc/llvm-project?rev=339794&view=rev
> Log:
> For FreeBSD, don't define _M in nasty_macros.hpp
>
> Because FreeBSD uses _M in its , and it is hard to avoid
> including that header, only define _M to NASTY_MACRO for other operating
> systems.  This fixes almost 2000 unexpected test failures.
>
> Discussed with Eric Fiselier.
>
> Modified:
> libcxx/trunk/test/support/nasty_macros.hpp
>
> Modified: libcxx/trunk/test/support/nasty_macros.hpp
> URL: 
> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/support/nasty_macros.hpp?rev=339794&r1=339793&r2=339794&view=diff
> ==
> --- libcxx/trunk/test/support/nasty_macros.hpp (original)
> +++ libcxx/trunk/test/support/nasty_macros.hpp Wed Aug 15 10:30:32 2018
> @@ -22,7 +22,11 @@
>  #define _J NASTY_MACRO
>  #define _K NASTY_MACRO
>  #define _L NASTY_MACRO
> +// Because FreeBSD uses _M in its , and it is hard to avoid
> +// including that header, only define _M for other operating systems.
> +#ifndef __FreeBSD__
>  #define _M NASTY_MACRO
> +#endif
>  #define _N NASTY_MACRO
>  #define _O NASTY_MACRO
>  #define _P NASTY_MACRO
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@shuaiwang i tried to apply this and check the clang-tidy part again, but it 
does not compile (log attached).
I update clang to master, did you add a matcher or something like this?

F6950472: error.log 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



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

[PATCH] D50662: Add dump() method for SourceRange

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

This seems like a reasonable addition. Maybe it could also provide `print` and 
`printToString` to more closely match the `SourceLocation` API?


Repository:
  rC Clang

https://reviews.llvm.org/D50662



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

[libcxx] r339854 - Merging r339743:

[Hans Wennborg via cfe-commits]

Author: hans
Date: Thu Aug 16 02:52:16 2018
New Revision: 339854

URL: http://llvm.org/viewvc/llvm-project?rev=339854&view=rev
Log:
Merging r339743:

r339743 | ldionne | 2018-08-15 02:30:03 +0200 (Wed, 15 Aug 2018) | 14 lines

[libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

Summary:
Since r338934, Clang emits an error when aligned allocation functions are
used in conjunction with a system libc++ dylib that does not support those
functions. This causes some tests to fail when testing against older libc++
dylibs. This commit marks those tests as UNSUPPORTED, and also documents the
various reasons for the tests being unsupported.

Reviewers: vsapsai, EricWF

Subscribers: christof, dexonsmith, cfe-commits, mclow.lists, EricWF

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


Added:

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp
  - copied unchanged from r339743, 
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp
  - copied unchanged from r339743, 
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp
  - copied unchanged from r339743, 
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp
  - copied unchanged from r339743, 
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
  - copied unchanged from r339743, 
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
  - copied unchanged from r339743, 
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
Removed:

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.fail.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.fail.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
Modified:
libcxx/branches/release_70/   (props changed)

libcxx/branches/release_70/test/libcxx/memory/aligned_allocation_macro.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp

libcxx/branches/release_70/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp

Propchange: libcxx/branches/release_70/
--
--- svn:mergeinfo (original)
+++ svn:mergeinfo Thu Aug 16 02:52:16 2018
@@ -1,2 +1,2 @@
 /libcxx/branches/apple:136569-137939
-/libcxx/trunk:339431,339697,339794
+/libcxx/trunk:339431,339697,339743,339794

Modified: 
libcxx/branches/release_70/test/libcxx/me

Re: [libcxx] r339743 - [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Hans Wennborg via cfe-commits]

Merged to 7.0 in r339854.

On Wed, Aug 15, 2018 at 2:30 AM, Louis Dionne via cfe-commits
 wrote:
> Author: ldionne
> Date: Tue Aug 14 17:30:03 2018
> New Revision: 339743
>
> URL: http://llvm.org/viewvc/llvm-project?rev=339743&view=rev
> Log:
> [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions
>
> Summary:
> Since r338934, Clang emits an error when aligned allocation functions are
> used in conjunction with a system libc++ dylib that does not support those
> functions. This causes some tests to fail when testing against older libc++
> dylibs. This commit marks those tests as UNSUPPORTED, and also documents the
> various reasons for the tests being unsupported.
>
> Reviewers: vsapsai, EricWF
>
> Subscribers: christof, dexonsmith, cfe-commits, mclow.lists, EricWF
>
> Differential Revision: https://reviews.llvm.org/D50341
>
> Added:
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
> Removed:
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.fail.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.fail.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
> Modified:
> libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
> 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
>
> Modified: libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp?rev=339743&r1=339742&r2=339743&view=diff
> ==
> --- libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp 
> (original)
> +++ libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp Tue Aug 
> 14 17:30:03 2018
> @@ -8,12 +8,14 @@
>  
> //===--===//
>
>  // UNSUPPORTED: c++98, c++03, c++11, c++14
> -// XFAIL: with_system_cxx_lib=macosx10.12
> -// XFAIL: with_system_cxx_lib=macosx10.11
> -// XFAIL: with_system_cxx_lib=macosx10.10
> -// XFAIL: with_system_cxx_lib=macosx10.9
> -// XFAIL: with_system_cxx_lib=macosx10.8
> -// XFAIL: with_system_cxx_lib=macosx10.7
> +
> +// aligned allocation functions are not provided prior to macosx10.13
> +// XFAIL: macosx10.12
> +// XFAIL: macosx10.11
> +// XFAIL: macosx10.10
> +// XFAIL: macosx10.9
> +// XFAIL: macosx10.8
> +// XFAIL: macosx10.7
>
>  #include 
>
>
> Modified: 
> libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp?rev=339743&r1=339742&r2=339743&view=diff
> ==
> --- 
> libcxx/trunk/test/std/langua

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly LSP-specific comments and comments about making TaskHandle thread-safe.

I'm also starting to wonder if the scope of this patch is too big, we could 
potentially split it into three separate bits:

1. Generic cancellation API, i.e. `Cancellation.h` and unit-tests of the API,
2. ClangdServer using the cancellation API.
3. LSP-specific bits, i.e. `cancelRequest` etc.

Not sure if it's worth splitting the last two, but splitting out the 
Cancellation API in a separate change could make review simpler.
But also happy to review the whole thing and land after it's done, feel free to 
pick whatever suits you best.




Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key>> CancellationTokenKey;
+} // namespace

kadircet wrote:
> ilya-biryukov wrote:
> > Having a `shared_ptr` key in the Context can cause data races (e.g. if we 
> > copy it concurrently from multiple threads).
> > I suggest we make `CancellationToken` move-only (i.e. disallow copies) and 
> > return `const CancellationToken&` when getting it from the context.
> As talked offline, copying std::shared_ptr is thread safe.
My mistake, thanks for explaining this one to me.
Copying of `CancellationToken`s LG!



Comment at: clangd/Cancellation.h:86
+/// Enables async tasks to check for cancellation signal, contains a read only
+/// token cached from context. Can be created with clangd::isCancelled. Totally
+/// thread-safe multiple threads can check for cancellation on the same token.

NIT: Maybe replace 'Totally thread-safe' with just `Thread-safe`? The latter 
conveys the same information.



Comment at: clangd/Cancellation.h:90
+public:
+  bool isCancelled() const { return Token ? static_cast(*Token) : false; 
}
+  operator bool() const { return isCancelled(); }

NIT: maybe use `Token->load()` instead of `static_cast(Token)`? Arguably, 
looks more concise.



Comment at: clangd/Cancellation.h:92
+  operator bool() const { return isCancelled(); }
+  friend CancellationToken isCancelled();
+

It's a bit confusing that this name clashes with a member function.
We seem to optimize for making this work anywhere:
```
  if (isCancelled())  // <-- get token from ctx, then use implicit conversion 
to check if it was cancelled
return llvm::makeError();
```

Which is probably a nice pattern. But we could have two functions that do the 
same thing with less magic (i.e. implicit conversions) involved at the call 
sites:
```
CancellationToken getCurrentCancellation();
void setCurrentCancellation(CancellationToken CT);

inline bool isCancelled() { return getCurrentCancellation().isCancelled(); }
```
Would allow to get rid of implicit conversion and friend function with the same 
signature as member. Would also make cases where we actually want to stash the 
token somewhere cleaner, e.g. instead of `CancellationToken CT = 
isCancelled()`, we'll have `CancellationToken CT = getCurrentCancellation()`.
WDYT?



Comment at: clangd/Cancellation.h:101
+
+/// Enables signalling a cancellation on an asyn task. Totally thread-safe, can
+/// trigger cancellation from multiple threads or make copies. Since contains a

s/asyn/async



Comment at: clangd/Cancellation.h:104
+/// std::shared_ptr inside copying is costy, avoid it as much as possible.
+class TaskHandle {
+public:

As discussed offline, maybe we should make `TaskHandle` move-only?
Conceptually, `TaskHandle` is an abstraction that represent some spawned async 
task and in that sense somewhat similar to futures.




Comment at: clangd/Cancellation.h:108
+  static TaskHandle create();
+  friend Context setCurrentCancellationToken(TaskHandle TH);
+

I wonder if we should have a `createCancellationToken()` method and a method to 
stash it in the context instead?
Would help if we want to make `TaskHandle` move-only and in general seems like 
a simpler user API, because things you put and get back out of the context is 
the same, i.e. it's a `CancellationToken`.
WDYT?



Comment at: clangd/ClangdLSPServer.cpp:74
 
+const std::string NormalizeRequestID(const json::Value &ID) {
+  std::string NormalizedID;

NIT: return `std::string`, const does not buys us anything here.



Comment at: clangd/ClangdLSPServer.cpp:354
+if (!List) {
+  return replyError(List.takeError());
+}

NIT: remove braces around short, single-statement branches



Comment at: clangd/ClangdLSPServer.cpp:631
+return;
+  const std::string &NormalizedID = NormalizeRequestID(*ID);
+  {

Use a value type here, i.e. `std::string` instead of `const std::string&`.



Comment at: clangd/ClangdLSPServer.cpp:642
+return;
+  const std::string &NormalizedID 

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50627#1198595, @hokein wrote:

> I think this patch is in a good scope (for empty preamble). I'd leave it as 
> it is. The failure of GoTodefinition test is caused by an inconsistent 
> behavior of using `lastBuiltPreamble`/`NewPreamble` in TUScheduler, I will 
> send out a separate patch fixing it (based on our discussion).


LG, thanks!

+ a few more nits to the tests




Comment at: unittests/clangd/TUSchedulerTests.cpp:333
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(Preamble->Preamble->Preamble.getBounds().Size,

NIT: just use `cantFail(std::move(Preamble))` to get the underlying result, no 
need for an extra `ASSERT_TRUE` here.



Comment at: unittests/clangd/TUSchedulerTests.cpp:348
+  // We expect to get an empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_EQ(Preamble->Preamble->Preamble.getBounds().Size,

NIT: also use `cantFail`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627



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

The last set of comments is for the previous version, might be missing some 
updates, sorry about that. Will make sure to review the one too!


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] D50764: [AST] Make NullStmt final and give it factory functions

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Thanks. I'm not planning to move forward with this until the contracts patch is 
ready (it might change enough to not even require this); that should leave 
plenty of time for others to take a look.


Repository:
  rC Clang

https://reviews.llvm.org/D50764



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

[PATCH] D50170: [libcxxabi] Fix test_exception_address_alignment test for ARM

[Hans Wennborg via Phabricator via cfe-commits]

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

It doesn't seem to get more reviewed than this.

yroux, I'd say go ahead and commit it.


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D50170



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

r339860 - [AST] Pack the unsigned of DependentTemplateSpecializationType into Type

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Thu Aug 16 03:28:18 2018
New Revision: 339860

URL: http://llvm.org/viewvc/llvm-project?rev=339860&view=rev
Log:
[AST] Pack the unsigned of DependentTemplateSpecializationType into Type

The bit-fields of `Type` have enough space for the member
`unsigned NumArgs` of `DependentTemplateSpecializationType`.

Reviewed By: erichkeane

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


Modified:
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/lib/AST/Type.cpp

Modified: cfe/trunk/include/clang/AST/Type.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=339860&r1=339859&r2=339860&view=diff
==
--- cfe/trunk/include/clang/AST/Type.h (original)
+++ cfe/trunk/include/clang/AST/Type.h Thu Aug 16 03:28:18 2018
@@ -1571,6 +1571,8 @@ protected:
 unsigned Keyword : 8;
   };
 
+  enum { NumTypeWithKeywordBits = 8 };
+
   class VectorTypeBitfields {
 friend class VectorType;
 friend class DependentVectorType;
@@ -1624,6 +1626,22 @@ protected:
 unsigned NumArgs;
   };
 
+  class DependentTemplateSpecializationTypeBitfields {
+friend class DependentTemplateSpecializationType;
+
+unsigned : NumTypeBits;
+unsigned : NumTypeWithKeywordBits;
+
+/// The number of template arguments named in this class template
+/// specialization, which is expected to be able to hold at least 1024
+/// according to [implimits]. However, as this limit is somewhat easy to
+/// hit with template metaprogramming we'd prefer to keep it as large
+/// as possible. At the moment it has been left as a non-bitfield since
+/// this type safely fits in 64 bits as an unsigned, so there is no reason
+/// to introduce the performance impact of a bitfield.
+unsigned NumArgs;
+  };
+
   class PackExpansionTypeBitfields {
 friend class PackExpansionType;
 
@@ -1655,6 +1673,8 @@ protected:
 TypeWithKeywordBitfields TypeWithKeywordBits;
 VectorTypeBitfields VectorTypeBits;
 TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
+DependentTemplateSpecializationTypeBitfields
+  DependentTemplateSpecializationTypeBits;
 PackExpansionTypeBitfields PackExpansionTypeBits;
 
 static_assert(sizeof(TypeBitfields) <= 8,
@@ -1680,6 +1700,9 @@ protected:
 static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
   "TemplateSpecializationTypeBitfields is larger"
   " than 8 bytes!");
+static_assert(sizeof(DependentTemplateSpecializationTypeBitfields) <= 8,
+  "DependentTemplateSpecializationTypeBitfields is larger"
+  " than 8 bytes!");
 static_assert(sizeof(PackExpansionTypeBitfields) <= 8,
   "PackExpansionTypeBitfields is larger than 8 bytes");
   };
@@ -5123,10 +5146,6 @@ class alignas(8) DependentTemplateSpecia
   /// The identifier of the template.
   const IdentifierInfo *Name;
 
-  /// The number of template arguments named in this class template
-  /// specialization.
-  unsigned NumArgs;
-
   DependentTemplateSpecializationType(ElaboratedTypeKeyword Keyword,
   NestedNameSpecifier *NNS,
   const IdentifierInfo *Name,
@@ -5151,12 +5170,14 @@ public:
   }
 
   /// Retrieve the number of template arguments.
-  unsigned getNumArgs() const { return NumArgs; }
+  unsigned getNumArgs() const {
+return DependentTemplateSpecializationTypeBits.NumArgs;
+  }
 
   const TemplateArgument &getArg(unsigned Idx) const; // in TemplateBase.h
 
   ArrayRef template_arguments() const {
-return {getArgs(), NumArgs};
+return {getArgs(), getNumArgs()};
   }
 
   using iterator = const TemplateArgument *;
@@ -5168,7 +5189,7 @@ public:
   QualType desugar() const { return QualType(this, 0); }
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context) {
-Profile(ID, Context, getKeyword(), NNS, Name, {getArgs(), NumArgs});
+Profile(ID, Context, getKeyword(), NNS, Name, {getArgs(), getNumArgs()});
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID,

Modified: cfe/trunk/lib/AST/Type.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=339860&r1=339859&r2=339860&view=diff
==
--- cfe/trunk/lib/AST/Type.cpp (original)
+++ cfe/trunk/lib/AST/Type.cpp Thu Aug 16 03:28:18 2018
@@ -2604,7 +2604,8 @@ DependentTemplateSpecializationType::Dep
   : TypeWithKeyword(Keyword, DependentTemplateSpecialization, Canon, true, 
true,
 /*VariablyModified=*/false,
 NNS && NNS->containsUnexpandedParameterPack()),
-NNS(NNS), Name(Name), NumArgs(Args.size()) {
+NNS(NNS), Name(Name) {
+  DependentTemplateSpecializationTypeBits.NumArgs = Args.size();
   assert((!NNS || NNS->isDependent()) &&
  "DependentTemplateSpecializa

[PATCH] D50712: [AST] Pack the unsigned of DependentTemplateSpecializationType into Type

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339860: [AST] Pack the unsigned of 
DependentTemplateSpecializationType into Type (authored by brunoricci, 
committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50712?vs=160800&id=160989#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50712

Files:
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/lib/AST/Type.cpp

Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -1571,6 +1571,8 @@
 unsigned Keyword : 8;
   };
 
+  enum { NumTypeWithKeywordBits = 8 };
+
   class VectorTypeBitfields {
 friend class VectorType;
 friend class DependentVectorType;
@@ -1624,6 +1626,22 @@
 unsigned NumArgs;
   };
 
+  class DependentTemplateSpecializationTypeBitfields {
+friend class DependentTemplateSpecializationType;
+
+unsigned : NumTypeBits;
+unsigned : NumTypeWithKeywordBits;
+
+/// The number of template arguments named in this class template
+/// specialization, which is expected to be able to hold at least 1024
+/// according to [implimits]. However, as this limit is somewhat easy to
+/// hit with template metaprogramming we'd prefer to keep it as large
+/// as possible. At the moment it has been left as a non-bitfield since
+/// this type safely fits in 64 bits as an unsigned, so there is no reason
+/// to introduce the performance impact of a bitfield.
+unsigned NumArgs;
+  };
+
   class PackExpansionTypeBitfields {
 friend class PackExpansionType;
 
@@ -1655,6 +1673,8 @@
 TypeWithKeywordBitfields TypeWithKeywordBits;
 VectorTypeBitfields VectorTypeBits;
 TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
+DependentTemplateSpecializationTypeBitfields
+  DependentTemplateSpecializationTypeBits;
 PackExpansionTypeBitfields PackExpansionTypeBits;
 
 static_assert(sizeof(TypeBitfields) <= 8,
@@ -1680,6 +1700,9 @@
 static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
   "TemplateSpecializationTypeBitfields is larger"
   " than 8 bytes!");
+static_assert(sizeof(DependentTemplateSpecializationTypeBitfields) <= 8,
+  "DependentTemplateSpecializationTypeBitfields is larger"
+  " than 8 bytes!");
 static_assert(sizeof(PackExpansionTypeBitfields) <= 8,
   "PackExpansionTypeBitfields is larger than 8 bytes");
   };
@@ -5123,10 +5146,6 @@
   /// The identifier of the template.
   const IdentifierInfo *Name;
 
-  /// The number of template arguments named in this class template
-  /// specialization.
-  unsigned NumArgs;
-
   DependentTemplateSpecializationType(ElaboratedTypeKeyword Keyword,
   NestedNameSpecifier *NNS,
   const IdentifierInfo *Name,
@@ -5151,12 +5170,14 @@
   }
 
   /// Retrieve the number of template arguments.
-  unsigned getNumArgs() const { return NumArgs; }
+  unsigned getNumArgs() const {
+return DependentTemplateSpecializationTypeBits.NumArgs;
+  }
 
   const TemplateArgument &getArg(unsigned Idx) const; // in TemplateBase.h
 
   ArrayRef template_arguments() const {
-return {getArgs(), NumArgs};
+return {getArgs(), getNumArgs()};
   }
 
   using iterator = const TemplateArgument *;
@@ -5168,7 +5189,7 @@
   QualType desugar() const { return QualType(this, 0); }
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context) {
-Profile(ID, Context, getKeyword(), NNS, Name, {getArgs(), NumArgs});
+Profile(ID, Context, getKeyword(), NNS, Name, {getArgs(), getNumArgs()});
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID,
Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -2604,7 +2604,8 @@
   : TypeWithKeyword(Keyword, DependentTemplateSpecialization, Canon, true, true,
 /*VariablyModified=*/false,
 NNS && NNS->containsUnexpandedParameterPack()),
-NNS(NNS), Name(Name), NumArgs(Args.size()) {
+NNS(NNS), Name(Name) {
+  DependentTemplateSpecializationTypeBits.NumArgs = Args.size();
   assert((!NNS || NNS->isDependent()) &&
  "DependentTemplateSpecializatonType requires dependent qualifier");
   TemplateArgument *ArgBuffer = getArgBuffer();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Jan Korous via Phabricator via cfe-commits]

jkorous added a reviewer: vsapsai.
jkorous added a comment.

Adding Volodymyr who landed related patch recently.


Repository:
  rC Clang

https://reviews.llvm.org/D50462



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

r339861 - [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Thu Aug 16 03:33:36 2018
New Revision: 339861

URL: http://llvm.org/viewvc/llvm-project?rev=339861&view=rev
Log:
[AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type

The bit-fields of Type have enough space for the member
unsigned NumArgs of SubstTemplateTypeParmPackType.

Reviewed By: erichkeane

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


Modified:
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/lib/AST/Type.cpp

Modified: cfe/trunk/include/clang/AST/Type.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=339861&r1=339860&r2=339861&view=diff
==
--- cfe/trunk/include/clang/AST/Type.h (original)
+++ cfe/trunk/include/clang/AST/Type.h Thu Aug 16 03:33:36 2018
@@ -1608,6 +1608,21 @@ protected:
 unsigned Keyword : 2;
   };
 
+  class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments, which is
+/// expected to be able to hold at least 1024 according to [implimits].
+/// However as this limit is somewhat easy to hit with template
+/// metaprogramming we'd prefer to keep it as large as possible.
+/// At the moment it has been left as a non-bitfield since this type
+/// safely fits in 64 bits as an unsigned, so there is no reason to
+/// introduce the performance impact of a bitfield.
+unsigned NumArgs;
+  };
+
   class TemplateSpecializationTypeBitfields {
 friend class TemplateSpecializationType;
 
@@ -1672,6 +1687,7 @@ protected:
 ReferenceTypeBitfields ReferenceTypeBits;
 TypeWithKeywordBitfields TypeWithKeywordBits;
 VectorTypeBitfields VectorTypeBits;
+SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
 TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
 DependentTemplateSpecializationTypeBitfields
   DependentTemplateSpecializationTypeBits;
@@ -1697,6 +1713,9 @@ protected:
   "TypeWithKeywordBitfields is larger than 8 bytes!");
 static_assert(sizeof(VectorTypeBitfields) <= 8,
   "VectorTypeBitfields is larger than 8 bytes!");
+static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
+  "SubstTemplateTypeParmPackTypeBitfields is larger"
+  " than 8 bytes!");
 static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
   "TemplateSpecializationTypeBitfields is larger"
   " than 8 bytes!");
@@ -4551,9 +4570,6 @@ class SubstTemplateTypeParmPackType : pu
   /// parameter pack is instantiated with.
   const TemplateArgument *Arguments;
 
-  /// The number of template arguments in \c Arguments.
-  unsigned NumArguments;
-
   SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
 QualType Canon,
 const TemplateArgument &ArgPack);
@@ -4566,6 +4582,10 @@ public:
 return Replaced;
   }
 
+  unsigned getNumArgs() const {
+return SubstTemplateTypeParmPackTypeBits.NumArgs;
+  }
+
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 

Modified: cfe/trunk/lib/AST/Type.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=339861&r1=339860&r2=339861&view=diff
==
--- cfe/trunk/lib/AST/Type.cpp (original)
+++ cfe/trunk/lib/AST/Type.cpp Thu Aug 16 03:33:36 2018
@@ -3285,11 +3285,12 @@ SubstTemplateTypeParmPackType(const Temp
   QualType Canon,
   const TemplateArgument &ArgPack)
 : Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
-  Replaced(Param),
-  Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+  Replaced(Param), Arguments(ArgPack.pack_begin()) {
+  SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
 
 TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
-  return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+  return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
 }
 
 void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID &ID) {


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

[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339861: [AST] Pack the unsigned of 
SubstTemplateTypeParmPackType into Type (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50713?vs=160795&id=160991#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50713

Files:
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/lib/AST/Type.cpp


Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
   QualType Canon,
   const TemplateArgument &ArgPack)
 : Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
-  Replaced(Param),
-  Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+  Replaced(Param), Arguments(ArgPack.pack_begin()) {
+  SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
 
 TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
-  return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+  return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
 }
 
 void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID &ID) {
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -1608,6 +1608,21 @@
 unsigned Keyword : 2;
   };
 
+  class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments, which is
+/// expected to be able to hold at least 1024 according to [implimits].
+/// However as this limit is somewhat easy to hit with template
+/// metaprogramming we'd prefer to keep it as large as possible.
+/// At the moment it has been left as a non-bitfield since this type
+/// safely fits in 64 bits as an unsigned, so there is no reason to
+/// introduce the performance impact of a bitfield.
+unsigned NumArgs;
+  };
+
   class TemplateSpecializationTypeBitfields {
 friend class TemplateSpecializationType;
 
@@ -1672,6 +1687,7 @@
 ReferenceTypeBitfields ReferenceTypeBits;
 TypeWithKeywordBitfields TypeWithKeywordBits;
 VectorTypeBitfields VectorTypeBits;
+SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
 TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
 DependentTemplateSpecializationTypeBitfields
   DependentTemplateSpecializationTypeBits;
@@ -1697,6 +1713,9 @@
   "TypeWithKeywordBitfields is larger than 8 bytes!");
 static_assert(sizeof(VectorTypeBitfields) <= 8,
   "VectorTypeBitfields is larger than 8 bytes!");
+static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
+  "SubstTemplateTypeParmPackTypeBitfields is larger"
+  " than 8 bytes!");
 static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
   "TemplateSpecializationTypeBitfields is larger"
   " than 8 bytes!");
@@ -4551,9 +4570,6 @@
   /// parameter pack is instantiated with.
   const TemplateArgument *Arguments;
 
-  /// The number of template arguments in \c Arguments.
-  unsigned NumArguments;
-
   SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
 QualType Canon,
 const TemplateArgument &ArgPack);
@@ -4566,6 +4582,10 @@
 return Replaced;
   }
 
+  unsigned getNumArgs() const {
+return SubstTemplateTypeParmPackTypeBits.NumArgs;
+  }
+
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 


Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
   QualType Canon,
   const TemplateArgument &ArgPack)
 : Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
-  Replaced(Param),
-  Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+  Replaced(Param), Arguments(ArgPack.pack_begin()) {
+  SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
 
 TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
-  return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+  return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
 }
 
 void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID &ID) {
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/i

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 160992.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

- Log on index request, remove FIXME that was addressed
- Remove SymbolID::forDecl, use existing helper instead


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727

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

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
   EXPECT_TRUE(Results.Completions.empty());
 }
 
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+  std::unique_ptr Index;
+  if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.StaticIndex = Index.get();
+
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
   return false;
   return true;
 }
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
 
 Matcher Sig(std::string Label,
   std::vector Params) {
@@ -1590,6 +1599,51 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SignatureHelpTest, IndexDocumentation) {
+  Symbol::Details DocDetails;
+  DocDetails.Documentation = "Doc from the index";
+
+  Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+  Foo0.Detail = &DocDetails;
+  Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+  Foo1.Detail = &DocDetails;
+  Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+  EXPECT_THAT(
+  signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+  foo(^);
+}
+  )cpp",
+ {Foo0, Foo1, Foo2})
+  .signatures,
+  ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+  AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+  AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -171,12 +171,11 @@
 CodeCompleteOptions Opts);
 
 /// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-const tooling::CompileCommand &Command,
-PrecompiledPreamble const *Preamble,
-StringRef Contents, Position Pos,
-IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileCommand &Command,
+  PrecompiledPreamble const *Preamble, StringRef Contents,
+  Position Pos, IntrusiveRefCntPtr VFS,
+  std::shared_ptr PCHs, SymbolIndex *Index);
 
 // For index-based completion, we only consider:
 //   * symbols in namespaces or translation unit scopes (e.g. no class
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -687,18 +687,23 @@
   llvm::unique_function ResultsCallback;
 };
 
-using ScoredSignature =
-std::pair;
+struct ScoredSignature {
+  // When set, requires documentation to be requested from the index with this
+  // ID.
+  llvm::Optional IDForDoc;
+  SignatureInformation Signature;
+  SignatureQualitySignals Quality;
+};
 
 class SignatureHelpCollector final : public CodeCompleteConsumer {
-
 public:
   SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts,
- SignatureHelp &SigHelp)
-  : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
+ SymbolIndex *Index, SignatureHelp &SigHelp)
+  : CodeComplete

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

drive by: I think this should  be `vlog` or `dlog`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

r339862 - [AST] Store the OwnedTagDecl as a trailing object in ElaboratedType.

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Thu Aug 16 03:48:16 2018
New Revision: 339862

URL: http://llvm.org/viewvc/llvm-project?rev=339862&view=rev
Log:
[AST] Store the OwnedTagDecl as a trailing object in ElaboratedType.

The TagDecl *OwnedTagDecl in ElaboratedType is quite commonly
null (at least when parsing all of Boost, it is non-null for only about 600
of the 66k ElaboratedType). Therefore we can save a pointer in the
common case by storing it as a trailing object, and storing a bit in the
bit-fields of Type indicating when the pointer is null.

Reviewed By: rjmccall

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


Modified:
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/lib/AST/ASTContext.cpp

Modified: cfe/trunk/include/clang/AST/Type.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=339862&r1=339861&r2=339862&view=diff
==
--- cfe/trunk/include/clang/AST/Type.h (original)
+++ cfe/trunk/include/clang/AST/Type.h Thu Aug 16 03:48:16 2018
@@ -45,6 +45,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include "llvm/Support/type_traits.h"
+#include "llvm/Support/TrailingObjects.h"
 #include 
 #include 
 #include 
@@ -1573,6 +1574,16 @@ protected:
 
   enum { NumTypeWithKeywordBits = 8 };
 
+  class ElaboratedTypeBitfields {
+friend class ElaboratedType;
+
+unsigned : NumTypeBits;
+unsigned : NumTypeWithKeywordBits;
+
+/// Whether the ElaboratedType has a trailing OwnedTagDecl.
+unsigned HasOwnedTagDecl : 1;
+  };
+
   class VectorTypeBitfields {
 friend class VectorType;
 friend class DependentVectorType;
@@ -1686,6 +1697,7 @@ protected:
 ObjCObjectTypeBitfields ObjCObjectTypeBits;
 ReferenceTypeBitfields ReferenceTypeBits;
 TypeWithKeywordBitfields TypeWithKeywordBits;
+ElaboratedTypeBitfields ElaboratedTypeBits;
 VectorTypeBitfields VectorTypeBits;
 SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
 TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
@@ -1711,6 +1723,8 @@ protected:
   "ReferenceTypeBitfields is larger than 8 bytes!");
 static_assert(sizeof(TypeWithKeywordBitfields) <= 8,
   "TypeWithKeywordBitfields is larger than 8 bytes!");
+static_assert(sizeof(ElaboratedTypeBitfields) <= 8,
+  "ElaboratedTypeBitfields is larger than 8 bytes!");
 static_assert(sizeof(VectorTypeBitfields) <= 8,
   "VectorTypeBitfields is larger than 8 bytes!");
 static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
@@ -5028,8 +5042,12 @@ public:
 /// source code, including tag keywords and any nested-name-specifiers.
 /// The type itself is always "sugar", used to express what was written
 /// in the source code but containing no additional semantic information.
-class ElaboratedType : public TypeWithKeyword, public llvm::FoldingSetNode {
+class ElaboratedType final
+: public TypeWithKeyword,
+  public llvm::FoldingSetNode,
+  private llvm::TrailingObjects {
   friend class ASTContext; // ASTContext creates these
+  friend TrailingObjects;
 
   /// The nested name specifier containing the qualifier.
   NestedNameSpecifier *NNS;
@@ -5037,26 +5055,29 @@ class ElaboratedType : public TypeWithKe
   /// The type that this qualified name refers to.
   QualType NamedType;
 
-  /// The (re)declaration of this tag type owned by this occurrence, or nullptr
-  /// if none.
-  TagDecl *OwnedTagDecl;
+  /// The (re)declaration of this tag type owned by this occurrence is stored
+  /// as a trailing object if there is one. Use getOwnedTagDecl to obtain
+  /// it, or obtain a null pointer if there is none.
 
   ElaboratedType(ElaboratedTypeKeyword Keyword, NestedNameSpecifier *NNS,
  QualType NamedType, QualType CanonType, TagDecl *OwnedTagDecl)
-: TypeWithKeyword(Keyword, Elaborated, CanonType,
-  NamedType->isDependentType(),
-  NamedType->isInstantiationDependentType(),
-  NamedType->isVariablyModifiedType(),
-  NamedType->containsUnexpandedParameterPack()),
-  NNS(NNS), NamedType(NamedType), OwnedTagDecl(OwnedTagDecl) {
+  : TypeWithKeyword(Keyword, Elaborated, CanonType,
+NamedType->isDependentType(),
+NamedType->isInstantiationDependentType(),
+NamedType->isVariablyModifiedType(),
+NamedType->containsUnexpandedParameterPack()),
+NNS(NNS), NamedType(NamedType) {
+ElaboratedTypeBits.HasOwnedTagDecl = false;
+if (OwnedTagDecl) {
+  ElaboratedTypeBits.HasOwnedTagDecl = true;
+  *getTrailingObjects() = OwnedTagDecl;
+}
 assert(!(Keyword == ETK_None && NNS == nullptr) &&
"ElaboratedType cannot have elaborated type keyword "
"

[PATCH] D50715: [AST] Store the OwnedTagDecl as a trailing object in ElaboratedType.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339862: [AST] Store the OwnedTagDecl as a trailing object in 
ElaboratedType. (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50715?vs=160606&id=160995#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50715

Files:
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/lib/AST/ASTContext.cpp

Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -45,6 +45,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include "llvm/Support/type_traits.h"
+#include "llvm/Support/TrailingObjects.h"
 #include 
 #include 
 #include 
@@ -1573,6 +1574,16 @@
 
   enum { NumTypeWithKeywordBits = 8 };
 
+  class ElaboratedTypeBitfields {
+friend class ElaboratedType;
+
+unsigned : NumTypeBits;
+unsigned : NumTypeWithKeywordBits;
+
+/// Whether the ElaboratedType has a trailing OwnedTagDecl.
+unsigned HasOwnedTagDecl : 1;
+  };
+
   class VectorTypeBitfields {
 friend class VectorType;
 friend class DependentVectorType;
@@ -1686,6 +1697,7 @@
 ObjCObjectTypeBitfields ObjCObjectTypeBits;
 ReferenceTypeBitfields ReferenceTypeBits;
 TypeWithKeywordBitfields TypeWithKeywordBits;
+ElaboratedTypeBitfields ElaboratedTypeBits;
 VectorTypeBitfields VectorTypeBits;
 SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
 TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
@@ -1711,6 +1723,8 @@
   "ReferenceTypeBitfields is larger than 8 bytes!");
 static_assert(sizeof(TypeWithKeywordBitfields) <= 8,
   "TypeWithKeywordBitfields is larger than 8 bytes!");
+static_assert(sizeof(ElaboratedTypeBitfields) <= 8,
+  "ElaboratedTypeBitfields is larger than 8 bytes!");
 static_assert(sizeof(VectorTypeBitfields) <= 8,
   "VectorTypeBitfields is larger than 8 bytes!");
 static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
@@ -5028,35 +5042,42 @@
 /// source code, including tag keywords and any nested-name-specifiers.
 /// The type itself is always "sugar", used to express what was written
 /// in the source code but containing no additional semantic information.
-class ElaboratedType : public TypeWithKeyword, public llvm::FoldingSetNode {
+class ElaboratedType final
+: public TypeWithKeyword,
+  public llvm::FoldingSetNode,
+  private llvm::TrailingObjects {
   friend class ASTContext; // ASTContext creates these
+  friend TrailingObjects;
 
   /// The nested name specifier containing the qualifier.
   NestedNameSpecifier *NNS;
 
   /// The type that this qualified name refers to.
   QualType NamedType;
 
-  /// The (re)declaration of this tag type owned by this occurrence, or nullptr
-  /// if none.
-  TagDecl *OwnedTagDecl;
+  /// The (re)declaration of this tag type owned by this occurrence is stored
+  /// as a trailing object if there is one. Use getOwnedTagDecl to obtain
+  /// it, or obtain a null pointer if there is none.
 
   ElaboratedType(ElaboratedTypeKeyword Keyword, NestedNameSpecifier *NNS,
  QualType NamedType, QualType CanonType, TagDecl *OwnedTagDecl)
-: TypeWithKeyword(Keyword, Elaborated, CanonType,
-  NamedType->isDependentType(),
-  NamedType->isInstantiationDependentType(),
-  NamedType->isVariablyModifiedType(),
-  NamedType->containsUnexpandedParameterPack()),
-  NNS(NNS), NamedType(NamedType), OwnedTagDecl(OwnedTagDecl) {
+  : TypeWithKeyword(Keyword, Elaborated, CanonType,
+NamedType->isDependentType(),
+NamedType->isInstantiationDependentType(),
+NamedType->isVariablyModifiedType(),
+NamedType->containsUnexpandedParameterPack()),
+NNS(NNS), NamedType(NamedType) {
+ElaboratedTypeBits.HasOwnedTagDecl = false;
+if (OwnedTagDecl) {
+  ElaboratedTypeBits.HasOwnedTagDecl = true;
+  *getTrailingObjects() = OwnedTagDecl;
+}
 assert(!(Keyword == ETK_None && NNS == nullptr) &&
"ElaboratedType cannot have elaborated type keyword "
"and name qualifier both null.");
   }
 
 public:
-  ~ElaboratedType();
-
   /// Retrieve the qualification on this type.
   NestedNameSpecifier *getQualifier() const { return NNS; }
 
@@ -5070,11 +5091,14 @@
   bool isSugared() const { return true; }
 
   /// Return the (re)declaration of this type owned by this occurrence of this
-  /// type, or nullptr if none.
-  TagDecl *getOwnedTagDecl() const { return OwnedTagDecl; }
+  /// type, or nullptr if there is none.
+  TagDecl *getOwnedTagDecl() const {

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Alex, nice work!

I am just wondering if it would be beneficial to assert Loc and End are in the 
same file. It might help to catch bugs.

I also stumbled upon this function but not sure it makes things significantly 
cleaner here:
https://clang.llvm.org/doxygen/SourceLocation_8h_source.html#l00175

LGTM otherwise.


Repository:
  rC Clang

https://reviews.llvm.org/D50740



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:742
+llvm::DenseMap FetchedDocs;
+if (Index) {
+  LookupRequest IndexRequest;

hokein wrote:
> nit: do we want to log anything here? It may be useful for debug.
Definitely useful. Thanks!



Comment at: clangd/index/Index.h:65
 public:
+  static llvm::Optional forDecl(const Decl &D);
+

hokein wrote:
> We already have this similar function in clangd/AST.h.
Thanks for pointing this out.
It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern 
would probably be a somewhat broken layering, right? E.g. `Index.h` should not 
directly depend on anything AST-specific


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D47196: [Time-report ](2): Recursive timers in Clang

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 updated this revision to Diff 160997.
avt77 added a comment.

The ability to produce debug output for 'ftiming' was added. As result now it's 
possible to check places where timers start/stop and for what functions it's 
being done (see changes in Utils.h).


https://reviews.llvm.org/D47196

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.h
  include/clang/Frontend/Utils.h
  lib/Analysis/AnalysisDeclContext.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendTiming.cpp
  lib/Parse/CMakeLists.txt
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseTemplate.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/TreeTransform.h
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  lib/Tooling/Tooling.cpp
  test/Frontend/ftime-report-template-decl.cpp
  test/Headers/opencl-c-header.cl

Index: test/Headers/opencl-c-header.cl
===
--- test/Headers/opencl-c-header.cl
+++ test/Headers/opencl-c-header.cl
@@ -71,4 +71,5 @@
 }
 #endif //__OPENCL_C_VERSION__
 
-// CHECK-MOD: Reading modules
+// CHECK-DAG-MOD: Clang Timers: CodeGen Functions
+// CHECK-DAG-MOD: Reading modules
Index: test/Frontend/ftime-report-template-decl.cpp
===
--- test/Frontend/ftime-report-template-decl.cpp
+++ test/Frontend/ftime-report-template-decl.cpp
@@ -3,9 +3,15 @@
 
 // Template function declarations
 template 
-void foo();
+T foo(T bar) {
+  T Result = bar * bar + bar / 1.2 + bar;
+  return Result;
+};
 template 
-void foo();
+T foo(T bar, U bar2) {
+  T Result = bar2 * bar + bar / 1.2 + bar2;
+  return Result;
+};
 
 // Template function definitions.
 template 
@@ -130,9 +136,15 @@
 template 
 oneT L<0>::O::Fun(U) { return one; }
 
-void Instantiate() {
+double Instantiate() {
   sassert(sizeof(L<0>::O::Fun(0)) == sizeof(one));
   sassert(sizeof(L<0>::O::Fun(0)) == sizeof(one));
+  int R1 = foo(123) + foo(177.2) - foo(331.442);
+  char R2 = foo('d', 1234) * foo(1.26);
+  int R3 = foo(1.2) + foo(11.22) / foo(66.77);
+  double R4 = foo(34.56, 1234);
+  double R5 = R1 + R2 * R3 - R4 + one[0] * foo(15.52) - two[1] / foo(51.25);
+  return R5 * R1 + R4 / R3 + R2;
 }
 }
 
@@ -150,7 +162,10 @@
 };
 _Wrap_alloc::rebind w;
 
-// CHECK: Miscellaneous Ungrouped Timers
+// FIXME:  We need more complex test to increase the compilation time;
+// otherwise we see the foolowing message from time to time only.
+// VIOLATILE-CHECK: Clang Timers: CodeGen Functions
+// CHECK-DAG: Miscellaneous Ungrouped Timers
 // CHECK-DAG: LLVM IR Generation Time
 // CHECK-DAG: Code Generation Time
 // CHECK: Total
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -12,6 +12,8 @@
 //
 //===--===//
 
+#define DEBUG_TYPE "clang-tooling"
+
 #include "clang/Tooling/Tooling.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
@@ -60,8 +62,6 @@
 #include 
 #include 
 
-#define DEBUG_TYPE "clang-tooling"
-
 using namespace clang;
 using namespace tooling;
 
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -11,6 +11,8 @@
 //
 //===--===//
 
+#define DEBUG_TYPE "AnalysisConsumer"
+
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "ModelInjector.h"
 #include "clang/AST/Decl.h"
@@ -48,8 +50,6 @@
 using namespace clang;
 using namespace ento;
 
-#define DEBUG_TYPE "AnalysisConsumer"
-
 static std::unique_ptr CreateUbiViz();
 
 STATISTIC(NumFunctionTopLevel, "The # of functions at top level.");
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -27,6 +27,7 @@
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/StmtOpenMP.h"
+#include "clang/Frontend/Utils.h"
 #include "clang/Sema/Designator.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Ownership.h"
@@ -10960,6 +10961,11 @@
 
   LSI->CallOperator = NewCallOperator;
 
+  FrontendTimeRAII FTRAII(
+  "TransformLambdaExpr", FrontendTimesIsEnabled,
+  getFrontendFunctionTimeCtx(),
+  {NewCallOperator, 0.0});
+
   for (unsigned I = 0, NumParams = NewCallOperator->getNumParams();
I != NumParams; ++I) {
 auto *P = NewCallOperator->getParam

Re: [PATCH] D50645: [clangd] Show non-instantiated decls in signatureHelp

[Yvan Roux via cfe-commits]

Hi Ilya,

This commit broke some of ARM's bots, log are available here:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/3120/steps/ninja%20check%201/logs/stdio

Thanks
Yvan

On Tue, 14 Aug 2018 at 11:37, Phabricator via Phabricator via
cfe-commits  wrote:
>
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL339665: [clangd] Show non-instantiated decls in 
> signatureHelp (authored by ibiryukov, committed by ).
> Herald added a subscriber: llvm-commits.
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D50645
>
> Files:
>   clang-tools-extra/trunk/clangd/CodeComplete.cpp
>   clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
>
>
> Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> ===
> --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> @@ -1537,6 +1537,59 @@
>EXPECT_EQ(0, Results.activeParameter);
>  }
>
> +TEST(SignatureHelpTest, InstantiatedSignatures) {
> +  EXPECT_THAT(signatures(R"cpp(
> +template 
> +void foo(T, T, T);
> +
> +int main() {
> +  foo(^);
> +}
> +  )cpp")
> +  .signatures,
> +  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
> +
> +  EXPECT_THAT(signatures(R"cpp(
> +template 
> +void foo(T, T, T);
> +
> +int main() {
> +  foo(10, ^);
> +})cpp")
> +  .signatures,
> +  ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
> +
> +  EXPECT_THAT(signatures(R"cpp(
> +template 
> +void foo(T...);
> +
> +int main() {
> +  foo(^);
> +}
> +  )cpp")
> +  .signatures,
> +  ElementsAre(Sig("foo(T...) -> void", {"T..."})));
> +
> +  // It is debatable whether we should substitute the outer template 
> parameter
> +  // ('T') in that case. Currently we don't substitute it in signature help, 
> but
> +  // do substitute in code complete.
> +  // FIXME: make code complete and signature help consistent, figure out 
> which
> +  // way is better.
> +  EXPECT_THAT(signatures(R"cpp(
> +template 
> +struct X {
> +  template 
> +  void foo(T, U);
> +};
> +
> +int main() {
> +  X().foo(^)
> +}
> +  )cpp")
> +  .signatures,
> +  ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
> +}
> +
>  } // namespace
>  } // namespace clangd
>  } // namespace clang
> Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
> ===
> --- clang-tools-extra/trunk/clangd/CodeComplete.cpp
> +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
> @@ -714,7 +714,15 @@
> "too many arguments");
>  SigHelp.activeParameter = static_cast(CurrentArg);
>  for (unsigned I = 0; I < NumCandidates; ++I) {
> -  const auto &Candidate = Candidates[I];
> +  OverloadCandidate Candidate = Candidates[I];
> +  // We want to avoid showing instantiated signatures, because they may 
> be
> +  // long in some cases (e.g. when 'T' is substituted with 
> 'std::string', we
> +  // would get 'std::basic_string').
> +  if (auto *Func = Candidate.getFunction()) {
> +if (auto *Pattern = Func->getTemplateInstantiationPattern())
> +  Candidate = OverloadCandidate(Pattern);
> +  }
> +
>const auto *CCS = Candidate.CreateSignatureString(
>CurrentArg, S, *Allocator, CCTUInfo, true);
>assert(CCS && "Expected the CodeCompletionString to be non-null");
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/Index.h:65
 public:
+  static llvm::Optional forDecl(const Decl &D);
+

ilya-biryukov wrote:
> hokein wrote:
> > We already have this similar function in clangd/AST.h.
> Thanks for pointing this out.
> It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern 
> would probably be a somewhat broken layering, right? E.g. `Index.h` should 
> not directly depend on anything AST-specific
Yes, I think we will not add any AST-specific stuff to `Index.h`, that's why we 
have `AST.h`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Basic/SourceManager.cpp:2035
+ "Passed invalid source location!");
+  assert(Start.isFileID() && End.isFileID() && Loc.isFileID() &&
+ "Passed non-file source location!");

Why do we disallow locations from macro expansions?



Comment at: lib/Basic/SourceManager.cpp:2049
+return false;
+  std::pair EndLoc = getDecomposedLoc(End);
+  // The point must be in the same file as the end location.

nit: the pattern seems repetitive. Maybe pull out a helper/lambda like: 
`isBeforeInSameFile(L1, L2)`?



Repository:
  rC Clang

https://reviews.llvm.org/D50740



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ioeric wrote:
> drive by: I think this should  be `vlog` or `dlog`.
Code completion also logs the number of results from sema, index, etc. using 
the `log()` call.
The added log message looks similar, so trying to be consistent with the rest 
of the code in this file.

Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave it 
to a separate patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[libcxxabi] r339865 - [libcxxabi] Fix test_exception_address_alignment test for ARM

[Yvan Roux via cfe-commits]

Author: yroux
Date: Thu Aug 16 04:38:09 2018
New Revision: 339865

URL: http://llvm.org/viewvc/llvm-project?rev=339865&view=rev
Log:
[libcxxabi] Fix test_exception_address_alignment test for ARM

Check _LIBCXXABI_ARM_EHABI macro instead of libunwind version.

Fixes PR34182

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

Modified:
libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp

Modified: libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp?rev=339865&r1=339864&r2=339865&view=diff
==
--- libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp (original)
+++ libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp Thu Aug 16 
04:38:09 2018
@@ -20,6 +20,7 @@
 
 #include 
 #include 
+#include <__cxxabi_config.h>
 
 #include 
 
@@ -27,7 +28,7 @@ struct __attribute__((aligned)) AlignedT
 
 // EHABI  : 8-byte aligned
 // Itanium: Largest supported alignment for the system
-#if defined(_LIBUNWIND_ARM_EHABI)
+#if defined(_LIBCXXABI_ARM_EHABI)
 #  define EXPECTED_ALIGNMENT 8
 #else
 #  define EXPECTED_ALIGNMENT alignof(AlignedType)


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

[PATCH] D50170: [libcxxabi] Fix test_exception_address_alignment test for ARM

[Yvan Roux via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339865: [libcxxabi] Fix test_exception_address_alignment 
test for ARM (authored by yroux, committed by ).
Herald added a subscriber: christof.

Changed prior to commit:
  https://reviews.llvm.org/D50170?vs=158696&id=160999#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50170

Files:
  libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp


Index: libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
===
--- libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
+++ libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
@@ -20,14 +20,15 @@
 
 #include 
 #include 
+#include <__cxxabi_config.h>
 
 #include 
 
 struct __attribute__((aligned)) AlignedType {};
 
 // EHABI  : 8-byte aligned
 // Itanium: Largest supported alignment for the system
-#if defined(_LIBUNWIND_ARM_EHABI)
+#if defined(_LIBCXXABI_ARM_EHABI)
 #  define EXPECTED_ALIGNMENT 8
 #else
 #  define EXPECTED_ALIGNMENT alignof(AlignedType)


Index: libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
===
--- libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
+++ libcxxabi/trunk/test/test_exception_address_alignment.pass.cpp
@@ -20,14 +20,15 @@
 
 #include 
 #include 
+#include <__cxxabi_config.h>
 
 #include 
 
 struct __attribute__((aligned)) AlignedType {};
 
 // EHABI  : 8-byte aligned
 // Itanium: Largest supported alignment for the system
-#if defined(_LIBUNWIND_ARM_EHABI)
+#if defined(_LIBCXXABI_ARM_EHABI)
 #  define EXPECTED_ALIGNMENT 8
 #else
 #  define EXPECTED_ALIGNMENT alignof(AlignedType)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

[Haojian Wu via Phabricator via cfe-commits]

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

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50627

Files:
  unittests/clangd/TUSchedulerTests.cpp


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,52 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -308,6 +308,52 @@
   UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
+TEST_F(TUSchedulerTests, EmptyPreamble) {
+  TUScheduler S(
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+  PreambleParsedCallback(),
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+  ASTRetentionPolicy());
+
+  auto Foo = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  Files[Header] = "void foo()";
+  Timestamps[Header] = time_t(0);
+  auto WithPreamble = R"cpp(
+#include "foo.h"
+int main() {}
+  )cpp";
+  auto WithEmptyPreamble = R"cpp(int main() {})cpp";
+  S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  S.runWithPreamble("getNonEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get a non-empty preamble.
+  ASSERT_TRUE(bool(Preamble));
+  EXPECT_GT(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Update the file which results in an empty preamble.
+  S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
+   [](std::vector) {});
+  // Wait for the preamble is being built.
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  S.runWithPreamble("getEmptyPreamble", Foo,
+[&](llvm::Expected Preamble) {
+  // We expect to get an empty preamble.
+  EXPECT_EQ(cantFail(std::move(Preamble))
+.Preamble->Preamble.getBounds()
+.Size,
+0u);
+});
+}
+
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
___
c

[clang-tools-extra] r339866 - Attempt to fix clangd tests on older compilers

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Thu Aug 16 04:41:19 2018
New Revision: 339866

URL: http://llvm.org/viewvc/llvm-project?rev=339866&view=rev
Log:
Attempt to fix clangd tests on older compilers

Old gcc versions of gcc struggle with raw string literals inside macros.

Inspired by rL339759

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

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=339866&r1=339865&r2=339866&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Thu Aug 16 
04:41:19 2018
@@ -1538,36 +1538,39 @@ TEST(SignatureHelpTest, OverloadsOrderin
 }
 
 TEST(SignatureHelpTest, InstantiatedSignatures) {
-  EXPECT_THAT(signatures(R"cpp(
+  StringRef Sig0 = R"cpp(
 template 
 void foo(T, T, T);
 
 int main() {
   foo(^);
 }
-  )cpp")
-  .signatures,
+  )cpp";
+
+  EXPECT_THAT(signatures(Sig0).signatures,
   ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
 
-  EXPECT_THAT(signatures(R"cpp(
+  StringRef Sig1 = R"cpp(
 template 
 void foo(T, T, T);
 
 int main() {
   foo(10, ^);
-})cpp")
-  .signatures,
+})cpp";
+
+  EXPECT_THAT(signatures(Sig1).signatures,
   ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"})));
 
-  EXPECT_THAT(signatures(R"cpp(
+  StringRef Sig2 = R"cpp(
 template 
 void foo(T...);
 
 int main() {
   foo(^);
 }
-  )cpp")
-  .signatures,
+  )cpp";
+
+  EXPECT_THAT(signatures(Sig2).signatures,
   ElementsAre(Sig("foo(T...) -> void", {"T..."})));
 
   // It is debatable whether we should substitute the outer template parameter
@@ -1575,7 +1578,7 @@ TEST(SignatureHelpTest, InstantiatedSign
   // do substitute in code complete.
   // FIXME: make code complete and signature help consistent, figure out which
   // way is better.
-  EXPECT_THAT(signatures(R"cpp(
+  StringRef Sig3 = R"cpp(
 template 
 struct X {
   template 
@@ -1585,8 +1588,9 @@ TEST(SignatureHelpTest, InstantiatedSign
 int main() {
   X().foo(^)
 }
-  )cpp")
-  .signatures,
+  )cpp";
+
+  EXPECT_THAT(signatures(Sig3).signatures,
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 


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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ilya-biryukov wrote:
> ioeric wrote:
> > drive by: I think this should  be `vlog` or `dlog`.
> Code completion also logs the number of results from sema, index, etc. using 
> the `log()` call.
> The added log message looks similar, so trying to be consistent with the rest 
> of the code in this file.
> 
> Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave 
> it to a separate patch.
I'm not sure which level code completion log messages should be in, but I think 
we should follow the guidelines in the logger documentation instead of the 
existing uses.

Quote from Logger.h
```
// log() is used for information important to understanding a clangd session.
// e.g. the names of LSP messages sent are logged at this level.
// This level could be enabled in production builds to allow later inspection.

// vlog() is used for details often needed for debugging clangd sessions.
// This level would typically be enabled for clangd developers.
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D47196: [Time-report ](2): Recursive timers in Clang

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 added a comment.

In https://reviews.llvm.org/D47196#1190377, @efriedma wrote:

> I mean, which of the callers of startFrontendTimer() is calling it with a 
> pointer to std::declval()?


F6951300: declval.log 

The attachment keeps debug output for

../bin/clang -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all 
-disable-free -disable-llvm-verifier -discard-value-names -main-file-name 
TransGCCalls.cpp -mrelocation-model pic -pic-level 2 -mthread-model posix 
-mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases 
-munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info 
-debugger-tuning=gdb -v -v -coverage-notes-file 
/home/atischenko/workspaces/bootstrap/Release-patch/tools/clang/lib/ARCMigrate/CMakeFiles/clangARCMigrate.dir/TransGCCalls.cpp.gcno
 -resource-dir 
/home/atischenko/workspaces/time-report-recursive-4/RelWithDebug/lib/clang/8.0.0
 -D GTEST_HAS_RTTI=0 -D _DEBUG -D _GNU_SOURCE -D __STDC_CONSTANT_MACROS -D 
__STDC_FORMAT_MACROS -D __STDC_LIMIT_MACROS -I 
/home/atischenko/workspaces/bootstrap/Release-patch/tools/clang/lib/ARCMigrate 
-I /home/atischenko/workspaces/bootstrap/llvm/tools/clang/lib/ARCMigrate -I 
/home/atischenko/workspaces/bootstrap/llvm/tools/clang/include -I 
/home/atischenko/workspaces/bootstrap/Release-patch/tools/clang/include -I 
/usr/include/libxml2 -I 
/home/atischenko/workspaces/bootstrap/Release-patch/include -I 
/home/atischenko/workspaces/bootstrap/llvm/include -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/x86_64-linux-gnu/c++/5.4.0
 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/x86_64-linux-gnu/c++/5.4.0
 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/backward 
-internal-isystem /usr/local/include -internal-isystem 
/home/atischenko/workspaces/time-report-recursive-4/RelWithDebug/lib/clang/8.0.0/include
 -internal-externc-isystem /usr/include/x86_64-linux-gnu 
-internal-externc-isystem /include -internal-externc-isystem /usr/include 
-Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wstring-conversion -Woverloaded-virtual 
-Wno-nested-anon-types -pedantic -std=c++11 -fdeprecated-macro 
-fdebug-compilation-dir 
/home/atischenko/workspaces/bootstrap/Release-patch/tools/clang/lib/ARCMigrate 
-ferror-limit 19 -fmessage-length 0 -fvisibility-inlines-hidden -ftime-report 
-fno-rtti -fobjc-runtime=gcc -fno-common -fdiagnostics-show-option -o 
TransGCCalls.cpp.o -x c++ 
/home/atischenko/workspaces/bootstrap/llvm/tools/clang/lib/ARCMigrate/TransGCCalls.cpp
 -faddrsig -mllvm -debug-only=ftiming

The full log is too big that's why I grepped "declval" only:

- :start: means the timer was started
- :stop-zero: means the timer was stopped and nothing was added to the 
calculated time period
- :stop-add: means the timer was stoped and the time slice was added to 
the time period.

There are DeclFunc* names and Tags to look for corresponding RAII objects 
inside sources. For example:

ActOnFunctionDeclarator:start:declval:

means

Tag:start:Name:

BTW, I see a lot of broken DeclFunc names: do you have any idea why it's 
possible? Example:

ActOnFunctionDeclarator:start:._exception:

Here  .. means non-ASCII symbols.


https://reviews.llvm.org/D47196



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > drive by: I think this should  be `vlog` or `dlog`.
> > Code completion also logs the number of results from sema, index, etc. 
> > using the `log()` call.
> > The added log message looks similar, so trying to be consistent with the 
> > rest of the code in this file.
> > 
> > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather 
> > leave it to a separate patch.
> I'm not sure which level code completion log messages should be in, but I 
> think we should follow the guidelines in the logger documentation instead of 
> the existing uses.
> 
> Quote from Logger.h
> ```
> // log() is used for information important to understanding a clangd session.
> // e.g. the names of LSP messages sent are logged at this level.
> // This level could be enabled in production builds to allow later inspection.
> 
> // vlog() is used for details often needed for debugging clangd sessions.
> // This level would typically be enabled for clangd developers.
> ```
My recent experience of debugging some weird GotoDef issues tells me that log 
of index should be on production (since it is a non-trivial part in a clangd 
session), it would be really helpful to understand what is going on. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

hokein wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > drive by: I think this should  be `vlog` or `dlog`.
> > > Code completion also logs the number of results from sema, index, etc. 
> > > using the `log()` call.
> > > The added log message looks similar, so trying to be consistent with the 
> > > rest of the code in this file.
> > > 
> > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather 
> > > leave it to a separate patch.
> > I'm not sure which level code completion log messages should be in, but I 
> > think we should follow the guidelines in the logger documentation instead 
> > of the existing uses.
> > 
> > Quote from Logger.h
> > ```
> > // log() is used for information important to understanding a clangd 
> > session.
> > // e.g. the names of LSP messages sent are logged at this level.
> > // This level could be enabled in production builds to allow later 
> > inspection.
> > 
> > // vlog() is used for details often needed for debugging clangd sessions.
> > // This level would typically be enabled for clangd developers.
> > ```
> My recent experience of debugging some weird GotoDef issues tells me that log 
> of index should be on production (since it is a non-trivial part in a clangd 
> session), it would be really helpful to understand what is going on. 
I agree that they are useful for debugging, but they might not be interesting 
to end-users. And I think that's why there is `vlog`. Clangd developers could 
use a different log level with `--log` flag.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeCompletionStrings.cpp:81
+std::string
+getDocComment(const ASTContext &Ctx,
+  const CodeCompleteConsumer::OverloadCandidate &Overload,

The function doesn't seem to carry its weight, and the difference from the 
other overload is a bit subtle. Maybe we could expose `getDeclComent` instead?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726



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

[libunwind] r339848 - [libunwind][mips] Modify the __mips_fpr macro check

[Stefan Maksimovic via cfe-commits]

Author: smaksimovic
Date: Thu Aug 16 01:47:43 2018
New Revision: 339848

URL: http://llvm.org/viewvc/llvm-project?rev=339848&view=rev
Log:
[libunwind][mips] Modify the __mips_fpr macro check

The __mips_fpr macro can take the value of 0 as well, change to account for 
that case.

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

Modified:
libunwind/trunk/src/UnwindRegistersRestore.S
libunwind/trunk/src/UnwindRegistersSave.S

Modified: libunwind/trunk/src/UnwindRegistersRestore.S
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/UnwindRegistersRestore.S?rev=339848&r1=339847&r2=339848&view=diff
==
--- libunwind/trunk/src/UnwindRegistersRestore.S (original)
+++ libunwind/trunk/src/UnwindRegistersRestore.S Thu Aug 16 01:47:43 2018
@@ -815,7 +815,7 @@ DEFINE_LIBUNWIND_PRIVATE_FUNCTION(_ZN9li
   .set noreorder
   .set nomacro
 #ifdef __mips_hard_float
-#if __mips_fpr == 32
+#if __mips_fpr != 64
   ldc1  $f0, (4 * 36 + 8 * 0)($4)
   ldc1  $f2, (4 * 36 + 8 * 2)($4)
   ldc1  $f4, (4 * 36 + 8 * 4)($4)

Modified: libunwind/trunk/src/UnwindRegistersSave.S
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/UnwindRegistersSave.S?rev=339848&r1=339847&r2=339848&view=diff
==
--- libunwind/trunk/src/UnwindRegistersSave.S (original)
+++ libunwind/trunk/src/UnwindRegistersSave.S Thu Aug 16 01:47:43 2018
@@ -168,7 +168,7 @@ DEFINE_LIBUNWIND_FUNCTION(unw_getcontext
   mflo  $8
   sw$8,  (4 * 34)($4)
 #ifdef __mips_hard_float
-#if __mips_fpr == 32
+#if __mips_fpr != 64
   sdc1  $f0, (4 * 36 + 8 * 0)($4)
   sdc1  $f2, (4 * 36 + 8 * 2)($4)
   sdc1  $f4, (4 * 36 + 8 * 4)($4)


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

[libunwind] r339849 - [libunwind][mips] Guard accumulator registers

[Stefan Maksimovic via cfe-commits]

Author: smaksimovic
Date: Thu Aug 16 01:49:50 2018
New Revision: 339849

URL: http://llvm.org/viewvc/llvm-project?rev=339849&view=rev
Log:
[libunwind][mips] Guard accumulator registers

Mipsr6 does not possess HI and LO accumulator registers, adjust validRegister 
functions to respect that.

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

Modified:
libunwind/trunk/src/Registers.hpp

Modified: libunwind/trunk/src/Registers.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/Registers.hpp?rev=339849&r1=339848&r2=339849&view=diff
==
--- libunwind/trunk/src/Registers.hpp (original)
+++ libunwind/trunk/src/Registers.hpp Thu Aug 16 01:49:50 2018
@@ -2759,10 +2759,12 @@ inline bool Registers_mips_o32::validReg
 return false;
   if (regNum <= UNW_MIPS_R31)
 return true;
+#if __mips_isa_rev != 6
   if (regNum == UNW_MIPS_HI)
 return true;
   if (regNum == UNW_MIPS_LO)
 return true;
+#endif
 #if defined(__mips_hard_float) && __mips_fpr == 32
   if (regNum >= UNW_MIPS_F0 && regNum <= UNW_MIPS_F31)
 return true;
@@ -3073,10 +3075,12 @@ inline bool Registers_mips_newabi::valid
 return false;
   if (regNum <= UNW_MIPS_R31)
 return true;
+#if __mips_isa_rev != 6
   if (regNum == UNW_MIPS_HI)
 return true;
   if (regNum == UNW_MIPS_LO)
 return true;
+#endif
   // FIXME: Hard float, DSP accumulator registers, MSA registers
   return false;
 }


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

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

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

I Will put `NumExceptions` in a trailing object. However it seems
that `FunctionProtoType` can be substantially cleaned up by converting
it to use `llvm::TrailingObjects` instead of manually doing the 
casts+arithmetic.

Therefore I plan to first convert `FunctionProtoType` to use 
`llvm::TrailingObjects`
and then resubmit this patch with the appropriate modifications.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

(I defer to Hans)


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);

JonasToth wrote:
> hugoeg wrote:
> > deannagarcia wrote:
> > > aaron.ballman wrote:
> > > > hokein wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this needs a `not(isExpansionInSystemHeader())` in there, 
> > > > > > as this is going to trigger on code that includes a header file 
> > > > > > using an `absl` namespace. If I'm incorrect and users typically 
> > > > > > include abseil as something other than system includes, you'll have 
> > > > > > to find a different way to solve this.
> > > > > Yeah, we have discussed this issue internally.  abseil-user code 
> > > > > usually includes abseil header like `#include 
> > > > > "whatever/abseil/base/optimization.h"`, so 
> > > > > `IsExpansionInSystemHeader` doesn't work for most cases. 
> > > > > 
> > > > > We need a way to filter out all headers being a part of abseil 
> > > > > library. I think we can create a matcher `InExpansionInAbseilHeader` 
> > > > > -- basically using `IsExpansionInFileMatching` with a regex 
> > > > > expression that matches typical abseil include path. What do you 
> > > > > think?
> > > > > 
> > > > > And we'll have more abseil checks (e.g.  `abseil-no-internal-deps`) 
> > > > > that use `InExpansionInAbseilHeader`, we should put it to a common 
> > > > > header.
> > > > I think that is a sensible approach.
> > > We will begin working on this, I think it will first be implemented in 
> > > abseil-no-internal-deps but once it looks good I will add it to this 
> > > patch.
> > I'm going to go ahead a implement this in ASTMatchers.h and include it on 
> > the patch for **abseil-no-internal-dep**s
> In principle it is ok, but I don't think ASTMatchers.h is the correct place, 
> because it is only of use to clang-tidy.
> 
> There is a `util` directory in clang-tidy for this kind of stuff. Defining 
> you own matchers works the same as in ASTMatchers, you can grep through 
> clang-tidy checks (e.g. `AST_MATCHER`) to have some examples of private 
> matchers.
`ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. We 
 have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific matchers. 
I'm not sure whether it is reasonable to put abseil-specific matchers there. 
Maybe we create a `AbseilMatcher.h` file in `clang-tidy/abseil` directory?


https://reviews.llvm.org/D50580



___
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 a comment.

@hugoeg, it looks like you are waiting for review, but this patch doesn't 
include the `IsExpansionInAbseilHeader` matcher. What's your plan of it?




Comment at: test/clang-tidy/abseil-fake-declarations.h:1
+namespace std {
+struct string {

I'd expect this header file is used as as a real absl library file:

* create an `absl` directory in `test/clang-tidy/Inputs/`, and this directory 
is the abseil source root directory
* use the real absl function/class names instead of some fake names in the 
header, e.g. this file could be `test/clang-tidy/Inputs/absl/strings/str_cat.h`.

This would make the lit test align with the real world case, and it could be 
helpful if you implement the `IsExpansionInAbseilHeader` matcher in this check. 



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:19
+namespace absl {
+  void OpeningNamespace() {
+strings_internal::InternalFunction();

the style doesn't looks correct to me.


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] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > drive by: I think this should  be `vlog` or `dlog`.
> > > > Code completion also logs the number of results from sema, index, etc. 
> > > > using the `log()` call.
> > > > The added log message looks similar, so trying to be consistent with 
> > > > the rest of the code in this file.
> > > > 
> > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather 
> > > > leave it to a separate patch.
> > > I'm not sure which level code completion log messages should be in, but I 
> > > think we should follow the guidelines in the logger documentation instead 
> > > of the existing uses.
> > > 
> > > Quote from Logger.h
> > > ```
> > > // log() is used for information important to understanding a clangd 
> > > session.
> > > // e.g. the names of LSP messages sent are logged at this level.
> > > // This level could be enabled in production builds to allow later 
> > > inspection.
> > > 
> > > // vlog() is used for details often needed for debugging clangd sessions.
> > > // This level would typically be enabled for clangd developers.
> > > ```
> > My recent experience of debugging some weird GotoDef issues tells me that 
> > log of index should be on production (since it is a non-trivial part in a 
> > clangd session), it would be really helpful to understand what is going on. 
> I agree that they are useful for debugging, but they might not be interesting 
> to end-users. And I think that's why there is `vlog`. Clangd developers could 
> use a different log level with `--log` flag.
I agree that `vlog` is probably a better fit here, but still think we should 
change it across `CodeComplete.cpp` in a single commit, rather than partially 
apply the guidelines to different parts of the file.

However, if everyone agrees we should use `vlog` here, happy to use `vlog` too 
and also send a patch to update all the rest of the usages.
The situation I'm trying to avoid is:
1. We commit `vlog` here.
2. Someone argues that using `log` is actually better and we decide to not 
change other usages to `log`.
3. We end up with inconsistent choices across this file: `vlog` for signature 
help and `log` for code completion.

But if there's an agreement that we should use `vlog` everywhere, more than 
happy to change it :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339874: [libcxx] By default, do not use internal_linkage to 
hide symbols from the ABI (authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50652?vs=160873&id=161009#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50652

Files:
  libcxx/trunk/CMakeLists.txt
  libcxx/trunk/docs/BuildingLibcxx.rst
  libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
  libcxx/trunk/include/__config
  libcxx/trunk/include/__config_site.in
  libcxx/trunk/utils/libcxx/test/config.py

Index: libcxx/trunk/CMakeLists.txt
===
--- libcxx/trunk/CMakeLists.txt
+++ libcxx/trunk/CMakeLists.txt
@@ -120,6 +120,7 @@
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the Microsoft ABI.")
+option(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT "Enable per TU ABI insulation by default. To be used by vendors." OFF)
 set(LIBCXX_ABI_DEFINES "" CACHE STRING "A semicolon separated list of ABI macros to define in the site config header.")
 option(LIBCXX_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 
@@ -662,6 +663,7 @@
 config_define_if(LIBCXX_ABI_UNSTABLE _LIBCPP_ABI_UNSTABLE)
 config_define_if(LIBCXX_ABI_FORCE_ITANIUM _LIBCPP_ABI_FORCE_ITANIUM)
 config_define_if(LIBCXX_ABI_FORCE_MICROSOFT _LIBCPP_ABI_FORCE_MICROSOFT)
+config_define_if(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT)
 
 config_define_if_not(LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE)
 config_define_if_not(LIBCXX_ENABLE_STDIN _LIBCPP_HAS_NO_STDIN)
Index: libcxx/trunk/utils/libcxx/test/config.py
===
--- libcxx/trunk/utils/libcxx/test/config.py
+++ libcxx/trunk/utils/libcxx/test/config.py
@@ -677,7 +677,8 @@
 if feature_macros[m]:
 define += '=%s' % (feature_macros[m])
 self.cxx.compile_flags += [define]
-if m == '_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS':
+if m == '_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS' or \
+   m == '_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT':
 continue
 if m == '_LIBCPP_ABI_VERSION':
 self.config.available_features.add('libcpp-abi-version-v%s'
Index: libcxx/trunk/docs/BuildingLibcxx.rst
===
--- libcxx/trunk/docs/BuildingLibcxx.rst
+++ libcxx/trunk/docs/BuildingLibcxx.rst
@@ -332,6 +332,15 @@
   Use the specified GCC toolchain and standard library when building the native
   stdlib benchmark tests.
 
+.. option:: LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT:BOOL
+
+  **Default**: ``OFF``
+
+  Pick the default for whether to constrain ABI-unstable symbols to
+  each individual translation unit. This setting controls whether
+  `_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT` is defined by default --
+  see the documentation of that macro for details.
+
 
 libc++ ABI Feature Options
 --
Index: libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
@@ -42,9 +42,7 @@
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,41 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI with `_LIBCPP_HIDE_FROM_ABI`
+  are local to each translation unit in addition to being local to each final
+  linked image. This macro is defined to either 0 or 1. When it is defined to
+  1, translation units compiled with different versions of libc++ can be linked
+  together, since all non ABI-facing functions are local to each translation unit.
+  This allows static archives built with different versions of libc++ to be linked
+  together. This also means that functions marked with `_LIBCPP_HIDE_FROM_ABI`
+  are not guaranteed to have the same address across translation unit boundaries.
+
+  When the macro is defined to 0, there is no guarantee that translation units
+  compiled with di

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ilya-biryukov wrote:
> ioeric wrote:
> > hokein wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > ioeric wrote:
> > > > > > drive by: I think this should  be `vlog` or `dlog`.
> > > > > Code completion also logs the number of results from sema, index, 
> > > > > etc. using the `log()` call.
> > > > > The added log message looks similar, so trying to be consistent with 
> > > > > the rest of the code in this file.
> > > > > 
> > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd 
> > > > > rather leave it to a separate patch.
> > > > I'm not sure which level code completion log messages should be in, but 
> > > > I think we should follow the guidelines in the logger documentation 
> > > > instead of the existing uses.
> > > > 
> > > > Quote from Logger.h
> > > > ```
> > > > // log() is used for information important to understanding a clangd 
> > > > session.
> > > > // e.g. the names of LSP messages sent are logged at this level.
> > > > // This level could be enabled in production builds to allow later 
> > > > inspection.
> > > > 
> > > > // vlog() is used for details often needed for debugging clangd 
> > > > sessions.
> > > > // This level would typically be enabled for clangd developers.
> > > > ```
> > > My recent experience of debugging some weird GotoDef issues tells me that 
> > > log of index should be on production (since it is a non-trivial part in a 
> > > clangd session), it would be really helpful to understand what is going 
> > > on. 
> > I agree that they are useful for debugging, but they might not be 
> > interesting to end-users. And I think that's why there is `vlog`. Clangd 
> > developers could use a different log level with `--log` flag.
> I agree that `vlog` is probably a better fit here, but still think we should 
> change it across `CodeComplete.cpp` in a single commit, rather than partially 
> apply the guidelines to different parts of the file.
> 
> However, if everyone agrees we should use `vlog` here, happy to use `vlog` 
> too and also send a patch to update all the rest of the usages.
> The situation I'm trying to avoid is:
> 1. We commit `vlog` here.
> 2. Someone argues that using `log` is actually better and we decide to not 
> change other usages to `log`.
> 3. We end up with inconsistent choices across this file: `vlog` for signature 
> help and `log` for code completion.
> 
> But if there's an agreement that we should use `vlog` everywhere, more than 
> happy to change it :-)
That sounds good to me. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[libcxx] r339874 - [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via cfe-commits]

Author: ldionne
Date: Thu Aug 16 05:44:28 2018
New Revision: 339874

URL: http://llvm.org/viewvc/llvm-project?rev=339874&view=rev
Log:
[libcxx] By default, do not use internal_linkage to hide symbols from the ABI

Summary:
https://reviews.llvm.org/D49240 led to symbol size problems in Chromium, and
we expect this may be the case in other projects built in debug mode too.
Instead, unless users explicitly ask for internal_linkage, we use always_inline
like we used to.

In the future, when we have a solution that allows us to drop always_inline
without falling back on internal_linkage, we can replace always_inline by
that.

Note that this commit introduces a change in contract for existing libc++
users: by default, libc++ used to guarantee that TUs built with different
versions of libc++ could be linked together. With the introduction of the
_LIBCPP_HIDE_FROM_ABI_PER_TU macro, the default behavior is that TUs built
with different libc++ versions are not guaranteed to link. This is a change
in contract but not a change in behavior, since the current implementation
still allows linking TUs built with different libc++ versions together.

Reviewers: EricWF, mclow.lists, dexonsmith, hans, rnk

Subscribers: christof, cfe-commits

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

Modified:
libcxx/trunk/CMakeLists.txt
libcxx/trunk/docs/BuildingLibcxx.rst
libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
libcxx/trunk/include/__config
libcxx/trunk/include/__config_site.in
libcxx/trunk/utils/libcxx/test/config.py

Modified: libcxx/trunk/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/CMakeLists.txt?rev=339874&r1=339873&r2=339874&view=diff
==
--- libcxx/trunk/CMakeLists.txt (original)
+++ libcxx/trunk/CMakeLists.txt Thu Aug 16 05:44:28 2018
@@ -120,6 +120,7 @@ set(LIBCXX_ABI_VERSION ${DEFAULT_ABI_VER
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the 
Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the 
Microsoft ABI.")
+option(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT "Enable per TU ABI insulation by 
default. To be used by vendors." OFF)
 set(LIBCXX_ABI_DEFINES "" CACHE STRING "A semicolon separated list of ABI 
macros to define in the site config header.")
 option(LIBCXX_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 
@@ -662,6 +663,7 @@ endif()
 config_define_if(LIBCXX_ABI_UNSTABLE _LIBCPP_ABI_UNSTABLE)
 config_define_if(LIBCXX_ABI_FORCE_ITANIUM _LIBCPP_ABI_FORCE_ITANIUM)
 config_define_if(LIBCXX_ABI_FORCE_MICROSOFT _LIBCPP_ABI_FORCE_MICROSOFT)
+config_define_if(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT 
_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT)
 
 config_define_if_not(LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE 
_LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE)
 config_define_if_not(LIBCXX_ENABLE_STDIN _LIBCPP_HAS_NO_STDIN)

Modified: libcxx/trunk/docs/BuildingLibcxx.rst
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/BuildingLibcxx.rst?rev=339874&r1=339873&r2=339874&view=diff
==
--- libcxx/trunk/docs/BuildingLibcxx.rst (original)
+++ libcxx/trunk/docs/BuildingLibcxx.rst Thu Aug 16 05:44:28 2018
@@ -332,6 +332,15 @@ libc++ Feature Options
   Use the specified GCC toolchain and standard library when building the native
   stdlib benchmark tests.
 
+.. option:: LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT:BOOL
+
+  **Default**: ``OFF``
+
+  Pick the default for whether to constrain ABI-unstable symbols to
+  each individual translation unit. This setting controls whether
+  `_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT` is defined by default --
+  see the documentation of that macro for details.
+
 
 libc++ ABI Feature Options
 --

Modified: libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst?rev=339874&r1=339873&r2=339874&view=diff
==
--- libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst (original)
+++ libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst Thu Aug 16 05:44:28 2018
@@ -42,9 +42,7 @@ Visibility Macros
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,41 @@ Visibility Macros
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the A

[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Oh, I thought that what everyone wanted was test-specific behaviour. I like 
both approaches you propose much more!
If we go for the generic option we can effectively start "checking stderr" in 
tests by using the flag. That would be nice.

Just to be sure we are all on the same page:

1. Do we want to exit right after elog() call?
2. Do we consider calling exit() in elog() smart or hacky?

Based on quick grepping if we'd like to propagate errors probably the biggest 
challenge would be ASTWorker.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50785



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 161011.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- Expose getDeclComment instead of getDocComment


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726

Files:
  clangd/CodeComplete.cpp
  clangd/CodeCompletionStrings.cpp
  clangd/CodeCompletionStrings.h


Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18 +32,8 @@
   const CodeCompletionResult &Result,
   bool CommentsFromHeaders);
 
-/// Gets a minimally formatted documentation for parameter of \p Result,
-/// corresponding to argument number \p ArgIndex.
-/// This currently looks for comments attached to the parameter itself, and
-/// doesn't extract them from function documentation.
-/// Returns empty string when no comment is available.
-/// If \p CommentsFromHeaders parameter is set, only comments from the main
-/// file will be returned. It is used to workaround crashes when parsing
-/// comments in the stale headers, coming from completion preamble.
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders);
+/// Similar to getDocComment, but returns the comment for a NamedDecl.
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &D);
 
 /// Formats the signature for an item, as a display string and snippet.
 /// e.g. for const_reference std::vector::at(size_type) const, this returns:
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -52,34 +52,20 @@
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";
   auto *Decl = Result.getDeclaration();
-  if (!Decl || llvm::isa(Decl)) {
+  if (!Decl)
+return "";
+  return getDeclComment(Ctx, *Decl);
+}
+
+std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
+  if (llvm::isa(Decl)) {
 // Namespaces often have too many redecls for any particular redecl comment
 // to be useful. Moreover, we often confuse file headers or generated
 // comments with namespace comments. Therefore we choose to just ignore
 // the comments for namespaces.
 return "";
   }
-  const RawComment *RC = getCompletionComment(Ctx, Decl);
-  if (!RC)
-return "";
-
-  // Sanity check that the comment does not come from the PCH. We choose to not
-  // write them into PCH, because they are racy and slow to load.
-  assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
-  std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), 
Ctx.getDiagnostics());
-  if (!looksLikeDocComment(Doc))
-return "";
-  return Doc;
-}
-
-std::string
-getParameterDocComment(const ASTContext &Ctx,
-   const CodeCompleteConsumer::OverloadCandidate &Result,
-   unsigned ArgIndex, bool CommentsFromHeaders) {
-  auto *Func = Result.getFunction();
-  if (!Func)
-return "";
-  const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
+  const RawComment *RC = getCompletionComment(Ctx, &Decl);
   if (!RC)
 return "";
   // Sanity check that the comment does not come from the PCH. We choose to not
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -729,8 +729,9 @@
   // FIXME: for headers, we need to get a comment from the index.
   ScoredSignatures.push_back(processOverloadCandidate(
   Candidate, *CCS,
-  getParameterDocComment(S.getASTContext(), Candidate, CurrentArg,
- /*CommentsFromHeaders=*/false)));
+  Candidate.getFunction()
+  ? getDeclComment(S.getASTContext(), *Candidate.getFunction())
+  : ""));
 }
 std::sort(ScoredSignatures.begin(), ScoredSignatures.end(),
   [](const ScoredSignature &L, const ScoredSignature &R) {


Index: clangd/CodeCompletionStrings.h
===
--- clangd/CodeCompletionStrings.h
+++ clangd/CodeCompletionStrings.h
@@ -32,18 +32,8 @@
   const CodeCompletionResult &Result,
   bool CommentsFromHeaders);
 
-/// Gets a minimally formatted documentation for parameter of \p Result,
-/// corresponding to argument number \p ArgIndex.
-/// This currently looks for comments attached to the parameter itself, and
-/// doesn't extract them from function documentation.
-/// Returns empty string when no comment is available.
-/// If \p CommentsFromHeaders parameter is set, only comments from the main
-/// file will

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added reviewers: ilya-biryukov, ioeric.
Herald added a reviewer: javed.absar.
Herald added a subscriber: kristof.beyls.

This patch significantly improves performance of the YAML serializer by 
optimizing `YAML::isNumeric` function. This function is called on the most 
strings and is highly inefficient for two reasons:

- It uses `Regex`, which is parsed and compiled each time this function is 
called
- It uses multiple passes which are not necessary

This patch introduces stateful ad hoc YAML number parser which does not rely on 
`Regex`. It also fixes YAML number format inconsistency: current implementation 
supports C-stile octal number format (`01234567`) which was present in YAML 1.0 
specialization (http://yaml.org/spec/1.0/), [Section 2.4. Tags, Example 2.19] 
but was deprecated and is no longer present in latest YAML 1.2 specification 
(http://yaml.org/spec/1.2/spec.html), see [Section 10.3.2. Tag Resolution]. 
Since the rest of the rest of the implementation does not support other 
deprecated YAML 1.0 numeric features such as sexagecimal numbers, commas as 
delimiters it is treated as inconsistency and not longer supported.

This performance bottleneck was identified while profiling Clangd's 
global-symbol-builder tool with my colleague @ilya-biryukov. The substantial 
part of the runtime was spent during a single-thread Reduce phase, which 
concludes with YAML serialization of collected symbol collection. Regex 
matching was accountable for approximately 45% of the whole runtime (which 
involves sharded Map phase), now it is reduced to 18% (which is spent in 
`clang::clangd::CanonicalIncludes` and can be also optimized because all used 
regexes are in fact either suffix matches or exact matches).

Benchmarking `global-symbol-builder` (using `hyperfine --warmup 2 --min-runs 5 
'command 1' 'command 2'` tool by processing a reasonable amount of code (26 
source files matched by `clang-tools-extra/clangd/*.cpp` with all transitive 
includes) confirmed our understanding of the performance bottleneck nature as 
it speeds up the command by the factor of 1.6x:

| Command| Mean [s]| Min…Max [s] |
| :---   | ---:| ---:|
| patch  | 84.7 ± 0.6  | 83.3…84.7   |
| master (https://reviews.llvm.org/rL339849) | 133.1 ± 0.8 | 132.4…134.6 |
|

Using smaller samples (e.g. by collecting symbols from 
`clang-tools-extra/clangd/AST.cpp` only) yields even better performance 
improvement, which is expected because Map phase takes less time compared to 
Reduce and is 2.05x faster:

| Command| Mean [ms]  | Min…Max [ms]  |
| :---   | ---:   | ---:  |
| patch  | 7607.6 ± 109.5 | 7533.3…7796.4 |
| master (https://reviews.llvm.org/rL339849) | 3702.2 ± 48.7  | 3635.1…3752.3 |


https://reviews.llvm.org/D50839

Files:
  llvm/include/llvm/Support/YAMLTraits.h
  llvm/unittests/Support/YAMLIOTest.cpp

Index: llvm/unittests/Support/YAMLIOTest.cpp
===
--- llvm/unittests/Support/YAMLIOTest.cpp
+++ llvm/unittests/Support/YAMLIOTest.cpp
@@ -16,16 +16,17 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using llvm::yaml::Hex16;
+using llvm::yaml::Hex32;
+using llvm::yaml::Hex64;
+using llvm::yaml::Hex8;
 using llvm::yaml::Input;
-using llvm::yaml::Output;
 using llvm::yaml::IO;
-using llvm::yaml::MappingTraits;
+using llvm::yaml::isNumeric;
 using llvm::yaml::MappingNormalization;
+using llvm::yaml::MappingTraits;
+using llvm::yaml::Output;
 using llvm::yaml::ScalarTraits;
-using llvm::yaml::Hex8;
-using llvm::yaml::Hex16;
-using llvm::yaml::Hex32;
-using llvm::yaml::Hex64;
 using ::testing::StartsWith;
 
 
@@ -2569,3 +2570,63 @@
 TestEscaped((char const *)foobar, "\"foo\\u200Bbar\"");
   }
 }
+
+TEST(YAMLIO, Numeric) {
+  EXPECT_TRUE(isNumeric(".inf"));
+  EXPECT_TRUE(isNumeric(".INF"));
+  EXPECT_TRUE(isNumeric(".Inf"));
+  EXPECT_TRUE(isNumeric("-.inf"));
+  EXPECT_TRUE(isNumeric("+.inf"));
+
+  EXPECT_TRUE(isNumeric(".nan"));
+  EXPECT_TRUE(isNumeric(".NaN"));
+  EXPECT_TRUE(isNumeric(".NAN"));
+
+  EXPECT_TRUE(isNumeric("0"));
+  EXPECT_TRUE(isNumeric("0."));
+  EXPECT_TRUE(isNumeric("0.0"));
+  EXPECT_TRUE(isNumeric("-0.0"));
+  EXPECT_TRUE(isNumeric("+0.0"));
+
+  EXPECT_TRUE(isNumeric("+12.0"));
+  EXPECT_TRUE(isNumeric(".5"));
+  EXPECT_TRUE(isNumeric("+.5"));
+  EXPECT_TRUE(isNumeric("-1.0"));
+
+  EXPECT_TRUE(isNumeric("2.3e4"));
+  EXPECT_TRUE(isNumeric("-2E+05"));
+  EXPECT_TRUE(isNumeric("+12e03"));
+  EXPECT_TRUE(isNumeric("6.8523015e+5"));
+
+  EXPECT_TRUE(isNumeric("1.e+1"));
+  EXPECT_TRUE(isNumeric(".0e+1"));
+
+  EXPECT_TRUE(isNumeric("0x2aF3"));
+  EXPECT_TRUE(isNumeric("0o01234567"));
+
+  EXPECT_FALSE(isNumeric("not a number"));
+  EXPECT_FALSE(isNumeric("."));
+  EXPECT_FALSE(isNumeric(".e+1"));
+  EXPECT_FALSE(isNumeric(".1e"));
+  EXPECT_FALSE(isNumeric(".1e+"));
+  EXPECT_FALSE(isNumeric(".1e++1"));
+
+  EXPECT_FALSE(isNumeric("+0x2A

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.h:85
+
+  /// Enables cursor to be moved around according to completions needs even 
when
+  /// snippets are disabled. For example selecting a function with parameters 
as

IIRC, the goal of this patch is to allow disabling snippet templates for 
function parameters while still allow appending `()` or `($0)` to function 
candidates. If that's still the case, I think what we want is probably an 
option like `DisableSnippetTemplateForFunctionArgs`? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: llvm/include/llvm/Support/YAMLTraits.h:453
 
-inline bool isNumber(StringRef S) {
-  static const char OctalChars[] = "01234567";
-  if (S.startswith("0") &&
-  S.drop_front().find_first_not_of(OctalChars) == StringRef::npos)
-return true;
-
-  if (S.startswith("0o") &&
-  S.drop_front(2).find_first_not_of(OctalChars) == StringRef::npos)
-return true;
+inline bool isNumeric(StringRef S) {
+  if (S.empty())

Passing-by thought, feel free to ignore.

Changes like these are a **great** targets for fuzzers.
Don't just rewrite the implementation, but instead write a new [optimized] 
function,
and add a fuzzer that would feed both of these functions **the same** input,
and assert the equality of their outputs. (and that neither of them crashes).

Would preserve the infinitely more readable code version, too.


https://reviews.llvm.org/D50839



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

[PATCH] D50689: [clangd] NFC: Improve Dex Iterators debugging traits

[Eric Liu via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:102
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDX}, ... END]" where IDN is N-th
+  /// PostingList entry and IDX is the one currently being pointed to by the

nit: I think *IDX* might look a bit nicer. But up to you :)


https://reviews.llvm.org/D50689



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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50385#1193600, @ioeric wrote:

> In https://reviews.llvm.org/D50385#1193545, @hokein wrote:
>
> > In https://reviews.llvm.org/D50385#1191914, @ioeric wrote:
> >
> > > 2 high-level questions:
> > >
> > > 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could 
> > > store occurrences as extra payload of `Symbol`?
> >
> >
> > Storing occurrences in `Symbol` structure is easy to misuse by users IMO -- 
> > if we go through this way, we will end up having a `getOccurrences`-like 
> > method in `Symbol` structure. Once users get the `Symbol` instance, it is 
> > natural for them to call `getOccurrences` to get all occurrences of the 
> > symbol. However this `getOccurrences` method doesn't do what users expected 
> > (just returning an incomplete set of results or empty). To query the symbol 
> > occurrences, we should always use index interface.
> >
> > Therefore, I think we should try to avoid these confusions in the design.
>
>
> Hmm, I think this is the same for other symbol payload e.g. definition can be 
> missing for a symbol. And it seems to me that the concern is on the 
> SymbolSlab level: if a slab is for a single TU, users should expect missing 
> information; if a slab is merged from all TUs, then users can expect 
> "complete" information. I think it's reasonable to assume that users of 
> SymbolSlab are aware of this. I think it's probably not worth the overhead of 
> maintaining and using two separate slabs.


I think it's reasonable to keep occurrences away from Symbol's Detail field. 
Stashing them together is only fine for the collector API, having any way to 
directly access occurrences through `Symbol` will be totally confusing for all 
the other users.
E.g., the `Index::lookup()` will not provide occurrences in the `Symbol` 
instances it returns, and if the accessors for those will be there it will only 
add confusion. So +1 to keeping them out of the `Symbol` class.

On the other hand, `SymbolSlab` feels like a perfectly reasonable place to 
store the occurrences in addition to the symbols themselves and it feels we 
should reuse its memory arena for storing any strings we need to allocate, etc.

>>> 2. Could we merge `SymbolOccurrenceCollector` into the existing 
>>> `SymbolCollector`? They look a lot alike. Having another index data 
>>> consumer seems like more overhead on the user side.
>> 
>> The `SymbolOccurrenceCollector` has many responsibilities (collecting 
>> declaration, definition, code completion information etc), and the code is 
>> growing complex now. Merging the `SymbolOccurrenceCollector` to it will make 
>> it more
> 
> Although the existing `SymbolCollector` supports different options, I think 
> it still has a pretty well-defined responsibility: gather information about 
> symbols. IMO, cross-reference is one of the property of symbol, and I don't 
> see strong reasons to keep them separated.
> 
>> complicated -- we will introduce more option flags like 
>> `collect-symbol-only`, `collect-occurrence-only` to configure it for our 
>> different use cases (we need to the implementation detail clearly in order 
>> to make a correct option for `SymbolCollector`).
> 
> I think these options are reasonable if they turn out to be necessary. And 
> making the SymbolCollector more complicated doesn't seem to be a problem if 
> we are indeed doing more complicated work, but I don't think this would turn 
> into a big problem as logic of xrefs seems pretty isolated.  Conversely, I 
> think implementing xrefs in a separate class would likely to cause more 
> duplicate and maintenance, e.g. two sets of options, two sets of 
> initializations or life-time tracking of collectors (they look a lot alike), 
> the same boilerplate factory code in tests, passing around two collectors in 
> user code.
> 
>> And I can foresee these two collectors might be run at different point 
>> (runWithPreamble vs runWithAST) in dynamic index.
> 
> With some options, this should be a problem I think?

+1 to merging into the `SymbolCollector`. Keeping the responsibilities separate 
inside a single class should be easy, e.g. something like that should be simple 
enough:

  SymbolCollector::handleDeclOccurence(args) {
this->processForSymbol(args); // handles keeping the Symbol structure 
up-to-date, i.e. adds definition locations, etc.
this->processForOccurrences(args); // appends occurrences to a list of 
xrefs.
  };

The main advantage that we get is less clang-specific boilerplate. The less 
`IndexDataConsumer`s, `FrontendActionFactory`s, `FrontendAction`s we create, 
the more focused and concise our code is.
And in that case, `SymbolCollector` is already handling those responsibilities 
for us and reusing looks like a good idea.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llv

[PATCH] D50689: [clangd] NFC: Improve Dex Iterators debugging traits

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 161013.
kbobyrev marked an inline comment as done.
kbobyrev added a comment.

Improved wording to prevent confusion: no more `IDX` (which is the one pointed 
to by the iterator) and `IDN`; just mention that the element being pointed to 
is the one enclosed in `{}` braces.


https://reviews.llvm.org/D50689

Files:
  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
@@ -231,13 +231,14 @@
   const PostingList L4 = {0, 1, 5};
   const PostingList L5;
 
-  EXPECT_EQ(llvm::to_string(*(create(L0))), "[4, 7, 8, 20, 42, 100]");
+  EXPECT_EQ(llvm::to_string(*(create(L0))), "[{4}, 7, 8, 20, 42, 100, END]");
 
   auto Nested = createAnd(createAnd(create(L1), create(L2)),
   createOr(create(L3), create(L4), create(L5)));
 
   EXPECT_EQ(llvm::to_string(*Nested),
-"(& (& [1, 3, 5, 8, 9] [1, 5, 7, 9]) (| [0, 5] [0, 1, 5] []))");
+"(& (& [{1}, 3, 5, 8, 9, END] [{1}, 5, 7, 9, END]) (| [0, {5}, "
+"END] [0, {1}, 5, END] [{END}]))");
 }
 
 TEST(DexIndexIterators, Limit) {
Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -99,7 +99,9 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
+  /// PostingList entry and the element which is pointed to by the PostingList
+  /// iterator is enclosed in {} braces.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);
Index: clang-tools-extra/clangd/index/dex/Iterator.cpp
===
--- clang-tools-extra/clangd/index/dex/Iterator.cpp
+++ clang-tools-extra/clangd/index/dex/Iterator.cpp
@@ -49,10 +49,19 @@
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
 OS << '[';
 auto Separator = "";
-for (const auto &ID : Documents) {
-  OS << Separator << ID;
+for (auto It = std::begin(Documents); It != std::end(Documents); ++It) {
+  OS << Separator;
+  if (It == Index)
+OS << '{' << *It << '}';
+  else
+OS << *It;
   Separator = ", ";
 }
+OS << Separator;
+if (Index == std::end(Documents))
+  OS << "{END}";
+else
+  OS << "END";
 OS << ']';
 return OS;
   }


Index: clang-tools-extra/unittests/clangd/DexIndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/DexIndexTests.cpp
+++ clang-tools-extra/unittests/clangd/DexIndexTests.cpp
@@ -231,13 +231,14 @@
   const PostingList L4 = {0, 1, 5};
   const PostingList L5;
 
-  EXPECT_EQ(llvm::to_string(*(create(L0))), "[4, 7, 8, 20, 42, 100]");
+  EXPECT_EQ(llvm::to_string(*(create(L0))), "[{4}, 7, 8, 20, 42, 100, END]");
 
   auto Nested = createAnd(createAnd(create(L1), create(L2)),
   createOr(create(L3), create(L4), create(L5)));
 
   EXPECT_EQ(llvm::to_string(*Nested),
-"(& (& [1, 3, 5, 8, 9] [1, 5, 7, 9]) (| [0, 5] [0, 1, 5] []))");
+"(& (& [{1}, 3, 5, 8, 9, END] [{1}, 5, 7, 9, END]) (| [0, {5}, "
+"END] [0, {1}, 5, END] [{END}]))");
 }
 
 TEST(DexIndexIterators, Limit) {
Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -99,7 +99,9 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
+  /// PostingList entry and the element which is pointed to by the PostingList
+  /// iterator is enclosed in {} braces.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);
Index: clang-tools-extra/clangd/index/dex/Iterator.cpp
===
--- clang-tools-extra/clangd/index/dex/Iterator.cpp
+++ clang-tools-extra/clangd/index/dex/It

[clang-tools-extra] r339877 - [clangd] NFC: Improve Dex Iterators debugging traits

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Thu Aug 16 06:19:43 2018
New Revision: 339877

URL: http://llvm.org/viewvc/llvm-project?rev=339877&view=rev
Log:
[clangd] NFC: Improve Dex Iterators debugging traits

This patch improves `dex::Iterator` string representation by
incorporating the information about the element which is currently being
pointed to by the `DocumentIterator`.

Reviewed by: ioeric

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

Modified:
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/Iterator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp?rev=339877&r1=339876&r2=339877&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp Thu Aug 16 06:19:43 
2018
@@ -49,10 +49,19 @@ public:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
 OS << '[';
 auto Separator = "";
-for (const auto &ID : Documents) {
-  OS << Separator << ID;
+for (auto It = std::begin(Documents); It != std::end(Documents); ++It) {
+  OS << Separator;
+  if (It == Index)
+OS << '{' << *It << '}';
+  else
+OS << *It;
   Separator = ", ";
 }
+OS << Separator;
+if (Index == std::end(Documents))
+  OS << "{END}";
+else
+  OS << "END";
 OS << ']';
 return OS;
   }

Modified: clang-tools-extra/trunk/clangd/index/dex/Iterator.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Iterator.h?rev=339877&r1=339876&r2=339877&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.h (original)
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.h Thu Aug 16 06:19:43 2018
@@ -99,7 +99,9 @@ public:
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
+  /// PostingList entry and the element which is pointed to by the PostingList
+  /// iterator is enclosed in {} braces.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);

Modified: clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp?rev=339877&r1=339876&r2=339877&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp Thu Aug 16 
06:19:43 2018
@@ -231,13 +231,14 @@ TEST(DexIndexIterators, StringRepresenta
   const PostingList L4 = {0, 1, 5};
   const PostingList L5;
 
-  EXPECT_EQ(llvm::to_string(*(create(L0))), "[4, 7, 8, 20, 42, 100]");
+  EXPECT_EQ(llvm::to_string(*(create(L0))), "[{4}, 7, 8, 20, 42, 100, END]");
 
   auto Nested = createAnd(createAnd(create(L1), create(L2)),
   createOr(create(L3), create(L4), create(L5)));
 
   EXPECT_EQ(llvm::to_string(*Nested),
-"(& (& [1, 3, 5, 8, 9] [1, 5, 7, 9]) (| [0, 5] [0, 1, 5] []))");
+"(& (& [{1}, 3, 5, 8, 9, END] [{1}, 5, 7, 9, END]) (| [0, {5}, "
+"END] [0, {1}, 5, END] [{END}]))");
 }
 
 TEST(DexIndexIterators, Limit) {


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

[PATCH] D50689: [clangd] NFC: Improve Dex Iterators debugging traits

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE339877: [clangd] NFC: Improve Dex Iterators debugging 
traits (authored by omtcyfz, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50689?vs=161013&id=161014#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50689

Files:
  clangd/index/dex/Iterator.cpp
  clangd/index/dex/Iterator.h
  unittests/clangd/DexIndexTests.cpp


Index: unittests/clangd/DexIndexTests.cpp
===
--- unittests/clangd/DexIndexTests.cpp
+++ unittests/clangd/DexIndexTests.cpp
@@ -231,13 +231,14 @@
   const PostingList L4 = {0, 1, 5};
   const PostingList L5;
 
-  EXPECT_EQ(llvm::to_string(*(create(L0))), "[4, 7, 8, 20, 42, 100]");
+  EXPECT_EQ(llvm::to_string(*(create(L0))), "[{4}, 7, 8, 20, 42, 100, END]");
 
   auto Nested = createAnd(createAnd(create(L1), create(L2)),
   createOr(create(L3), create(L4), create(L5)));
 
   EXPECT_EQ(llvm::to_string(*Nested),
-"(& (& [1, 3, 5, 8, 9] [1, 5, 7, 9]) (| [0, 5] [0, 1, 5] []))");
+"(& (& [{1}, 3, 5, 8, 9, END] [{1}, 5, 7, 9, END]) (| [0, {5}, "
+"END] [0, {1}, 5, END] [{END}]))");
 }
 
 TEST(DexIndexIterators, Limit) {
Index: clangd/index/dex/Iterator.cpp
===
--- clangd/index/dex/Iterator.cpp
+++ clangd/index/dex/Iterator.cpp
@@ -49,10 +49,19 @@
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
 OS << '[';
 auto Separator = "";
-for (const auto &ID : Documents) {
-  OS << Separator << ID;
+for (auto It = std::begin(Documents); It != std::end(Documents); ++It) {
+  OS << Separator;
+  if (It == Index)
+OS << '{' << *It << '}';
+  else
+OS << *It;
   Separator = ", ";
 }
+OS << Separator;
+if (Index == std::end(Documents))
+  OS << "{END}";
+else
+  OS << "END";
 OS << ']';
 return OS;
   }
Index: clangd/index/dex/Iterator.h
===
--- clangd/index/dex/Iterator.h
+++ clangd/index/dex/Iterator.h
@@ -99,7 +99,9 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
+  /// PostingList entry and the element which is pointed to by the PostingList
+  /// iterator is enclosed in {} braces.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);


Index: unittests/clangd/DexIndexTests.cpp
===
--- unittests/clangd/DexIndexTests.cpp
+++ unittests/clangd/DexIndexTests.cpp
@@ -231,13 +231,14 @@
   const PostingList L4 = {0, 1, 5};
   const PostingList L5;
 
-  EXPECT_EQ(llvm::to_string(*(create(L0))), "[4, 7, 8, 20, 42, 100]");
+  EXPECT_EQ(llvm::to_string(*(create(L0))), "[{4}, 7, 8, 20, 42, 100, END]");
 
   auto Nested = createAnd(createAnd(create(L1), create(L2)),
   createOr(create(L3), create(L4), create(L5)));
 
   EXPECT_EQ(llvm::to_string(*Nested),
-"(& (& [1, 3, 5, 8, 9] [1, 5, 7, 9]) (| [0, 5] [0, 1, 5] []))");
+"(& (& [{1}, 3, 5, 8, 9, END] [{1}, 5, 7, 9, END]) (| [0, {5}, "
+"END] [0, {1}, 5, END] [{END}]))");
 }
 
 TEST(DexIndexIterators, Limit) {
Index: clangd/index/dex/Iterator.cpp
===
--- clangd/index/dex/Iterator.cpp
+++ clangd/index/dex/Iterator.cpp
@@ -49,10 +49,19 @@
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
 OS << '[';
 auto Separator = "";
-for (const auto &ID : Documents) {
-  OS << Separator << ID;
+for (auto It = std::begin(Documents); It != std::end(Documents); ++It) {
+  OS << Separator;
+  if (It == Index)
+OS << '{' << *It << '}';
+  else
+OS << *It;
   Separator = ", ";
 }
+OS << Separator;
+if (Index == std::end(Documents))
+  OS << "{END}";
+else
+  OS << "END";
 OS << ']';
 return OS;
   }
Index: clangd/index/dex/Iterator.h
===
--- clangd/index/dex/Iterator.h
+++ clangd/index/dex/Iterator.h
@@ -99,7 +99,9 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
+  /// PostingList entry and the element which is pointed to by the Postin

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50337#1198914, @kbobyrev wrote:

> Don't separate the logic for "long" and "short" queries: 
> https://reviews.llvm.org/D50517 (https://reviews.llvm.org/rCTE339548) 
> introduced incomplete trigrams which can be used on for "short" queries, too.


Have you forgotten to upload the new revision? :)


https://reviews.llvm.org/D50337



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

[PATCH] D50700: [clangd] Generate better incomplete bigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
 
+  EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"}));
+  EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"}));

I'm not sure if this is correct. If users have explicitly typed `_`, they are 
likely to want a `_` there. You mentioned in the patch summary that users might 
want to match two heads with this. Could you provide an example?


https://reviews.llvm.org/D50700



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Kirill Bobyrev via Phabricator via cfe-commits]

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

Sorry, the last diff was the old one. Should be correct now.


https://reviews.llvm.org/D50337

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/clangd/index/dex/DexIndex.h
  clang-tools-extra/clangd/index/dex/Token.h
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/DexIndexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/TestIndexOperations.cpp
  clang-tools-extra/unittests/clangd/TestIndexOperations.h

Index: clang-tools-extra/unittests/clangd/TestIndexOperations.h
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TestIndexOperations.h
@@ -0,0 +1,57 @@
+//===-- IndexHelpers.h --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_INDEXTESTCOMMON_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_INDEXTESTCOMMON_H
+
+#include "index/Index.h"
+#include "index/Merge.h"
+#include "index/dex/DexIndex.h"
+#include "index/dex/Iterator.h"
+#include "index/dex/Token.h"
+#include "index/dex/Trigram.h"
+
+namespace clang {
+namespace clangd {
+
+Symbol symbol(llvm::StringRef QName);
+
+struct SlabAndPointers {
+  SymbolSlab Slab;
+  std::vector Pointers;
+};
+
+// Create a slab of symbols with the given qualified names as both IDs and
+// names. The life time of the slab is managed by the returned shared pointer.
+// If \p WeakSymbols is provided, it will be pointed to the managed object in
+// the returned shared pointer.
+std::shared_ptr>
+generateSymbols(std::vector QualifiedNames,
+std::weak_ptr *WeakSymbols = nullptr);
+
+// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
+// to the `generateSymbols` above.
+std::shared_ptr>
+generateNumSymbols(int Begin, int End,
+   std::weak_ptr *WeakSymbols = nullptr);
+
+std::string getQualifiedName(const Symbol &Sym);
+
+std::vector match(const SymbolIndex &I,
+   const FuzzyFindRequest &Req,
+   bool *Incomplete = nullptr);
+
+// Returns qualified names of symbols with any of IDs in the index.
+std::vector lookup(const SymbolIndex &I,
+llvm::ArrayRef IDs);
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/unittests/clangd/TestIndexOperations.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TestIndexOperations.cpp
@@ -0,0 +1,89 @@
+//===-- IndexHelpers.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestIndexOperations.h"
+
+namespace clang {
+namespace clangd {
+
+Symbol symbol(llvm::StringRef QName) {
+  Symbol Sym;
+  Sym.ID = SymbolID(QName.str());
+  size_t Pos = QName.rfind("::");
+  if (Pos == llvm::StringRef::npos) {
+Sym.Name = QName;
+Sym.Scope = "";
+  } else {
+Sym.Name = QName.substr(Pos + 2);
+Sym.Scope = QName.substr(0, Pos + 2);
+  }
+  return Sym;
+}
+
+// Create a slab of symbols with the given qualified names as both IDs and
+// names. The life time of the slab is managed by the returned shared pointer.
+// If \p WeakSymbols is provided, it will be pointed to the managed object in
+// the returned shared pointer.
+std::shared_ptr>
+generateSymbols(std::vector QualifiedNames,
+std::weak_ptr *WeakSymbols) {
+  SymbolSlab::Builder Slab;
+  for (llvm::StringRef QName : QualifiedNames)
+Slab.insert(symbol(QName));
+
+  auto Storage = std::make_shared();
+  Storage->Slab = std::move(Slab).build();
+  for (const auto &Sym : Storage->Slab)
+Storage->Pointers.push_back(&Sym);
+  if (WeakSymbols)
+*WeakSymbols = Storage;
+  auto *Pointers = &Storage->Pointers;
+  return {std::move(Storage), Pointers};
+}
+
+// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
+// to the `generateSymbols` above.
+std::shared_ptr>
+generateNumSymbols(int Begin, int End,
+   std::weak_ptr *WeakSymbols) {
+  std::vector Names;
+  for (int i = Begin; i <= End; i++)
+Names.push_back(std::to_string(i));
+  return generateSymbols(Names, WeakSymbols);
+}
+
+std::string getQualifiedName(const Symbol &Sym) {
+  return (Sym.Scope + Sym.Name).str()

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> Hmm, I think this is the same for other symbol payload e.g. definition can be 
> missing for a symbol. And it seems to me that the concern is on the 
> SymbolSlab level: if a slab is for a single TU, users should expect missing 
> information; if a slab is merged from all TUs, then users can expect 
> "complete" information. I think it's reasonable to assume that users of 
> SymbolSlab are aware of this. I think it's probably not worth the overhead of 
> maintaining and using two separate slabs.

My concerns of storing occurrences as an extra payload of `Symbol` are:

- `SymbolSlab` is more like an implementation detail.  Users of `SymbolIndex` 
are not aware of it, they only get `Symbol` objects, so it easily confuses 
users if they see any occurrence-related interface/member in `Symbol`. And we 
will write a looong comment explaining its correct behavior. It'd be better if 
we avoid this confusion in the API level.
- The fields in `Symbol` structure are symbol properties, and could be stored 
in memory. However, occurrences are not, we can't guarantee that.
- It seems that we are coupling `ID`, `Symbol`, `SymbolOccurrence` together: in 
the index implementation, we will go through ID=>Symbol=>Occurrences rather 
than ID=>Occurrences.

> I think these options are reasonable if they turn out to be necessary.

I think they are necessary. For collecting all occurrences for local symbols 
from the AST, we only need symbol occurrence information, other information 
(e.g. declaration&definition location, #include) should be discarded; Index for 
code completion should not collect symbol occurrences.

> And making the SymbolCollector more complicated doesn't seem to be a problem 
> if we are indeed doing more complicated work, but I don't think this would 
> turn into a big problem as logic of xrefs seems pretty isolated.

If xrefs is quite isolated, I think it is a good signal to have a dedicated 
class handling it.

> I think implementing xrefs in a separate class would likely to cause more 
> duplicate and maintenance, e.g. two sets of options, two sets of 
> initializations or life-time tracking of collectors (they look a lot alike), 
> the same boilerplate factory code in tests, passing around two collectors in 
> user code.

Merging xrefs to `SymbolCollector` couldn't avoid these problems, I think it is 
a matter of where we put these code:

- different initialization of `SymbolCollector` for different use cases (e.g. 
setting different flags in SymbolCollectorOptions).
- for dynamic index, index for xrefs and code completion would be triggered at 
different point: index for xrefs should happen when AST is ready; index for 
code completion happens when Preamble is ready;  we might end up with two slabs 
instances in the dynamic index (1 symbol slab + 1 occurrence slab vs. 2 symbol 
slabs).

The duplication is mainly about AST frontend action boilerplate code. To 
eliminate it, we could do some refactorings:

- get rid of the clang ast action code in SymbolCollector, and 
SymbolOccurrenceCollector
- introduce an `IndexSymbol` which is a subclass `index::IndexDataConsumer`
- the `IndexSymbol` has two mode (indexing symbol or indexing occurrence),  and 
dispatch ast information to `SymbolCollector`/`SymbolOccurrenceCollector`.


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] D50700: [clangd] Generate better incomplete bigrams for the Dex index

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
 
+  EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"}));
+  EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"}));

ioeric wrote:
> I'm not sure if this is correct. If users have explicitly typed `_`, they are 
> likely to want a `_` there. You mentioned in the patch summary that users 
> might want to match two heads with this. Could you provide an example?
The particular example I had on my mind was `unique_ptr`. Effectively, if the 
query is `SC` then `StrCat` would be matched (because the incomplete trigram 
would be `sc$` and two heads from the symbol identifier would also yield 
`sc$`), but for `u_p`, `unique_ptr` is currently not matched.

On the other hand, if there's something like `m_field` and user types `mf` or 
`m_f` it will be matched in both cases, because `m_field` yields `mf$` in the 
index build stage, so this change doesn't decrease code completion quality for 
such cases.


https://reviews.llvm.org/D50700



___
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 161018.
kadircet marked 21 inline comments as done.
kadircet added a comment.

- Resolve discussions.


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,79 @@
+#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 = TaskHandle::create();
+  WithContext ContextWithCancellation(
+  setCurrentCancellationToken(TH.createCancellationToken()));
+  EXPECT_FALSE(isCancelled());
+  TH.cancel();
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskHandleTestHandleDiesContextLives) {
+  llvm::Optional ContextWithCancellation;
+  {
+auto CancellableTaskHandle = TaskHandle::create();
+ContextWithCancellation.emplace(setCurrentCancellationToken(
+CancellableTaskHandle.createCancellationToken()));
+EXPECT_FALSE(isCancelled());
+CancellableTaskHandle.cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  EXPECT_TRUE(isCancelled());
+}
+
+TEST(CancellationTest, TaskHandleContextDiesHandleLives) {
+  auto CancellableTaskHandle = TaskHandle::create();
+  {
+WithContext ContextWithCancellation(setCurrentCancellationToken(
+CancellableTaskHandle.createCancellationToken()));
+EXPECT_FALSE(isCancelled());
+CancellableTaskHandle.cancel();
+EXPECT_TRUE(isCancelled());
+  }
+  // Still should be able to cancel without any problems.
+  CancellableTaskHandle.cancel();
+}
+
+TEST(CancellationTest, CancellationToken) {
+  auto CancellableTaskHandle = TaskHandle::create();
+  WithContext ContextWithCancellation(setCurrentCancellationToken(
+  CancellableTaskHandle.createCancellationToken()));
+  const auto &CT = getCurrentCancellationToken();
+  EXPECT_FALSE(CT);
+  CancellableTaskHandle.cancel();
+  EXPECT_TRUE(CT);
+}
+
+TEST(CancellationTest, AsynCancellationTest) {
+  std::atomic HasCancelled(false);
+  Notification Cancelled;
+  auto TaskToBeCancelled = [&](CancellationToken CT) {
+WithContext ContextGuard(setCurrentCancellationToken(std::move(CT)));
+Cancelled.wait();
+HasCancelled = isCancelled();
+  };
+  TaskHandle TH = TaskHandle::create();
+  std::thread AsyncTask(TaskToBeCancelled, TH.createCancellationToken());
+  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,13 @@
 llvm::json::Value toJSON(const DocumentHighlight &DH);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
+struct CancelParams {
+  std::string ID;
+};
+llvm::json::Value toJSON(const CancelParams &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const CancelParams &);
+bool fromJSON(const llvm::json::Value &, Cancel

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/Cancellation.h:92
+  operator bool() const { return isCancelled(); }
+  friend CancellationToken isCancelled();
+

ilya-biryukov wrote:
> It's a bit confusing that this name clashes with a member function.
> We seem to optimize for making this work anywhere:
> ```
>   if (isCancelled())  // <-- get token from ctx, then use implicit conversion 
> to check if it was cancelled
> return llvm::makeError();
> ```
> 
> Which is probably a nice pattern. But we could have two functions that do the 
> same thing with less magic (i.e. implicit conversions) involved at the call 
> sites:
> ```
> CancellationToken getCurrentCancellation();
> void setCurrentCancellation(CancellationToken CT);
> 
> inline bool isCancelled() { return getCurrentCancellation().isCancelled(); }
> ```
> Would allow to get rid of implicit conversion and friend function with the 
> same signature as member. Would also make cases where we actually want to 
> stash the token somewhere cleaner, e.g. instead of `CancellationToken CT = 
> isCancelled()`, we'll have `CancellationToken CT = getCurrentCancellation()`.
> WDYT?
Totally agreed, actually, didn't even notice they had the same signature :D, 
thanks!
 But I still think implicit conversion of CancellationToken to bool seems like 
a good thing to have.



Comment at: clangd/ClangdLSPServer.h:179
+  // associated with only their thread.
+  void CleanupTaskHandle();
+  void StoreTaskHandle(TaskHandle TH);

ilya-biryukov wrote:
> These two methods look like a nice suit for better wrapped in a RAII-object. 
> It would add the value to the associated map on construction and remove on 
> destruction.
> One problem is that all operations calls would become weird, because we'll 
> have to move this RAII object around and moving into lambdas is not fun 
> without C++14 (which allows `[x=std::move(y)]() {}`..
> 
> Maybe add a comment that calls to this function should always be paired and a 
> FIXME that we should have an API that enforces this?
> 
Yeah that would be really nice to have them in a RAII object, but the thing is 
cleanup can occur only after the task dies. Which we don't directly know since 
threads are being detached. So the only way to trigger destruction is at the 
callback, which is where I call cleanup currently. Maybe we can store the 
object within context to trigger destructor at the end of the thread, but still 
don't think it would look any better or ease the job of the caller.


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

[libunwind] r339878 - [libunwind][mips] Include gcc_s for linkage

[Stefan Maksimovic via cfe-commits]

Author: smaksimovic
Date: Thu Aug 16 06:40:16 2018
New Revision: 339878

URL: http://llvm.org/viewvc/llvm-project?rev=339878&view=rev
Log:
[libunwind][mips] Include gcc_s for linkage

When compiling with optimizations, mips requires various helper 
routines(__ashldi3 and the like) contained in libgcc_s.
Conditionally include libgcc_s in the set of libraries to be linked to.

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

Modified:
libunwind/trunk/src/CMakeLists.txt

Modified: libunwind/trunk/src/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/CMakeLists.txt?rev=339878&r1=339877&r2=339878&view=diff
==
--- libunwind/trunk/src/CMakeLists.txt (original)
+++ libunwind/trunk/src/CMakeLists.txt Thu Aug 16 06:40:16 2018
@@ -52,6 +52,7 @@ set(LIBUNWIND_SOURCES
 # Generate library list.
 set(libraries ${LIBUNWINDCXX_ABI_LIBRARIES})
 append_if(libraries LIBUNWIND_HAS_C_LIB c)
+append_if(libraries LIBUNWIND_HAS_GCC_S_LIB gcc_s)
 append_if(libraries LIBUNWIND_HAS_DL_LIB dl)
 if (LIBUNWIND_ENABLE_THREADS)
   append_if(libraries LIBUNWIND_HAS_PTHREAD_LIB pthread)


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

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Just drive-by comments, the maintainers of the code are in a much better 
position to give feedback, of course.
Nevertheless, my few cents:

- Getting rid of a regex in favor of the explicit loop is definitely a good 
thing. It's incredibly how much time is spent there when serializing big chunks 
of YAML (our case in clangd).
- Other changes are definitely less of a win performance-wise and I personally 
find the new code a bit harder to read than before, so don't feel as confident 
about those.
- Actual correctness fixes are a good thing, but could go in as a separate 
patch.

I would suggest splitting the patch into two: (1) rewriting the regex, (2) 
other changes. (1) is such a clear win, that it would be a pitty to have it 
delayed by reviewing other parts of the patch :-)




Comment at: llvm/include/llvm/Support/YAMLTraits.h:466
+  static const char HexChars[] = "0123456789abcdefABCDEF";
+  static const char OctalChars[] = "01234567";
+  bool ParseHex = S.startswith("0x");

I would argue the previous version of parsing hex and octal chars was more 
readable.
And I'm not sure the new heavily optimized version is more performant too.



Comment at: llvm/include/llvm/Support/YAMLTraits.h:494
+  bool FoundExponent = false;
+  for (size_t I = 0; I < Tail.size(); ++I) {
+char Symbol = Tail[I];

A more structured way to parse a floating numbers would be to:

1. skip digits until we find `.` or exponent (`e`)
2. if we find `.`, skip digits until we find an exponent.
3. if we find an exponent, skip the next symbol if it's `'+'` or `'-'`, then 
skip digits until the end of the string.

having a code structure that mirrors that would make the code more readable.



https://reviews.llvm.org/D50839



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

[PATCH] D50700: [clangd] Generate better incomplete bigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
 
+  EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"}));
+  EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"}));

kbobyrev wrote:
> ioeric wrote:
> > I'm not sure if this is correct. If users have explicitly typed `_`, they 
> > are likely to want a `_` there. You mentioned in the patch summary that 
> > users might want to match two heads with this. Could you provide an example?
> The particular example I had on my mind was `unique_ptr`. Effectively, if the 
> query is `SC` then `StrCat` would be matched (because the incomplete trigram 
> would be `sc$` and two heads from the symbol identifier would also yield 
> `sc$`), but for `u_p`, `unique_ptr` is currently not matched.
> 
> On the other hand, if there's something like `m_field` and user types `mf` or 
> `m_f` it will be matched in both cases, because `m_field` yields `mf$` in the 
> index build stage, so this change doesn't decrease code completion quality 
> for such cases.
The problem is that `u_p` can now match `upXXX` with `up$`, which might be a 
bit surprising for users. 

It seems to me that another way way to handle this is to generate `u_p` trigram 
for `unique_ptr`. Have we considered including separators like `_` in trigrams? 


https://reviews.llvm.org/D50700



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

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: llvm/include/llvm/Support/YAMLTraits.h:477
+  if (ParseOct && std::find(std::begin(OctalChars), std::end(OctalChars),
+Char) == std::end(OctalChars))
+return false;

Can you use strchr here? I would expect the compiler to fold the string 
constants into bit tests, creating both more compact and faster code.


https://reviews.llvm.org/D50839



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

[clang-tools-extra] r339879 - Fixed unused variable warning. NFCI.

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Thu Aug 16 06:55:10 2018
New Revision: 339879

URL: http://llvm.org/viewvc/llvm-project?rev=339879&view=rev
Log:
Fixed unused variable warning. NFCI.

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

Modified: clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp?rev=339879&r1=339878&r2=339879&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp Thu Aug 16 
06:55:10 2018
@@ -58,7 +58,8 @@ TEST(FileDistanceTests, BadSource) {
   EXPECT_EQ(D.distance("b/b/b/c"), 17u); // a+up+down+down+down+down, not 
b+down
 }
 
-auto UseUnittestScheme = UnittestSchemeAnchorSource;
+// Force the unittest URI scheme to be linked,
+static int LLVM_ATTRIBUTE_UNUSED UseUnittestScheme = 
UnittestSchemeAnchorSource;
 
 TEST(FileDistanceTests, URI) {
   FileDistanceOptions Opts;


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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Change option name.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835

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


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1590,6 +1590,55 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(CompletionTest, RenderWithSnippetsForFunctionArgsDisabled) {
+  CodeCompleteOptions Opts;
+  Opts.DisableSnippetTemplateForFunctionArgs = true;
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "()";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
+  }
+
+  Opts.EnableSnippets = true;
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "()";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x()");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "(${0:bool})";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -82,6 +82,10 @@
   /// Include completions that require small corrections, e.g. change '.' to
   /// '->' on member access etc.
   bool IncludeFixIts = false;
+
+  /// Disables snippet generation for function arguments. Does nothing if
+  /// snippets are not enabled.
+  bool DisableSnippetTemplateForFunctionArgs = false;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ 
API.
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -1371,8 +1371,14 @@
   LSP.additionalTextEdits.push_back(FixIt);
 }
   }
-  if (Opts.EnableSnippets)
-LSP.textEdit->newText += SnippetSuffix;
+  if (Opts.EnableSnippets && !SnippetSuffix.empty()) {
+if (Opts.DisableSnippetTemplateForFunctionArgs)
+  // Check whether function has any parameters or not.
+  LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
+else
+  LSP.textEdit->newText += SnippetSuffix;
+  }
+
   // FIXME(kadircet): Do not even fill insertText after making sure textEdit is
   // compatible with most of the editors.
   LSP.insertText = LSP.textEdit->newText;


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1590,6 +1590,55 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(CompletionTest, RenderWithSnippetsForFunctionArgsDisabled) {
+  CodeCompleteOptions Opts;
+  Opts.DisableSnippetTemplateForFunctionArgs = true;
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "()";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
+  }
+
+  Opts.EnableSnippets = true;
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "()";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x()");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+
+  {
+CodeCompletion C;
+C.RequiredQualifier = "Foo::";
+C.Name = "x";
+C.SnippetSuffix = "(${0:bool})";
+
+auto R = C.render(Opts);
+EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
+EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ cla

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/CodeComplete.h:85
+
+  /// Enables cursor to be moved around according to completions needs even 
when
+  /// snippets are disabled. For example selecting a function with parameters 
as

ioeric wrote:
> IIRC, the goal of this patch is to allow disabling snippet templates for 
> function parameters while still allow appending `()` or `($0)` to function 
> candidates. If that's still the case, I think what we want is probably an 
> option like `DisableSnippetTemplateForFunctionArgs`? 
Yeah, that makes totally more sense. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



___
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 added inline comments.



Comment at: test/clang-tidy/abseil-fake-declarations.h:1
+namespace std {
+struct string {

hokein wrote:
> I'd expect this header file is used as as a real absl library file:
> 
> * create an `absl` directory in `test/clang-tidy/Inputs/`, and this directory 
> is the abseil source root directory
> * use the real absl function/class names instead of some fake names in the 
> header, e.g. this file could be 
> `test/clang-tidy/Inputs/absl/strings/str_cat.h`.
> 
> This would make the lit test align with the real world case, and it could be 
> helpful if you implement the `IsExpansionInAbseilHeader` matcher in this 
> check. 
My mentors on the absl team told me to refrain from using real absl 
function/class names as I did originally. They would prefer I use fake names. 
I'd be happy to update the filename something real like str_cat.h though

I have been implementing IsExpansionInHeader for the past couple days and I 
hope to have it included on this patch by the end of the week. 



Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:19
+namespace absl {
+  void OpeningNamespace() {
+strings_internal::InternalFunction();

hokein wrote:
> the style doesn't looks correct to me.
This is the style given from clang-format


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] D50580: [clang-tidy] Abseil: no namespace check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

That sounds good as well, just not in clang and best in `clang-tidy/utils`

> `ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. 
> We  have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific 
> matchers. I'm not sure whether it is reasonable to put abseil-specific 
> matchers there. Maybe we create a `AbseilMatcher.h` file in 
> `clang-tidy/abseil` directory?
> 
> https://reviews.llvm.org/D50580


https://reviews.llvm.org/D50580



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