[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

kbobyrev wrote:
> sammccall wrote:
> > eduucaldas wrote:
> > > Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax 
> > > constructs usually have nice classes with accessors.
> > > 
> > > For instance `CompoundStatement` has the accessors `getLbrace` and 
> > > `getRbrace` that seem to be exactly what you want.
> > > 
> > > However these might not give exactly the first leaf and last leaf in the 
> > > case of syntactically incorrect code.
> > I think we should treat all bracket-like things generically. Today this is 
> > just CompoundStmt, but we want to handle init lists, function calls, parens 
> > around for-loop conditions, template parameter and arg lists etc in the 
> > same way.
> > 
> > This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are generic 
> > - we can have one set of logic to handle all of these. (No opinion on 
> > whether that should live here or in the syntax trees library, but putting 
> > it here for now seems fine).
> > 
> > So in the end I think checking the class name and then grabbing the braces 
> > by role (not kind) is the right thing here.
> > We definitely want to avoid asserting that the code looks the way we expect 
> > though.
> > So in the end I think checking the class name and then grabbing the braces 
> > by role (not kind) is the right thing here.
> > We definitely want to avoid asserting that the code looks the way we expect 
> > though.
> 
> Can you elaborate a bit on how this would work? Is your proposal to iterate 
> through `CompoundStatement` first-level children and grab the `OpenParen` + 
> `CloseParen` roles?
Exactly. And bail out if both don't exist.

And this can be done on Tree, so it's trivial to add support for function calls 
etc (but feel free to keep the scope small)



Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:771
   }
-  Leaf *getLbrace();
+  Leaf *getLbrace() const;
   /// FIXME: use custom iterator instead of 'vector'.

This doesn't look right: now a const method grants mutable access to the child. 
I think we need both overloads :-(

(Fortunately this is to be tablegen'd one day...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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


[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

In D89743#2341779 , @aaron.ballman 
wrote:

> This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify 
scope first.

> One question I have is: as it stands, this is not a particularly useful 
> matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs 
(clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a 
basic matcher wasn't hard to add.

> are you planning to extend this functionality so that you can do something 
> like `attr(hasName("foo"))`, or specify the syntax the attribute uses, 
> `isImplicit()`, etc?

I hadn't planned to - it definitely makes sense though I don't have any 
immediate use cases. I can do any of:

- leave as-is, waiting for someone to add matchers to make it useful
- scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest 
another way)
- add some simple matcher like hasName (I guess in a followup patch) to make it 
minimally useful, with space for more

What do you think?




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6737
+///   struct [[nodiscard]] Foo{};
+///   void bar(int * __attribute__((nonnull)) );
+/// \endcode

aaron.ballman wrote:
> Can you also add an example using `__declspec()`?
> 
> Should I expect this to work on attributes added in other, perhaps surprising 
> ways, like through `#pragma` or keywords?
> ```
> // Some pragmas add attributes under the hood
> #pragma omp declare simd 
> int min(int a, int b) { return a < b ? a : b; }
> 
> // Other pragmas only exist to add attributes
> #pragma clang attribute push (__attribute__((annotate("custom"))), apply_to = 
> function)
> void function(); // The function now has the annotate("custom") attribute
> #pragma clang attribute pop
> 
> // Still other attributes come through keywords
> alignas(16) int i;
> ```
> If this is expected to match, we should document it more clearly.
I think the answer to all of these is yes, I'll update the docs.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:238
+ScopedIncrement ScopedDepth();
+return (A == nullptr || traverse(*A));
+  }

aaron.ballman wrote:
> Should we be properly handling `IgnoreUnlessSpelledInSource` for implicit 
> attributes? e.g., `[[gnu::interrupt]] void func(int *i);` which gets two 
> attributes (the `interrupt` attribute and an implicit `used` attribute).
I think we should, I just wasn't thinking clearly about where that should go :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743

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


[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:39
   FoldingRange Range;
-  Range.startLine = Symbol.range.start.line;
-  Range.startCharacter = Symbol.range.start.character;
-  Range.endLine = Symbol.range.end.line;
-  Range.endCharacter = Symbol.range.end.character;
-  Result.push_back(Range);
-  for (const auto  : Symbol.children)
-collectFoldingRanges(Child, Result);
+  Range.startCharacter = SM.getSpellingColumnNumber(SR.getBegin()) - 1;
+  Range.startLine = SM.getSpellingLineNumber(SR.getBegin()) - 1;

Have you considered how you want macro-expanded code to work?

As written, this code can produce ranges inside macro definitions, invalid 
ranges (if { and } aren't from the same macro expansion) or even crash 
(FirstToken->endLocation() may be an invalid one-past-the-end location).

My suggestion would be to have this return optional and simply 
bail out if the location are not file locations for now. (Later we can deal 
with macro args in some cases)



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

eduucaldas wrote:
> Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax 
> constructs usually have nice classes with accessors.
> 
> For instance `CompoundStatement` has the accessors `getLbrace` and 
> `getRbrace` that seem to be exactly what you want.
> 
> However these might not give exactly the first leaf and last leaf in the case 
> of syntactically incorrect code.
I think we should treat all bracket-like things generically. Today this is just 
CompoundStmt, but we want to handle init lists, function calls, parens around 
for-loop conditions, template parameter and arg lists etc in the same way.

This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are generic - we 
can have one set of logic to handle all of these. (No opinion on whether that 
should live here or in the syntax trees library, but putting it here for now 
seems fine).

So in the end I think checking the class name and then grabbing the braces by 
role (not kind) is the right thing here.
We definitely want to avoid asserting that the code looks the way we expect 
though.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:56
+assert(LastToken->kind() == tok::TokenKind::r_brace);
+const SourceRange SR(FirstToken->endLocation(), LastToken->location());
+FoldingRange Range = constructFoldingRange(SR, SM);

this is worthy of a comment (fold the entire range inside the brackets, 
including whitespace)



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:60
+// nodes and newlines.
+if (Tree->findFirstLeaf()->getNextSibling() != Tree->findLastLeaf() ||
+Range.startLine != Range.endLine)

until we support FoldingRangeClientCapabilities, should we just have the line 
check?



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:206
   R"cpp(
-[[int global_variable]];
+void func() {[[
+  int Variable = 100;

This is too many test cases for straightforward compoundstmt, I think. We have 
only once type of node handled for now, but we can't afford to write and 
maintain this many tests once we have lots.

I think a single if{}-elseif(nobraces)-else{} along with the existing function 
examples is probably enough. (Function examples show missing range for empty 
body, though it could use a comment)

We definitely need cases for macros:
 - entire {} within one macro arg
 - entire {} in the macro body
 - some combination (e.g. { in macro body, } outside the macro)
(we won't need these for every node type, assuming we have some consistent 
handling)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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


[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Yep, let's revert to the previous state and land that, and I'll puzzle over the 
examples you give (because always returning false shouldn't affect behavior, 
just performance).

I have put together D89785  for more general 
attribute support, and it has a generalization of the fix here. (It returns 
`false` for any node with an attribute attached).
But it's worth landing this first as it has good tests for the objc cases, and 
that patch has its own prerequisites and risks of regressions.
(Not a timely coincidence, rather I got curious about the AST around Attrs 
after seeing this patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89579

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


[PATCH] D89783: [format] foo..h should be the main-header for foo..cc

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thank you!




Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:243
+  StringRef MatchingFileStem = matchingStem(FileName);  // foo for foo.cu.cc
+  // main-header examples:
+  //   1) foo.h => foo.cc

nit: comment is internally inconsistent here, which makes it a bit hard to 
follow.
 - either use bullets for both lists, or for neither
 - either use placeholders like 'foo' or like '', but not both
 - list the most important/obvious examples at the top for both cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89783

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


[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 299324.
sammccall added a comment.

Update docs, dynamic registry and tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/AST/ASTFwd.h
  clang/include/clang/AST/ASTTypeTraits.h
  clang/include/clang/ASTMatchers/ASTMatchFinder.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/AST/ASTTypeTraits.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/AST/ASTTypeTraitsTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1875,6 +1875,17 @@
  nestedNameSpecifier()));
 }
 
+TEST_P(ASTMatchersTest, Attr) {
+  if (!GetParam().isCXX())
+return;
+  if (GetParam().isCXX17OrLater())
+EXPECT_TRUE(matches("struct [[nodiscard]] F{};", attr()));
+  EXPECT_TRUE(matches("int x(int * __attribute__((nonnull)) );", attr()));
+  // On windows, some nodes an implicit visibility attribute.
+  if (!GetParam().hasDelayedTemplateParsing() /* Windows */)
+EXPECT_TRUE(notMatches("struct F{}; int x(int *);", attr()));
+}
+
 TEST_P(ASTMatchersTest, NullStmt) {
   EXPECT_TRUE(matches("void f() {int i;;}", nullStmt()));
   EXPECT_TRUE(notMatches("void f() {int i;}", nullStmt()));
Index: clang/unittests/AST/ASTTypeTraitsTest.cpp
===
--- clang/unittests/AST/ASTTypeTraitsTest.cpp
+++ clang/unittests/AST/ASTTypeTraitsTest.cpp
@@ -120,6 +120,7 @@
   VERIFY_NAME(CallExpr);
   VERIFY_NAME(Type);
   VERIFY_NAME(ConstantArrayType);
+  VERIFY_NAME(NonNullAttr);
 #undef VERIFY_NAME
 }
 
@@ -148,6 +149,13 @@
  nestedNameSpecifierLoc()));
 }
 
+TEST(DynTypedNode, AttrSourceRange) {
+  RangeVerifier Verifier;
+  Verifier.expectRange(1, 31, 1, 31);
+  EXPECT_TRUE(Verifier.match("void x(char *y __attribute__((nonnull)) );",
+ ast_matchers::attr()));
+}
+
 TEST(DynTypedNode, DeclDump) {
   DumpVerifier Verifier;
   Verifier.expectSubstring("FunctionDecl");
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -141,6 +141,7 @@
   REGISTER_MATCHER(asmStmt);
   REGISTER_MATCHER(atomicExpr);
   REGISTER_MATCHER(atomicType);
+  REGISTER_MATCHER(attr);
   REGISTER_MATCHER(autoType);
   REGISTER_MATCHER(autoreleasePoolStmt)
   REGISTER_MATCHER(binaryConditionalOperator);
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -973,6 +973,7 @@
 const internal::VariadicAllOfMatcher nestedNameSpecifier;
 const internal::VariadicAllOfMatcher
 nestedNameSpecifierLoc;
+const internal::VariadicAllOfMatcher attr;
 const internal::VariadicDynCastAllOfMatcher
 cudaKernelCallExpr;
 const AstTypeMatcher builtinType;
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -131,6 +131,8 @@
 else if (const TemplateArgumentLoc *TALoc =
  DynNode.get())
   traverse(*TALoc);
+else if (const Attr *A = DynNode.get())
+  traverse(*A);
 // FIXME: Add other base types after adding tests.
 
 // It's OK to always overwrite the bound nodes, as if there was
@@ -231,6 +233,10 @@
 ScopedIncrement ScopedDepth();
 return traverse(TAL);
   }
+  bool TraverseAttr(Attr *A) {
+ScopedIncrement ScopedDepth();
+return (A == nullptr || traverse(*A));
+  }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
 if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
 TK_IgnoreUnlessSpelledInSource)
@@ -314,6 +320,9 @@
   bool baseTraverse(TemplateArgumentLoc TAL) {
 return VisitorBase::TraverseTemplateArgumentLoc(TAL);
   }
+  bool baseTraverse(const Attr ) {
+return VisitorBase::TraverseAttr(const_cast());
+  }
 
   // Sets 'Matched' to true if 'Matcher' matches 'Node' and:
   //   0 < CurrentDepth <= MaxDepth.
@@ -458,6 +467,7 @@
   bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS);
   bool TraverseConstructorInitializer(CXXCtorInitializer *CtorInit);
   bool TraverseTemplateArgumentLoc(TemplateArgumentLoc TAL);
+  bool TraverseAttr(Attr *AttrNode);
 
   // 

[PATCH] D89785: [clangd] Add basic support for attributes (selection, hover)

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

These aren't terribly common, but we currently mishandle them badly.
Not only do we not recogize the attributes themselves, but we often end up
selecting some node other than the parent (because source ranges aren't accurate
in the presence of attributes).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89785

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -415,7 +415,19 @@
 template  class Container> class A {};
 A<[[V^ector]]> a;
   )cpp",
-   "TemplateArgumentLoc"}};
+   "TemplateArgumentLoc"},
+
+  // Attributes
+  {R"cpp(
+void f(int * __attribute__(([[no^nnull]])) );
+  )cpp",
+   "NonNullAttr"},
+
+  {R"cpp(
+// Digraph syntax for attributes to avoid accidental annotations.
+class <:[gsl::Owner([[in^t]])]:> X{};
+  )cpp",
+   "BuiltinTypeLoc"}};
 
   for (const Case  : Cases) {
 trace::TestTracer Tracer;
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,16 @@
 HI.NamespaceScope = "ObjC::"; // FIXME: fix it
 HI.Definition = "char data";
   }},
+  {
+  R"cpp(
+   void foo(int * __attribute__(([[non^null]], noescape)) );
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "nonnull";
+HI.Kind = index::SymbolKind::Unknown; // FIXME: no suitable value
+HI.Definition = "__attribute__((nonnull()))";
+HI.Documentation = ""; // FIXME
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -11,6 +11,7 @@
 #include "Annotations.h"
 #include "ParsedAST.h"
 #include "TestTU.h"
+#include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/SourceManager.h"
@@ -206,6 +207,31 @@
 }
   }
 }
+
+TEST(ClangdAST, HasAttributes) {
+  const char *Code = R"cpp(
+class X{};
+class [[nodiscard]] Y{};
+void f(int * a, int * __attribute__((nonnull)) b);
+void foo(bool c) {
+  if (c)
+[[unlikely]] return;
+}
+  )cpp";
+  ParsedAST AST = TestTU::withCode(Code).build();
+  ASSERT_FALSE(hasAttributes(DynTypedNode::create(findDecl(AST, "X";
+  ASSERT_TRUE(hasAttributes(DynTypedNode::create(findDecl(AST, "Y";
+  ASSERT_FALSE(hasAttributes(DynTypedNode::create(findDecl(AST, "f";
+  ASSERT_FALSE(
+  hasAttributes(DynTypedNode::create(findUnqualifiedDecl(AST, "a";
+  ASSERT_TRUE(
+  hasAttributes(DynTypedNode::create(findUnqualifiedDecl(AST, "b";
+
+  Stmt *FooBody = cast(findDecl(AST, "foo")).getBody();
+  IfStmt *FooIf = cast(cast(FooBody)->body_front());
+  ASSERT_FALSE(hasAttributes(DynTypedNode::create(*FooIf)));
+  ASSERT_TRUE(hasAttributes(DynTypedNode::create(*FooIf->getThen(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -182,6 +182,12 @@
   ST.commonAncestor()) {
 if (NodeKind)
   *NodeKind = N->ASTNode.getNodeKind();
+// Attributes don't target decls, look at the
+// thing it's attached to.
+// We still report the original NodeKind!
+// This makes the `override` hack work.
+if (N->ASTNode.get() && N->Parent)
+  N = N->Parent;
 llvm::copy(targetDecl(N->ASTNode, Relations),
   

[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
sammccall requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89743

Files:
  clang/include/clang/AST/ASTFwd.h
  clang/include/clang/AST/ASTTypeTraits.h
  clang/include/clang/ASTMatchers/ASTMatchFinder.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/AST/ASTTypeTraits.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTypeTraitsTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1875,6 +1875,15 @@
  nestedNameSpecifier()));
 }
 
+TEST_P(ASTMatchersTest, Attr) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  EXPECT_TRUE(matches("struct [[nodiscard]] F{};", attr()));
+  EXPECT_TRUE(matches("int x(int * __attribute__((nonnull)) );", attr()));
+  EXPECT_TRUE(notMatches("struct F{}; int x(int *);", attr()));
+}
+
 TEST_P(ASTMatchersTest, NullStmt) {
   EXPECT_TRUE(matches("void f() {int i;;}", nullStmt()));
   EXPECT_TRUE(notMatches("void f() {int i;}", nullStmt()));
Index: clang/unittests/AST/ASTTypeTraitsTest.cpp
===
--- clang/unittests/AST/ASTTypeTraitsTest.cpp
+++ clang/unittests/AST/ASTTypeTraitsTest.cpp
@@ -120,6 +120,7 @@
   VERIFY_NAME(CallExpr);
   VERIFY_NAME(Type);
   VERIFY_NAME(ConstantArrayType);
+  VERIFY_NAME(NonNullAttr);
 #undef VERIFY_NAME
 }
 
@@ -148,6 +149,13 @@
  nestedNameSpecifierLoc()));
 }
 
+TEST(DynTypedNode, AttrSourceRange) {
+  RangeVerifier Verifier;
+  Verifier.expectRange(1, 31, 1, 31);
+  EXPECT_TRUE(Verifier.match("void x(char *y __attribute__((nonnull)) );",
+ ast_matchers::attr()));
+}
+
 TEST(DynTypedNode, DeclDump) {
   DumpVerifier Verifier;
   Verifier.expectSubstring("FunctionDecl");
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -973,6 +973,7 @@
 const internal::VariadicAllOfMatcher nestedNameSpecifier;
 const internal::VariadicAllOfMatcher
 nestedNameSpecifierLoc;
+const internal::VariadicAllOfMatcher attr;
 const internal::VariadicDynCastAllOfMatcher
 cudaKernelCallExpr;
 const AstTypeMatcher builtinType;
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -131,6 +131,8 @@
 else if (const TemplateArgumentLoc *TALoc =
  DynNode.get())
   traverse(*TALoc);
+else if (const Attr *A = DynNode.get())
+  traverse(*A);
 // FIXME: Add other base types after adding tests.
 
 // It's OK to always overwrite the bound nodes, as if there was
@@ -231,6 +233,10 @@
 ScopedIncrement ScopedDepth();
 return traverse(TAL);
   }
+  bool TraverseAttr(Attr *A) {
+ScopedIncrement ScopedDepth();
+return (A == nullptr || traverse(*A));
+  }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
 if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
 TK_IgnoreUnlessSpelledInSource)
@@ -314,6 +320,9 @@
   bool baseTraverse(TemplateArgumentLoc TAL) {
 return VisitorBase::TraverseTemplateArgumentLoc(TAL);
   }
+  bool baseTraverse(const Attr ) {
+return VisitorBase::TraverseAttr(const_cast());
+  }
 
   // Sets 'Matched' to true if 'Matcher' matches 'Node' and:
   //   0 < CurrentDepth <= MaxDepth.
@@ -458,6 +467,7 @@
   bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS);
   bool TraverseConstructorInitializer(CXXCtorInitializer *CtorInit);
   bool TraverseTemplateArgumentLoc(TemplateArgumentLoc TAL);
+  bool TraverseAttr(Attr *AttrNode);
 
   // Matches children or descendants of 'Node' with 'BaseMatcher'.
   bool memoizedMatchesRecursively(const DynTypedNode , ASTContext ,
@@ -571,6 +581,8 @@
   match(*N);
 } else if (auto *N = Node.get()) {
   match(*N);
+} else if (auto *N = Node.get()) {
+  match(*N);
 }
   }
 
@@ -697,6 +709,9 @@
   void matchDispatch(const TemplateArgumentLoc *Node) {
 matchWithoutFilter(*Node, Matchers->TemplateArgumentLoc);
   }
+  void matchDispatch(const Attr *Node) {
+matchWithoutFilter(*Node, Matchers->Attr);
+  }
   void matchDispatch(const void *) { /* Do nothing. */ }
   /// @}
 
@@ -1068,6 +1083,11 @@
   return RecursiveASTVisitor::TraverseTemplateArgumentLoc(Loc);
 }
 
+bool 

[PATCH] D86936: [clang] Limit the maximum level of fold-expr expansion.

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a subscriber: rsmith.
sammccall added a comment.

In D86936#2338664 , @erichkeane wrote:

> Why did we select the 'bracket-depth' for this?  The documentation on that 
> doesn't seem to be anything close to what this should be based on:

Based on a discussion with @rsmith - the formal definition of pack expansion is 
that it expands to nested parenthesized expressions expressions.

>   Sets the limit for nested parentheses, brackets, and braces to N in blocks, 
> declarators, expressions, and struct or union declarations.
>
> The 256 default limit is REALLY small for a fold expression, particularly 
> since the instantiation depth limit is 1024 by default.  I think this patch 
> needs to be changed to use the InstantiationDepth instead.  @rjmccall, 
> thoughts?

One motivation here is to limit exposure of tools to stack overflows. A modest 
increase in the stack size of DynTypedNode triggered stack overflows in various 
traversals on real code.
While it's possible in principle to write traversals that use only data 
recursion (and I've also fixed the ones in clang itself that I can find), in 
practice a lot of code and tools rely on recursive traversals, so trivially 
creating arbitrarily deep ASTs without any limit is certain to break things 
with unclear responsibility. (Whereas this change places responsibility with 
boost numerics).

FWIW, somewhere between 1024 and 2048 was the critical depth on the systems we 
looked at, so 1024 seems... plausible, but large.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86936

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


[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:611
+  // robust.
+  return false;
 if (!SelChecker.mayHit(S)) {

I'm not sure we actually want to *do it* at this point, as you're seeing we may 
have some unintended consequences.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89579

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


[PATCH] D89700: [clangd] Don't offer to expand auto in structured binding declarations.

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: usaxena95.
Herald added subscribers: cfe-commits, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

auto must be used for the code to parse.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89700

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


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -572,6 +572,8 @@
 R"cpp(const char * x = "test";)cpp");
 
   EXPECT_UNAVAILABLE("dec^ltype(au^to) x = 10;");
+  // expanding types in structured bindings is syntactically invalid.
+  EXPECT_UNAVAILABLE("const ^auto &[x,y] = (int[]){1,2};");
 
   // FIXME: Auto-completion in a template requires disabling delayed template
   // parsing.
Index: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
@@ -53,13 +53,24 @@
 
 std::string ExpandAutoType::title() const { return "Expand auto type"; }
 
+// Structured bindings must use auto, e.g. `const auto& [a,b,c] = ...;`.
+// Return whether N (an AutoTypeLoc) is such an auto that must not be expanded.
+bool isStructuredBindingType(const SelectionTree::Node *N) {
+  // Walk up the TypeLoc chain, because auto may be qualified.
+  while (N && N->ASTNode.get())
+N = N->Parent;
+  // The relevant type is the only direct type child of a Decomposition.
+  return N && N->ASTNode.get();
+}
+
 bool ExpandAutoType::prepare(const Selection& Inputs) {
   CachedLocation = llvm::None;
   if (auto *Node = Inputs.ASTSelection.commonAncestor()) {
 if (auto *TypeNode = Node->ASTNode.get()) {
   if (const AutoTypeLoc Result = TypeNode->getAs()) {
 // Code in apply() does handle 'decltype(auto)' yet.
-if (!Result.getTypePtr()->isDecltypeAuto())
+if (!Result.getTypePtr()->isDecltypeAuto() &&
+!isStructuredBindingType(Node))
   CachedLocation = Result;
   }
 }


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -572,6 +572,8 @@
 R"cpp(const char * x = "test";)cpp");
 
   EXPECT_UNAVAILABLE("dec^ltype(au^to) x = 10;");
+  // expanding types in structured bindings is syntactically invalid.
+  EXPECT_UNAVAILABLE("const ^auto &[x,y] = (int[]){1,2};");
 
   // FIXME: Auto-completion in a template requires disabling delayed template
   // parsing.
Index: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
@@ -53,13 +53,24 @@
 
 std::string ExpandAutoType::title() const { return "Expand auto type"; }
 
+// Structured bindings must use auto, e.g. `const auto& [a,b,c] = ...;`.
+// Return whether N (an AutoTypeLoc) is such an auto that must not be expanded.
+bool isStructuredBindingType(const SelectionTree::Node *N) {
+  // Walk up the TypeLoc chain, because auto may be qualified.
+  while (N && N->ASTNode.get())
+N = N->Parent;
+  // The relevant type is the only direct type child of a Decomposition.
+  return N && N->ASTNode.get();
+}
+
 bool ExpandAutoType::prepare(const Selection& Inputs) {
   CachedLocation = llvm::None;
   if (auto *Node = Inputs.ASTSelection.commonAncestor()) {
 if (auto *TypeNode = Node->ASTNode.get()) {
   if (const AutoTypeLoc Result = TypeNode->getAs()) {
 // Code in apply() does handle 'decltype(auto)' yet.
-if (!Result.getTypePtr()->isDecltypeAuto())
+if (!Result.getTypePtr()->isDecltypeAuto() &&
+!isStructuredBindingType(Node))
   CachedLocation = Result;
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:621-623
+  if (auto AT = TL->getAs())
+S = AT.getModifiedLoc()
+.getSourceRange(); // should we just return false?

dgoldman wrote:
> Let me know if you think it would be better to return false here
Yes, it's better to return false, which is more conservative. As written, this 
code will prevent targeting things within the attribute. (We can't currently 
target the attribute itself due to DynTypedNode limitations, but I'd like to 
fix that)



Comment at: clang-tools-extra/clangd/Selection.cpp:607
 SourceRange S = N.getSourceRange();
 if (auto *TL = N.get()) {
   // DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to





Comment at: clang-tools-extra/clangd/Selection.cpp:619
 S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+  // AttributeTypeLoc points to the attribute's range, NOT the modified
+  // type's range.

AttributeTypeLoc -> AttributedTypeLoc

This isn't true in general actually - attributes can be prefix or postfix (or 
other things...) and the heuristics in TypeLoc::get{Begin,End}Loc basically 
assume postfix for `Attributed`, which is right sometimes and wrong sometimes. 
(And getSourceRange() doesn't have enough information to get this right - it'd 
need the SourceManager to compare locations).

TypeLoc ranges in general seem pretty error prone. I've suggested a comment - 
we should consider just bailing out entirely here in future.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2008
+  }},
+  // Should work even with nullability attribute.
+  {

this is the same as the above case (apart from the selection changes, which are 
tested in SelectionTests)

same for the rest of the MYObject tests.
The ones after that are good as they're testing hovering on a different type of 
entity.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2052
+  {
+  R"cpp(
+  @class ForwardDeclared;

these tests seem good (though unrelated to the rest of the patch, you might 
want to land them separately).

The Fooey test relies on selection of a protocol in a protocol-list, you may 
want to test this directly in SelectionTests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89579

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


[PATCH] D89571: [clangd] Add textDocument/ast extension method to dump the AST

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 298961.
sammccall added a comment.

(for real this time)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89571

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/DumpAST.cpp
  clang-tools-extra/clangd/DumpAST.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/ast.test
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/DumpASTTests.cpp

Index: clang-tools-extra/clangd/unittests/DumpASTTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/DumpASTTests.cpp
@@ -0,0 +1,135 @@
+//===-- DumpASTTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "DumpAST.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using testing::SizeIs;
+
+TEST(DumpASTTests, BasicInfo) {
+  std::pair Cases[] = {
+  {R"cpp(
+float root(int *x) {
+  return *x + 1;
+}
+  )cpp",
+   R"(
+declaration: Function - root
+  type: FunctionProto
+type: Builtin - float
+declaration: ParmVar - x
+  type: Pointer
+type: Builtin - int
+  statement: Compound
+statement: Return
+  expression: ImplicitCast - IntegralToFloating
+expression: BinaryOperator - +
+  expression: ImplicitCast - LValueToRValue
+expression: UnaryOperator - *
+  expression: ImplicitCast - LValueToRValue
+expression: DeclRef - x
+  expression: IntegerLiteral - 1
+  )"},
+  {R"cpp(
+namespace root {
+struct S { static const int x = 0; };
+int y = S::x + root::S().x;
+}
+  )cpp",
+   R"(
+declaration: Namespace - root
+  declaration: CXXRecord - S
+declaration: Var - x
+  type: Qualified - const
+type: Builtin - int
+  expression: IntegerLiteral - 0
+declaration: CXXConstructor
+declaration: CXXConstructor
+declaration: CXXConstructor
+declaration: CXXDestructor
+  declaration: Var - y
+type: Builtin - int
+expression: ExprWithCleanups
+  expression: BinaryOperator - +
+expression: ImplicitCast - LValueToRValue
+  expression: DeclRef - x
+specifier: TypeSpec
+  type: Record - S
+expression: ImplicitCast - LValueToRValue
+  expression: Member - x
+expression: MaterializeTemporary - rvalue
+  expression: CXXTemporaryObject - S
+type: Elaborated
+  specifier: Namespace - root::
+  type: Record - S
+  )"},
+  {R"cpp(
+template  int root() {
+  (void)root();
+  return T::value;
+}
+  )cpp",
+   R"(
+declaration: FunctionTemplate - root
+  declaration: TemplateTypeParm - T
+  declaration: Function - root
+type: FunctionProto
+  type: Builtin - int
+statement: Compound
+  expression: CStyleCast - ToVoid
+type: Builtin - void
+expression: Call
+  expression: ImplicitCast - FunctionToPointerDecay
+expression: DeclRef - root
+  template argument: Type
+type: Builtin - unsigned int
+  statement: Return
+expression: DependentScopeDeclRef - value
+  specifier: TypeSpec
+type: TemplateTypeParm - T
+  )"},
+  };
+  for (const auto  : Cases) {
+ParsedAST AST = TestTU::withCode(Case.first).build();
+auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "root")),
+AST.getTokens(), AST.getASTContext());
+EXPECT_EQ(llvm::StringRef(Case.second).trim(),
+  llvm::StringRef(llvm::to_string(Node)).trim());
+  }
+}
+
+TEST(DumpASTTests, Range) {
+  Annotations Case("$var[[$type[[int]] x]];");
+  ParsedAST AST = TestTU::withCode(Case.code()).build();
+  auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "x")), AST.getTokens(),
+  AST.getASTContext());
+  EXPECT_EQ(Node.range, Case.range("var"));
+  ASSERT_THAT(Node.children, SizeIs(1)) << "Expected one child typeloc";
+  EXPECT_EQ(Node.children.front().range, Case.range("type"));
+}
+
+TEST(DumpASTTests, Arcana) {
+  ParsedAST AST = TestTU::withCode("int 

[PATCH] D89571: [clangd] Add textDocument/ast extension method to dump the AST

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 298959.
sammccall added a comment.

Oops, forgot newly added files :-\


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89571

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt

Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -51,6 +51,7 @@
   DexTests.cpp
   DiagnosticsTests.cpp
   DraftStoreTests.cpp
+  DumpASTTests.cpp
   ExpectedTypeTest.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -5,6 +5,7 @@
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:"capabilities": {
+# CHECK-NEXT:  "astProvider": true,
 # CHECK-NEXT:  "codeActionProvider": true,
 # CHECK-NEXT:  "completionProvider": {
 # CHECK-NEXT:"allCommitCharacters": [
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -1576,6 +1576,45 @@
 };
 llvm::json::Value toJSON(const FoldingRange );
 
+/// Payload for textDocument/ast request.
+/// This request is a clangd extension.
+struct ASTParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The position of the node to be dumped.
+  /// The highest-level node that entirely contains the range will be returned.
+  Range range;
+};
+bool fromJSON(const llvm::json::Value &, ASTParams &, llvm::json::Path);
+
+/// Simplified description of a clang AST node.
+/// This is clangd's internal representation of C++ code.
+struct ASTNode {
+  /// The general kind of node, such as "expression"
+  /// Corresponds to the base AST node type such as Expr.
+  std::string role;
+  /// The specific kind of node this is, such as "BinaryOperator".
+  /// This is usually a concrete node class (with Expr etc suffix dropped).
+  /// When there's no hierarchy (e.g. TemplateName), the variant (NameKind).
+  std::string kind;
+  /// Brief additional information, such as "||" for the particular operator.
+  /// The information included depends on the node kind, and may be empty.
+  std::string detail;
+  /// A one-line dump of detailed information about the node.
+  /// This includes role/kind/description information, but is rather cryptic.
+  /// It is similar to the output from `clang -Xclang -ast-dump`.
+  /// May be empty for certain types of nodes.
+  std::string arcana;
+  /// The range of the original source file covered by this node.
+  /// May be missing for implicit nodes, or those created by macro expansion.
+  llvm::Optional range;
+  /// Nodes nested within this one, such as the operands of a BinaryOperator.
+  std::vector children;
+};
+llvm::json::Value toJSON(const ASTNode &);
+llvm::raw_ostream <<(llvm::raw_ostream &, const ASTNode &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -1300,5 +1300,41 @@
   return Result;
 }
 
+bool fromJSON(const llvm::json::Value , ASTParams ,
+  llvm::json::Path P) {
+  llvm::json::ObjectMapper O(Params, P);
+  return O && O.map("textDocument", R.textDocument) && O.map("range", R.range);
+}
+
+llvm::json::Value toJSON(const ASTNode ) {
+  llvm::json::Object Result{
+  {"role", N.role},
+  {"kind", N.kind},
+  };
+  if (!N.children.empty())
+Result["children"] = N.children;
+  if (!N.detail.empty())
+Result["detail"] = N.detail;
+  if (!N.arcana.empty())
+Result["arcana"] = N.arcana;
+  if (N.range)
+Result["range"] = *N.range;
+  return Result;
+}
+
+llvm::raw_ostream <<(llvm::raw_ostream , const ASTNode ) {
+  std::function Print = [&](const ASTNode ,
+ unsigned Level) {
+OS.indent(2 * Level) << N.role << ": " << N.kind;
+if (!N.detail.empty())
+  OS << " - " << N.detail;
+OS << "\n";
+for (const ASTNode  : N.children)
+  Print(C, Level + 1);
+  };
+  Print(Root, 0);
+  return OS;
+}
+
 } // namespace clangd
 } // namespace clang
Index: 

[PATCH] D89571: [clangd] Add textDocument/ast extension method to dump the AST

2020-10-16 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: adamcz.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, mgorny.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

This is a mass-market version of the "dump AST" tweak we have behind
-hidden-features.
I think in this friendlier form it'll be useful for people outside clang
developers, which would justify making it a real feature.
It could be useful as a step towards lightweight clang-AST tooling in clangd
itself (like matcher-based search).

Advantages over the tweak:

- simplified information makes it more accessible, likely somewhat useful 
without learning too much clang internals
- can be shown in a tree view
- structured information gives some options for presentation (e.g. icon + two 
text colors + tooltip in vscode)
- clickable nodes jump to the corresponding code

Disadvantages:

- a bunch of code to handle different node types
- likely missing some important info vs dump-ast due to brevity/oversight
- may end up chasing/maintaining support for the long tail of nodes

Demo with VSCode support: https://imgur.com/a/6gKfyIV


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89571

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt

Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -51,6 +51,7 @@
   DexTests.cpp
   DiagnosticsTests.cpp
   DraftStoreTests.cpp
+  DumpASTTests.cpp
   ExpectedTypeTest.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -5,6 +5,7 @@
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:"capabilities": {
+# CHECK-NEXT:  "astProvider": true,
 # CHECK-NEXT:  "codeActionProvider": true,
 # CHECK-NEXT:  "completionProvider": {
 # CHECK-NEXT:"allCommitCharacters": [
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -1576,6 +1576,45 @@
 };
 llvm::json::Value toJSON(const FoldingRange );
 
+/// Payload for textDocument/ast request.
+/// This request is a clangd extension.
+struct ASTParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The position of the node to be dumped.
+  /// The highest-level node that entirely contains the range will be returned.
+  Range range;
+};
+bool fromJSON(const llvm::json::Value &, ASTParams &, llvm::json::Path);
+
+/// Simplified description of a clang AST node.
+/// This is clangd's internal representation of C++ code.
+struct ASTNode {
+  /// The general kind of node, such as "expression"
+  /// Corresponds to the base AST node type such as Expr.
+  std::string role;
+  /// The specific kind of node this is, such as "BinaryOperator".
+  /// This is usually a concrete node class (with Expr etc suffix dropped).
+  /// When there's no hierarchy (e.g. TemplateName), the variant (NameKind).
+  std::string kind;
+  /// Brief additional information, such as "||" for the particular operator.
+  /// The information included depends on the node kind, and may be empty.
+  std::string detail;
+  /// A one-line dump of detailed information about the node.
+  /// This includes role/kind/description information, but is rather cryptic.
+  /// It is similar to the output from `clang -Xclang -ast-dump`.
+  /// May be empty for certain types of nodes.
+  std::string arcana;
+  /// The range of the original source file covered by this node.
+  /// May be missing for implicit nodes, or those created by macro expansion.
+  llvm::Optional range;
+  /// Nodes nested within this one, such as the operands of a BinaryOperator.
+  std::vector children;
+};
+llvm::json::Value toJSON(const ASTNode &);
+llvm::raw_ostream <<(llvm::raw_ostream &, const ASTNode &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -1300,5 +1300,41 @@
   return Result;
 }
 
+bool fromJSON(const llvm::json::Value , ASTParams ,
+   

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

2020-10-16 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:623
 {"typeHierarchyProvider", true},
+{"memoryTreeProvider", true}, // clangd extension.
 ;

one last naming nit: I think `memoryUsage` would put appropriately more weight 
on the semantics rather than the data structure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89277

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


[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

2020-10-16 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

This looks really nice!




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:615
 {"workspaceSymbolProvider", true},
 {"referencesProvider", true},
 {"executeCommandProvider",

can you add `{"memoryTreeProvider", true} // clangd extension`?

I'd like to add client support in VSCode and it's useful if the command is only 
visible with versions of clangd that support it.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1389
 
+void ClangdLSPServer::onDumpMemoryTree(const NoParams &,
+   Callback Reply) {

Putting the serialization code inline is a tempting option here because it's 
(mostly) just a map.
It's not something we do often though, so it does bury the protocol details in 
an unexpected place.

A couple of alternatives to consider:
 - map to a `struct UsageTree { unsigned Self; unsigned Total; Map Children; }` defined in protocol.h, and define marshalling in the 
usual way
 - skip the struct and use MemoryTree as the type, providing a toJSON(const 
MemoryTree&) function in protocol.h

I lean towards the last alternative because it provides a bit more regularity 
without adding much more redundancy, but this is up to you.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1391
+   Callback Reply) {
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();

nit: Alloc -> DetailAlloc



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1458
   MsgHandler->bind("textDocument/semanticTokens/full/delta", 
::onSemanticTokensDelta);
+  MsgHandler->bind("$/dumpMemoryTree", ::onDumpMemoryTree);
   if (Opts.FoldingRanges)

I think "dump" is a bit casual and also redundant - LSP methods to obtain 
information tend to have noun names. (executeCommand is the exception that 
proves the rule - it's used for side effects).

I went through the same question with the AST-dumping and landed on 
textDocument/ast as my preferred option, for what it's worth.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:145
  Callback);
+  /// This is a clangd extension. Provides a json tree representing memory 
usage
+  /// hierarchy. Keys starting with an underscore(_) represent leaves, e.g.

(if you decide to move toJSON to protocol.h, then the second sentence onward 
probably belongs there)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89277

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


[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

2020-10-14 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

(sorry out today and haven't looked at code yet)

If it's a custom method, I think it should return the data as a json structure 
- the client already has to have custom support to invoke it, displaying the 
result isn't much extra work.

And I would really love to add a tree view to vscode, I think it wouldn't be 
hard (vs call hierarchy: no laziness and no direction-flipping) and could be 
reused for an AST viewer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89277

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


[PATCH] D88299: [clang-format] Add MacroUnexpander.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

This is magnificently difficult :-)
I've sunk a fair few hours into understanding it now, and need to send some 
comments based on the interface+tests.
Some of these will be misguided or answered by the implementation, but I'm 
afraid I can't fit it all into my head (on a reasonable timeframe) until I 
understand it better.




Comment at: clang/lib/Format/FormatToken.h:449
 
+  /// When macro expansion introduces parents, those are marked as
+  /// \c MacroParent, so formatting knows their children need to be formatted.

I can't really understand from the comment when this is supposed to be set, and 
there are no tests of it.

(The comment is vague: is a "parent" the inverse of FormatToken::Children here? 
Is this scenario when the parents in question are new, or their children are 
new, or both? What part of the code is "formatting", and why would it otherwise 
skip the children?)



Comment at: clang/lib/Format/Macros.h:134
 
+/// Matches formatted lines that were created by macro expansion to a format
+/// for the original macro call.

"matches formatted lines" probably describes the hard technical problem it has 
to solve, but not so much what it does for the caller:  what the transformation 
is between its inputs and its outputs.

Is it something like:

```
Rewrites expanded code (containing tokens expanded from macros) into spelled 
code (containing tokens for the macro call itself). Token types are preserved, 
so macro arguments in the output have semantically-correct types from their 
expansion. This is the point of expansion/unexpansion: to allow this 
information to be used in formatting.
```

[Is it just tokentypes? I guess it's also Role and MustBreakBefore and some 
other stuff like that?]



Comment at: clang/lib/Format/Macros.h:142
+/// When getting the formatted lines of the expansion via the \c addLine 
method:
+/// -> class A {
+/// -> public:

I'm a bit confused by these arrows. It doesn't seem that they each point to an 
unwrappedline passed to addLine?



Comment at: clang/lib/Format/Macros.h:142
+/// When getting the formatted lines of the expansion via the \c addLine 
method:
+/// -> class A {
+/// -> public:

sammccall wrote:
> I'm a bit confused by these arrows. It doesn't seem that they each point to 
> an unwrappedline passed to addLine?
This example didn't really help me understand the interface of this class, it 
seems to be a special case:
 - the input is a single block construct (rather than e.g. a whole program), 
but it's not clear why that's the case.
 - the output (unexpanded form) consists of exactly a macro call with no 
leading/trailing tokens, which isn't true in general

If the idea is to provide as input the minimal range of lines that span the 
macro, we should say so somewhere. But I would like to understand why we're not 
simply processing the whole file.



Comment at: clang/lib/Format/Macros.h:147
+///
+/// Creates the unwrapped lines containing the macro call tokens so that
+/// the macro call tokens fit the semantic structure of the expanded formatted

this says "creates the unwrapped lines" but getResult() returns only one.
Does the plural here refer to the tree? Maybe just use singular or "the 
unwrappedlinetree"?



Comment at: clang/lib/Format/Macros.h:154
+/// -> })
+class MacroUnexpander {
+public:

I get the symmetry between the expander/unexpander classes, but the name is 
making it harder for me to follow the code.
 - the extra compound+negation in the name makes it hard/slow to understand, as 
I'm always thinking first about expansion
 - the fact that expansion/unexpansion are not actually opposites completely 
breaks my intuition. It also creates two different meanings of "unexpanded" 
that led me down the garden path a bit (e.g. in the test).

Would you consider `MacroCollapser`? It's artificial, but expand/collapse are 
well-known antonyms without being the same word.

(Incidentally, I just realized this is why I find "UnwrappedLine" so slippery - 
the ambiguity between "this isn't wrapped" and "this was unwrapped")



Comment at: clang/lib/Format/Macros.h:164
+  /// For the given \p Line, match all occurences of tokens expanded from a
+  /// macro and replace them with the original macro call in \c getResult().
+  void addLine(const UnwrappedLine );

I find this hard to follow.
- again, the "match" part is an implementation detail that sounds interesting 
but isn't :-)
- "from a macro" sounds like one in particular, but is actually every macro
- the bit about "getResult" is a separate point that feels shoehorned in

What about:
"Replaces tokens that were expanded from macros with the original macro calls. 
The result is stored and available in getResult()"



Comment at: 

[PATCH] D89238: [clangd] Go-to-definition from non-renaming alias is unambiguous.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

It's not helpful to show the alias itself as an option.
This fixes a regression accepted in f24649b77d856157c64841457dcc4f70530d607c 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89238

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1118,13 +1118,12 @@
   // decls.
   R"cpp(
   namespace ns { class [[Foo]] {}; }
-  // FIXME: don't return the using decl if it touches the cursor position.
-  using ns::[[F^oo]];
+  using ns::F^oo;
 )cpp",
 
   R"cpp(
   namespace ns { int [[x]](char); int [[x]](double); }
-  using ns::[[^x]];
+  using ns::^x;
 )cpp",
 
   R"cpp(
@@ -1157,7 +1156,7 @@
   };
   template 
   struct Derived : Base {
-using Base::[[w^aldo]];
+using Base::w^aldo;
   };
 )cpp",
   };
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -171,22 +171,33 @@
   return Merged.CanonicalDeclaration;
 }
 
+std::vector>
+getDeclAtPositionWithRelations(ParsedAST , SourceLocation Pos,
+   DeclRelationSet Relations,
+   ASTNodeKind *NodeKind = nullptr) {
+  unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
+  std::vector> Result;
+  auto ResultFromTree = [&](SelectionTree ST) {
+if (const SelectionTree::Node *N = ST.commonAncestor()) {
+  if (NodeKind)
+*NodeKind = N->ASTNode.getNodeKind();
+  llvm::copy_if(allTargetDecls(N->ASTNode), std::back_inserter(Result),
+[&](auto ) { return !(Entry.second & ~Relations); });
+}
+return !Result.empty();
+  };
+  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Offset,
+Offset, ResultFromTree);
+  return Result;
+}
+
 std::vector
 getDeclAtPosition(ParsedAST , SourceLocation Pos, DeclRelationSet Relations,
   ASTNodeKind *NodeKind = nullptr) {
-  unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
   std::vector Result;
-  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Offset,
-Offset, [&](SelectionTree ST) {
-  if (const SelectionTree::Node *N =
-  ST.commonAncestor()) {
-if (NodeKind)
-  *NodeKind = N->ASTNode.getNodeKind();
-llvm::copy(targetDecl(N->ASTNode, Relations),
-   std::back_inserter(Result));
-  }
-  return !Result.empty();
-});
+  for (auto  :
+   getDeclAtPositionWithRelations(AST, Pos, Relations, NodeKind))
+Result.push_back(Entry.first);
   return Result;
 }
 
@@ -316,8 +327,10 @@
   // Emit all symbol locations (declaration or definition) from AST.
   DeclRelationSet Relations =
   DeclRelation::TemplatePattern | DeclRelation::Alias;
-  for (const NamedDecl *D :
-   getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
+  auto Candidates =
+  getDeclAtPositionWithRelations(AST, CurLoc, Relations, NodeKind);
+  for (const auto  : Candidates) {
+const NamedDecl *D = E.first;
 // Special case: void foo() ^override: jump to the overridden method.
 if (const auto *CMD = llvm::dyn_cast(D)) {
   const InheritableAttr *Attr = D->getAttr();
@@ -333,6 +346,18 @@
   }
 }
 
+// Special case: the cursor is on an alias, prefer other results.
+// This targets "using ns::^Foo", where the target is more interesting.
+// This does not trigger on renaming aliases:
+//   `using Foo = ^Bar` already targets Bar via a TypeLoc
+//   `using ^Foo = Bar` has no other results, as Underlying is filtered.
+if (E.second & DeclRelation::Alias && Candidates.size() > 1 &&
+// beginLoc/endLoc are a token range, so rewind the identifier we're in.
+SM.isPointWithin(TouchedIdentifier ? TouchedIdentifier->location()
+   : CurLoc,
+ D->getBeginLoc(), D->getEndLoc()))
+  continue;
+
 // Special case: the point of declaration of a template 

[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:72
 
+  /// Profiles resource-usage. No-op if there's no active tracer.
+  void profile(MemoryTree ) const;

drop the no-op part?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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


[PATCH] D89233: [clangd] Refine recoveryAST flags in clangd

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This is fine as is, but could consider retiring the flags instead.




Comment at: clang-tools-extra/clangd/ClangdServer.h:131
+/// If true, turn on the `-frecovery-ast` clang flag;
+/// If false, respect to the value in clang.
+bool BuildRecoveryAST = false;

nit: respect to -> respect

This could just be "If true, force -frecovery-ast-type"



Comment at: clang-tools-extra/clangd/Compiler.cpp:84
 
-  // Recovery expression currently only works for C++.
-  if (CI->getLangOpts()->CPlusPlus) {
-CI->getLangOpts()->RecoveryAST = Inputs.Opts.BuildRecoveryAST;
-CI->getLangOpts()->RecoveryASTType = Inputs.Opts.PreserveRecoveryASTType;
-  }
+  if (Inputs.Opts.BuildRecoveryAST)
+CI->getLangOpts()->RecoveryAST = true;

I don't have any problem with the changes to the flags, but we can also do 
experiments just with config now:

```
CompileFlags:
  Add: [-Xclang, -frecovery-ast]
```

in ~/.config/clangd/config.yaml


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89233

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


[PATCH] D89131: [clangd] Validate optional fields more strictly.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Sorry, I thought this was accepted. Had addressed the two comments, happy to 
address more or revert, LMK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89131

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


[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8f1de22c7681: [clangd] Stop capturing trace args if the 
tracer doesnt need them. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89135

Files:
  clang-tools-extra/clangd/support/Trace.cpp
  clang-tools-extra/clangd/support/Trace.h
  clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Index: clang-tools-extra/clangd/unittests/support/TraceTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -186,6 +186,11 @@
  StartsWith("d,dist,\"a\nb\",1"), ""));
 }
 
+TEST_F(CSVMetricsTracerTest, IgnoresArgs) {
+  trace::Span Tracer("Foo");
+  EXPECT_EQ(nullptr, Tracer.Args);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/support/Trace.h
===
--- clang-tools-extra/clangd/support/Trace.h
+++ clang-tools-extra/clangd/support/Trace.h
@@ -79,8 +79,13 @@
   /// Returns a derived context that will be destroyed when the event ends.
   /// Usually implementations will store an object in the returned context
   /// whose destructor records the end of the event.
-  /// The args are *Args, only complete when the event ends.
-  virtual Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args);
+  /// The tracer may capture event details provided in SPAN_ATTACH() calls.
+  /// In this case it should call AttachDetails(), and pass in an empty Object
+  /// to hold them. This Object should be owned by the context, and the data
+  /// will be complete by the time the context is destroyed.
+  virtual Context
+  beginSpan(llvm::StringRef Name,
+llvm::function_ref AttachDetails);
   // Called when a Span is destroyed (it may still be active on other threads).
   // beginSpan() and endSpan() will always form a proper stack on each thread.
   // The Context returned by beginSpan is active, but Args is not ready.
@@ -146,6 +151,8 @@
   llvm::json::Object *const Args;
 
 private:
+  // Awkward constructor works around constant initialization.
+  Span(std::pair);
   WithContext RestoreCtx;
 };
 
Index: clang-tools-extra/clangd/support/Trace.cpp
===
--- clang-tools-extra/clangd/support/Trace.cpp
+++ clang-tools-extra/clangd/support/Trace.cpp
@@ -53,9 +53,12 @@
 
   // We stash a Span object in the context. It will record the start/end,
   // and this also allows us to look up the parent Span's information.
-  Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args) override {
-return Context::current().derive(
-SpanKey, std::make_unique(this, Name, Args));
+  Context beginSpan(
+  llvm::StringRef Name,
+  llvm::function_ref AttachDetails) override {
+auto JS = std::make_unique(this, Name);
+AttachDetails(>Args);
+return Context::current().derive(SpanKey, std::move(JS));
   }
 
   // Trace viewer requires each thread to properly stack events.
@@ -85,9 +88,9 @@
 private:
   class JSONSpan {
   public:
-JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object *Args)
+JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)
 : StartTime(Tracer->timestamp()), EndTime(0), Name(Name),
-  TID(llvm::get_threadid()), Tracer(Tracer), Args(Args) {
+  TID(llvm::get_threadid()), Tracer(Tracer) {
   // ~JSONSpan() may run in a different thread, so we need to capture now.
   Tracer->captureThreadMetadata();
 
@@ -125,7 +128,7 @@
   // Finally, record the event (ending at EndTime, not timestamp())!
   Tracer->jsonEvent("X",
 llvm::json::Object{{"name", std::move(Name)},
-   {"args", std::move(*Args)},
+   {"args", std::move(Args)},
{"dur", EndTime - StartTime}},
 TID, StartTime);
 }
@@ -133,6 +136,8 @@
 // May be called by any thread.
 void markEnded() { EndTime = Tracer->timestamp(); }
 
+llvm::json::Object Args;
+
   private:
 static int64_t nextID() {
   static std::atomic Next = {0};
@@ -144,7 +149,6 @@
 std::string Name;
 uint64_t TID;
 JSONTracer *Tracer;
-llvm::json::Object *Args;
   };
   static Key> SpanKey;
 
@@ -277,12 +281,11 @@
   T->instant("Log", llvm::json::Object{{"Message", Message.str()}});
 }
 
-// Returned context owns Args.
-static Context makeSpanContext(llvm::Twine Name, llvm::json::Object *Args,
-   const Metric ) {
+// The JSON object is event args (owned by 

[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 297555.
sammccall marked an inline comment as done.
sammccall added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89135

Files:
  clang-tools-extra/clangd/support/Trace.cpp
  clang-tools-extra/clangd/support/Trace.h
  clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Index: clang-tools-extra/clangd/unittests/support/TraceTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -186,6 +186,11 @@
  StartsWith("d,dist,\"a\nb\",1"), ""));
 }
 
+TEST_F(CSVMetricsTracerTest, IgnoresArgs) {
+  trace::Span Tracer("Foo");
+  EXPECT_EQ(nullptr, Tracer.Args);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/support/Trace.h
===
--- clang-tools-extra/clangd/support/Trace.h
+++ clang-tools-extra/clangd/support/Trace.h
@@ -79,8 +79,13 @@
   /// Returns a derived context that will be destroyed when the event ends.
   /// Usually implementations will store an object in the returned context
   /// whose destructor records the end of the event.
-  /// The args are *Args, only complete when the event ends.
-  virtual Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args);
+  /// The tracer may capture event details provided in SPAN_ATTACH() calls.
+  /// In this case it should call AttachDetails(), and pass in an empty Object
+  /// to hold them. This Object should be owned by the context, and the data
+  /// will be complete by the time the context is destroyed.
+  virtual Context
+  beginSpan(llvm::StringRef Name,
+llvm::function_ref AttachDetails);
   // Called when a Span is destroyed (it may still be active on other threads).
   // beginSpan() and endSpan() will always form a proper stack on each thread.
   // The Context returned by beginSpan is active, but Args is not ready.
@@ -146,6 +151,8 @@
   llvm::json::Object *const Args;
 
 private:
+  // Awkward constructor works around constant initialization.
+  Span(std::pair);
   WithContext RestoreCtx;
 };
 
Index: clang-tools-extra/clangd/support/Trace.cpp
===
--- clang-tools-extra/clangd/support/Trace.cpp
+++ clang-tools-extra/clangd/support/Trace.cpp
@@ -53,9 +53,12 @@
 
   // We stash a Span object in the context. It will record the start/end,
   // and this also allows us to look up the parent Span's information.
-  Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args) override {
-return Context::current().derive(
-SpanKey, std::make_unique(this, Name, Args));
+  Context beginSpan(
+  llvm::StringRef Name,
+  llvm::function_ref AttachDetails) override {
+auto JS = std::make_unique(this, Name);
+AttachDetails(>Args);
+return Context::current().derive(SpanKey, std::move(JS));
   }
 
   // Trace viewer requires each thread to properly stack events.
@@ -85,9 +88,9 @@
 private:
   class JSONSpan {
   public:
-JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object *Args)
+JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)
 : StartTime(Tracer->timestamp()), EndTime(0), Name(Name),
-  TID(llvm::get_threadid()), Tracer(Tracer), Args(Args) {
+  TID(llvm::get_threadid()), Tracer(Tracer) {
   // ~JSONSpan() may run in a different thread, so we need to capture now.
   Tracer->captureThreadMetadata();
 
@@ -125,7 +128,7 @@
   // Finally, record the event (ending at EndTime, not timestamp())!
   Tracer->jsonEvent("X",
 llvm::json::Object{{"name", std::move(Name)},
-   {"args", std::move(*Args)},
+   {"args", std::move(Args)},
{"dur", EndTime - StartTime}},
 TID, StartTime);
 }
@@ -133,6 +136,8 @@
 // May be called by any thread.
 void markEnded() { EndTime = Tracer->timestamp(); }
 
+llvm::json::Object Args;
+
   private:
 static int64_t nextID() {
   static std::atomic Next = {0};
@@ -144,7 +149,6 @@
 std::string Name;
 uint64_t TID;
 JSONTracer *Tracer;
-llvm::json::Object *Args;
   };
   static Key> SpanKey;
 
@@ -277,12 +281,11 @@
   T->instant("Log", llvm::json::Object{{"Message", Message.str()}});
 }
 
-// Returned context owns Args.
-static Context makeSpanContext(llvm::Twine Name, llvm::json::Object *Args,
-   const Metric ) {
+// The JSON object is event args (owned by context), if the tracer wants them.
+static std::pair
+makeSpanContext(llvm::Twine Name, const Metric ) {
   if (!T)
-

[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/support/Trace.cpp:91
   public:
-JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object 
*Args)
+llvm::json::Object Args;
+JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)

kadircet wrote:
> move this near other fileds?
It's public unlike the other fields, but moved it to the bottom of the public 
section



Comment at: clang-tools-extra/clangd/support/Trace.cpp:302
+: llvm::StringRef(Name.str()),
+   [&](llvm::json::Object *A) { Args = A; });
+  return std::make_pair(std::move(Ctx), Args);

added assertions here (nonnull and empty) to guard against any API confusion



Comment at: clang-tools-extra/clangd/support/Trace.h:86
+  beginSpan(llvm::StringRef Name,
+llvm::function_ref RequestArgs);
   // Called when a Span is destroyed (it may still be active on other threads).

kadircet wrote:
> nit: at first i parsed request in `RequestArgs` as a verb (e.g. `i request 
> arguments`), which was confusing a little bit. Hence the comment also didn't 
> make much sense, I only managed to error correct after seeing the usage :D
> 
> Maybe something like `SetRequestArgs` to emphasize that one provides request 
> arguments using this lambda rather than receiving them?
This *is* actually the intention.

Changed to `AttachDetails` to avoid ambiguity, and rewrote the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89135

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


[PATCH] D89131: [clangd] Validate optional fields more strictly.

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.
Closed by commit rGc2d4280328e4: [clangd] Validate optional fields more 
strictly. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D89131?vs=297228=297547#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89131

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

Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -484,12 +484,10 @@
 bool fromJSON(const llvm::json::Value , DidChangeTextDocumentParams ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O)
-return false;
-  O.map("forceRebuild", R.forceRebuild);  // Optional clangd extension.
-  return O.map("textDocument", R.textDocument) &&
+  return O && O.map("textDocument", R.textDocument) &&
  O.map("contentChanges", R.contentChanges) &&
- O.map("wantDiagnostics", R.wantDiagnostics);
+ O.map("wantDiagnostics", R.wantDiagnostics) &&
+ O.mapOptional("forceRebuild", R.forceRebuild);
 }
 
 bool fromJSON(const llvm::json::Value , FileChangeType ,
@@ -578,12 +576,10 @@
 bool fromJSON(const llvm::json::Value , Diagnostic ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O || !O.map("range", R.range) || !O.map("message", R.message))
-return false;
-  O.map("severity", R.severity);
-  O.map("category", R.category);
-  O.map("code", R.code);
-  O.map("source", R.source);
+  return O && O.map("range", R.range) && O.map("message", R.message) &&
+ O.mapOptional("severity", R.severity) &&
+ O.mapOptional("category", R.category) &&
+ O.mapOptional("code", R.code) && O.mapOptional("source", R.source);
   return true;
 }
 
@@ -800,10 +796,8 @@
 bool fromJSON(const llvm::json::Value , ApplyWorkspaceEditResponse ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Response, P);
-  if (!O || !O.map("applied", R.applied))
-return false;
-  O.map("failureReason", R.failureReason);
-  return true;
+  return O && O.map("applied", R.applied) &&
+ O.map("failureReason", R.failureReason);
 }
 
 bool fromJSON(const llvm::json::Value , TextDocumentPositionParams ,
@@ -816,16 +810,11 @@
 bool fromJSON(const llvm::json::Value , CompletionContext ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O)
-return false;
-
   int TriggerKind;
-  if (!O.map("triggerKind", TriggerKind))
+  if (!O || !O.map("triggerKind", TriggerKind) ||
+  !O.mapOptional("triggerCharacter", R.triggerCharacter))
 return false;
   R.triggerKind = static_cast(TriggerKind);
-
-  if (auto *TC = Params.getAsObject()->get("triggerCharacter"))
-return fromJSON(*TC, R.triggerCharacter, P.field("triggerCharacter"));
   return true;
 }
 
@@ -1126,8 +1115,8 @@
   llvm::json::ObjectMapper O(Params, P);
   if (!O)
 return true; // 'any' type in LSP.
-  O.map("compilationDatabaseChanges", S.compilationDatabaseChanges);
-  return true;
+  return O.mapOptional("compilationDatabaseChanges",
+   S.compilationDatabaseChanges);
 }
 
 bool fromJSON(const llvm::json::Value , InitializationOptions ,
@@ -1136,11 +1125,10 @@
   if (!O)
 return true; // 'any' type in LSP.
 
-  fromJSON(Params, Opts.ConfigSettings, P);
-  O.map("compilationDatabasePath", Opts.compilationDatabasePath);
-  O.map("fallbackFlags", Opts.fallbackFlags);
-  O.map("clangdFileStatus", Opts.FileStatus);
-  return true;
+  return fromJSON(Params, Opts.ConfigSettings, P) &&
+ O.map("compilationDatabasePath", Opts.compilationDatabasePath) &&
+ O.mapOptional("fallbackFlags", Opts.fallbackFlags) &&
+ O.mapOptional("clangdFileStatus", Opts.FileStatus);
 }
 
 bool fromJSON(const llvm::json::Value , TypeHierarchyDirection ,
@@ -1193,20 +1181,13 @@
   llvm::json::ObjectMapper O(Params, P);
 
   // Required fields.
-  if (!(O && O.map("name", I.name) && O.map("kind", I.kind) &&
-O.map("uri", I.uri) && O.map("range", I.range) &&
-O.map("selectionRange", I.selectionRange))) {
-return false;
-  }
-
-  // Optional fields.
-  O.map("detail", I.detail);
-  O.map("deprecated", I.deprecated);
-  O.map("parents", I.parents);
-  O.map("children", I.children);
-  O.map("data", I.data);
-
-  return true;
+  return O && O.map("name", I.name) && O.map("kind", I.kind) &&
+ O.map("uri", I.uri) && O.map("range", I.range) &&
+ O.map("selectionRange", I.selectionRange) &&
+ O.mapOptional("detail", I.detail) &&
+ O.mapOptional("deprecated", I.deprecated) &&
+   

[PATCH] D88463: [clangd] Try harder to get accurate ranges for documentSymbols in macros

2020-10-12 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! This seems totally complete now, though I bet there's another case 
possible somehow!




Comment at: clang-tools-extra/clangd/FindSymbols.cpp:157
+  SourceLocation FallbackNameLoc;
+  if (isSpelledInSource(Loc, SM)) {
+// Prefer the spelling loc, but save the expansion loc as a fallback.

Hmm... the common file-location case takes a pretty weird path here: both 
NameLoc and FallbackNameLoc end up set to Loc, and hopefully we never hit the 
!contains condition (but if we did, we'd just calculate it twice and then hit 
the second fallback)

Maybe clearer, though a little longer, to handle explicitly like

```
SourceLocation NameLoc = ND.getLocation();
SourceLocation FallbackNameLoc;
if (NameLoc.isMacroLocation()) {
  if (spelled in source) {
NameLoc = getSpelling
FallbackNameLoc = getExpanson
  } else {
NameLoc = getExpansion
  }
}
```

up to you



Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:637
+  AllOf(WithName("Test"), SymNameRange(Main.range("expansion2"))),
+  AllOf(WithName("waldo"),
+SymRangeContains(Main.range("bodyStatement");

nit: despite the "contains" relation being critical, I think it's worth 
asserting the exact range


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88463

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


[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

The tracer is now expected to allocate+free the args itself.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89135

Files:
  clang-tools-extra/clangd/support/Trace.cpp
  clang-tools-extra/clangd/support/Trace.h
  clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Index: clang-tools-extra/clangd/unittests/support/TraceTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -186,6 +186,11 @@
  StartsWith("d,dist,\"a\nb\",1"), ""));
 }
 
+TEST_F(CSVMetricsTracerTest, IgnoresArgs) {
+  trace::Span Tracer("Foo");
+  EXPECT_EQ(nullptr, Tracer.Args);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/support/Trace.h
===
--- clang-tools-extra/clangd/support/Trace.h
+++ clang-tools-extra/clangd/support/Trace.h
@@ -79,8 +79,11 @@
   /// Returns a derived context that will be destroyed when the event ends.
   /// Usually implementations will store an object in the returned context
   /// whose destructor records the end of the event.
-  /// The args are *Args, only complete when the event ends.
-  virtual Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args);
+  /// If the tracer wants event details, it should call RequestArgs. The pointer
+  /// must be valid over the whole event, and will be populated before it ends.
+  virtual Context
+  beginSpan(llvm::StringRef Name,
+llvm::function_ref RequestArgs);
   // Called when a Span is destroyed (it may still be active on other threads).
   // beginSpan() and endSpan() will always form a proper stack on each thread.
   // The Context returned by beginSpan is active, but Args is not ready.
@@ -146,6 +149,8 @@
   llvm::json::Object *const Args;
 
 private:
+  // Awkward constructor works around constant initialization.
+  Span(std::pair);
   WithContext RestoreCtx;
 };
 
Index: clang-tools-extra/clangd/support/Trace.cpp
===
--- clang-tools-extra/clangd/support/Trace.cpp
+++ clang-tools-extra/clangd/support/Trace.cpp
@@ -53,9 +53,12 @@
 
   // We stash a Span object in the context. It will record the start/end,
   // and this also allows us to look up the parent Span's information.
-  Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args) override {
-return Context::current().derive(
-SpanKey, std::make_unique(this, Name, Args));
+  Context beginSpan(
+  llvm::StringRef Name,
+  llvm::function_ref RequestArgs) override {
+auto JS = std::make_unique(this, Name);
+RequestArgs(>Args);
+return Context::current().derive(SpanKey, std::move(JS));
   }
 
   // Trace viewer requires each thread to properly stack events.
@@ -85,9 +88,10 @@
 private:
   class JSONSpan {
   public:
-JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object *Args)
+llvm::json::Object Args;
+JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)
 : StartTime(Tracer->timestamp()), EndTime(0), Name(Name),
-  TID(llvm::get_threadid()), Tracer(Tracer), Args(Args) {
+  TID(llvm::get_threadid()), Tracer(Tracer) {
   // ~JSONSpan() may run in a different thread, so we need to capture now.
   Tracer->captureThreadMetadata();
 
@@ -125,7 +129,7 @@
   // Finally, record the event (ending at EndTime, not timestamp())!
   Tracer->jsonEvent("X",
 llvm::json::Object{{"name", std::move(Name)},
-   {"args", std::move(*Args)},
+   {"args", std::move(Args)},
{"dur", EndTime - StartTime}},
 TID, StartTime);
 }
@@ -144,7 +148,6 @@
 std::string Name;
 uint64_t TID;
 JSONTracer *Tracer;
-llvm::json::Object *Args;
   };
   static Key> SpanKey;
 
@@ -277,12 +280,11 @@
   T->instant("Log", llvm::json::Object{{"Message", Message.str()}});
 }
 
-// Returned context owns Args.
-static Context makeSpanContext(llvm::Twine Name, llvm::json::Object *Args,
-   const Metric ) {
+// The JSON object is event args (owned by context), if the tracer wants them.
+static std::pair
+makeSpanContext(llvm::Twine Name, const Metric ) {
   if (!T)
-return Context::current().clone();
-  WithContextValue WithArgs{std::unique_ptr(Args)};
+return std::make_pair(Context::current().clone(), nullptr);
   llvm::Optional WithLatency;
   using Clock = 

[PATCH] D89131: [clangd] Validate optional fields more strictly.

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89131

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

Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -484,12 +484,10 @@
 bool fromJSON(const llvm::json::Value , DidChangeTextDocumentParams ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O)
-return false;
-  O.map("forceRebuild", R.forceRebuild);  // Optional clangd extension.
-  return O.map("textDocument", R.textDocument) &&
+  return O && O.map("textDocument", R.textDocument) &&
  O.map("contentChanges", R.contentChanges) &&
- O.map("wantDiagnostics", R.wantDiagnostics);
+ O.map("wantDiagnostics", R.wantDiagnostics) &&
+ O.mapOptional("forceRebuild", R.forceRebuild);
 }
 
 bool fromJSON(const llvm::json::Value , FileChangeType ,
@@ -578,12 +576,10 @@
 bool fromJSON(const llvm::json::Value , Diagnostic ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O || !O.map("range", R.range) || !O.map("message", R.message))
-return false;
-  O.map("severity", R.severity);
-  O.map("category", R.category);
-  O.map("code", R.code);
-  O.map("source", R.source);
+  return O && O.map("range", R.range) && O.map("message", R.message) &&
+ O.mapOptional("severity", R.severity) &&
+ O.mapOptional("category", R.category) &&
+ O.mapOptional("code", R.code) && O.mapOptional("source", R.source);
   return true;
 }
 
@@ -800,10 +796,8 @@
 bool fromJSON(const llvm::json::Value , ApplyWorkspaceEditResponse ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Response, P);
-  if (!O || !O.map("applied", R.applied))
-return false;
-  O.map("failureReason", R.failureReason);
-  return true;
+  return O && O.map("applied", R.applied) &&
+ O.mapOptional("failureReason", R.failureReason);
 }
 
 bool fromJSON(const llvm::json::Value , TextDocumentPositionParams ,
@@ -816,16 +810,11 @@
 bool fromJSON(const llvm::json::Value , CompletionContext ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O)
-return false;
-
   int TriggerKind;
-  if (!O.map("triggerKind", TriggerKind))
+  if (!O || !O.map("triggerKind", TriggerKind) ||
+  !O.mapOptional("triggerCharacter", R.triggerCharacter))
 return false;
   R.triggerKind = static_cast(TriggerKind);
-
-  if (auto *TC = Params.getAsObject()->get("triggerCharacter"))
-return fromJSON(*TC, R.triggerCharacter, P.field("triggerCharacter"));
   return true;
 }
 
@@ -1126,8 +1115,8 @@
   llvm::json::ObjectMapper O(Params, P);
   if (!O)
 return true; // 'any' type in LSP.
-  O.map("compilationDatabaseChanges", S.compilationDatabaseChanges);
-  return true;
+  return O.mapOptional("compilationDatabaseChanges",
+   S.compilationDatabaseChanges);
 }
 
 bool fromJSON(const llvm::json::Value , InitializationOptions ,
@@ -1136,11 +1125,11 @@
   if (!O)
 return true; // 'any' type in LSP.
 
-  fromJSON(Params, Opts.ConfigSettings, P);
-  O.map("compilationDatabasePath", Opts.compilationDatabasePath);
-  O.map("fallbackFlags", Opts.fallbackFlags);
-  O.map("clangdFileStatus", Opts.FileStatus);
-  return true;
+  return fromJSON(Params, Opts.ConfigSettings, P) &&
+ O.mapOptional("compilationDatabasePath",
+   Opts.compilationDatabasePath) &&
+ O.mapOptional("fallbackFlags", Opts.fallbackFlags) &&
+ O.mapOptional("clangdFileStatus", Opts.FileStatus);
 }
 
 bool fromJSON(const llvm::json::Value , TypeHierarchyDirection ,
@@ -1193,20 +1182,13 @@
   llvm::json::ObjectMapper O(Params, P);
 
   // Required fields.
-  if (!(O && O.map("name", I.name) && O.map("kind", I.kind) &&
-O.map("uri", I.uri) && O.map("range", I.range) &&
-O.map("selectionRange", I.selectionRange))) {
-return false;
-  }
-
-  // Optional fields.
-  O.map("detail", I.detail);
-  O.map("deprecated", I.deprecated);
-  O.map("parents", I.parents);
-  O.map("children", I.children);
-  O.map("data", I.data);
-
-  return true;
+  return O && O.map("name", I.name) && O.map("kind", I.kind) &&
+ O.map("uri", I.uri) && O.map("range", I.range) &&
+ O.map("selectionRange", I.selectionRange) &&
+ O.mapOptional("detail", I.detail) &&
+ O.mapOptional("deprecated", I.deprecated) &&
+ O.mapOptional("parents", I.parents) &&
+ O.mapOptional("children", I.children) && O.mapOptional("data", I.data);
 }
 
 bool fromJSON(const llvm::json::Value ,

[PATCH] D89126: [clangd] Support CodeActionParams.only

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89126

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/code-action-request.test

Index: clang-tools-extra/clangd/test/code-action-request.test
===
--- clang-tools-extra/clangd/test/code-action-request.test
+++ clang-tools-extra/clangd/test/code-action-request.test
@@ -51,6 +51,47 @@
 # CHECK-NEXT:}
 # CHECK-NEXT:  ]
 ---
+{
+  "jsonrpc": "2.0",
+  "id": 2,
+  "method": "textDocument/codeAction",
+  "params": {
+"textDocument": { "uri": "test:///main.cpp" },
+"range": {
+"start": {"line": 0, "character": 0},
+"end": {"line": 0, "character": 4}
+},
+"context": {
+"diagnostics": [],
+"only": ["quickfix"]
+}
+}
+}
+#  CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": []
+---
+{
+  "jsonrpc": "2.0",
+  "id": 3,
+  "method": "textDocument/codeAction",
+  "params": {
+"textDocument": { "uri": "test:///main.cpp" },
+"range": {
+"start": {"line": 0, "character": 0},
+"end": {"line": 0, "character": 4}
+},
+"context": {
+"diagnostics": [],
+"only": ["refactor"]
+}
+}
+}
+#  CHECK:  "id": 3,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+---
 {"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"test:///main.cpp","selection":{"end":{"character":4,"line":0},"start":{"character":0,"line":0}},"tweakID":"ExpandAutoType"}]}}
 #  CHECK:"newText": "int",
 # CHECK-NEXT:"range": {
@@ -64,7 +105,7 @@
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---
-{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
 ---
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -862,8 +862,19 @@
 llvm::json::Value toJSON(const PublishDiagnosticsParams &);
 
 struct CodeActionContext {
-  /// An array of diagnostics.
+  /// An array of diagnostics known on the client side overlapping the range
+  /// provided to the `textDocument/codeAction` request. They are provided so
+  /// that the server knows which errors are currently presented to the user for
+  /// the given range. There is no guarantee that these accurately reflect the
+  /// error state of the resource. The primary parameter to compute code actions
+  /// is the provided range.
   std::vector diagnostics;
+
+  /// Requested kind of actions to return.
+  ///
+  /// Actions not of this kind are filtered out by the client before being
+  /// shown. So servers can omit computing them.
+  std::vector only;
 };
 bool fromJSON(const llvm::json::Value &, CodeActionContext &, llvm::json::Path);
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -600,7 +600,10 @@
 bool fromJSON(const llvm::json::Value , CodeActionContext ,
   llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  return O && O.map("diagnostics", R.diagnostics);
+  if (!O || !O.map("diagnostics", R.diagnostics))
+return false;
+  O.map("only", R.only);
+  return true;
 }
 
 llvm::raw_ostream <<(llvm::raw_ostream , const Diagnostic ) {
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -984,12 +984,24 @@
   if (!Code)
 return Reply(llvm::make_error(
 "onCodeAction called for non-added file", ErrorCode::InvalidParams));
+
+  // Checks whether a particular CodeActionKind is included in the response.
+  auto KindAllowed = [Only(Params.context.only)](llvm::StringRef Kind) {
+if (Only.empty())
+  return true;
+return llvm::any_of(Only, [&](llvm::StringRef Base) {
+  return Kind.consume_front(Base) && (Kind.empty() || Kind.startswith("."));
+});
+  };
+
   // We provide a code action for Fixes on the specified diagnostics.
   std::vector FixIts;
-  for (const Diagnostic  : Params.context.diagnostics) {
-for (auto  : getFixes(File.file(), D)) {
-  FixIts.push_back(toCodeAction(F, Params.textDocument.uri));
-  

[PATCH] D88724: [clangd] Make the tweak filter a parameter to enumerateTweaks. NFC

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7530b254e93a: [clangd] Make the tweak filter a parameter to 
enumerateTweaks. NFC (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88724

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h

Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -163,11 +163,6 @@
 /// Enable preview of FoldingRanges feature.
 bool FoldingRanges = false;
 
-/// Returns true if the tweak should be enabled.
-std::function TweakFilter = [](const Tweak ) {
-  return !T.hidden(); // only enable non-hidden tweaks.
-};
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
@@ -294,7 +289,9 @@
 llvm::StringLiteral Kind;
   };
   /// Enumerate the code tweaks available to the user at a specified point.
+  /// Tweaks where Filter returns false will not be checked or included.
   void enumerateTweaks(PathRef File, Range Sel,
+   llvm::unique_function Filter,
Callback> CB);
 
   /// Apply the code tweak with a specified \p ID.
@@ -382,8 +379,6 @@
   // If true, preserve the type for recovery AST.
   bool PreserveRecoveryASTType = false;
 
-  std::function TweakFilter;
-
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap>
   CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -180,7 +180,7 @@
   SuggestMissingIncludes(Opts.SuggestMissingIncludes),
   BuildRecoveryAST(Opts.BuildRecoveryAST),
   PreserveRecoveryASTType(Opts.PreserveRecoveryASTType),
-  TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
+  WorkspaceRoot(Opts.WorkspaceRoot),
   // Pass a callback into `WorkScheduler` to extract symbols from a newly
   // parsed file and rebuild the file index synchronously each time an AST
   // is parsed.
@@ -492,13 +492,15 @@
   return std::move(Result);
 }
 
-void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
-   Callback> CB) {
+void ClangdServer::enumerateTweaks(
+PathRef File, Range Sel, llvm::unique_function Filter,
+Callback> CB) {
   // Tracks number of times a tweak has been offered.
   static constexpr trace::Metric TweakAvailable(
   "tweak_available", trace::Metric::Counter, "tweak_id");
   auto Action = [File = File.str(), Sel, CB = std::move(CB),
- this](Expected InpAST) mutable {
+ Filter =
+ std::move(Filter)](Expected InpAST) mutable {
 if (!InpAST)
   return CB(InpAST.takeError());
 auto Selections = tweakSelection(Sel, *InpAST);
@@ -507,11 +509,11 @@
 std::vector Res;
 // Don't allow a tweak to fire more than once across ambiguous selections.
 llvm::DenseSet PreparedTweaks;
-auto Filter = [&](const Tweak ) {
-  return TweakFilter(T) && !PreparedTweaks.count(T.id());
+auto DeduplicatingFilter = [&](const Tweak ) {
+  return Filter(T) && !PreparedTweaks.count(T.id());
 };
 for (const auto  : *Selections) {
-  for (auto  : prepareTweaks(*Sel, Filter)) {
+  for (auto  : prepareTweaks(*Sel, DeduplicatingFilter)) {
 Res.push_back({T->id(), T->title(), T->kind()});
 PreparedTweaks.insert(T->id());
 TweakAvailable.record(1, T->id());
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -50,6 +50,10 @@
 /// per-request, but LSP allows limited/no customizations.
 clangd::CodeCompleteOptions CodeComplete;
 clangd::RenameOptions Rename;
+/// Returns true if the tweak should be enabled.
+std::function TweakFilter = [](const Tweak ) {
+  return !T.hidden(); // only enable non-hidden tweaks.
+};
   };
 
   ClangdLSPServer(Transport , const ThreadsafeFS ,
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1030,7 +1030,15 @@
 return Reply(llvm::json::Array(Commands));
   };
 
-  Server->enumerateTweaks(File.file(), Params.range, std::move(ConsumeActions));
+  Server->enumerateTweaks(
+  File.file(), Params.range,
+  

[PATCH] D88204: [clangd] Drop path suffix mapping for std symbols

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Taking another look at the header list, there are a couple of classes of 
symbols beyond c/c++ standard library.

We don't have an alternative database for these. Maybe we should keep the 
mechanism but stop using it for the standard library? What do you think?




Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:49
   /// in  need to be mapped individually). Approximately, the following
   /// system headers are handled:
   ///   - C++ standard library e.g. bits/basic_string.h$ -> 

should we update these comments?



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:51
   ///   - C++ standard library e.g. bits/basic_string.h$ -> 
   ///   - Posix library e.g. bits/pthreadtypes.h$ -> 
   ///   - Compiler extensions, e.g. include/avx512bwintrin.h$ -> 

hmm, are we going to regress all of posix?



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:52
   ///   - Posix library e.g. bits/pthreadtypes.h$ -> 
   ///   - Compiler extensions, e.g. include/avx512bwintrin.h$ -> 
   /// The mapping is hardcoded and hand-maintained, so it might not cover all

and builtins, these are actually owned by us so should be portable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88204

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


[PATCH] D89046: [AST] Build recovery expression by default for all language.

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang/test/CodeGen/builtins-ppc-error.c:51
 void testCTF(int index) {
-  vec_ctf(vsi, index); //expected-error {{argument to 
'__builtin_altivec_vcfsx' must be a constant integer}}
-  vec_ctf(vui, index); //expected-error {{argument to 
'__builtin_altivec_vcfsx' must be a constant integer}}
+  vec_ctf(vsi, index); //expected-error {{argument to 
'__builtin_altivec_vcfsx' must be a constant integer}} expected-error 
{{argument to '__builtin_altivec_vcfux' must be a constant integer}}
+  vec_ctf(vui, index); //expected-error {{argument to 
'__builtin_altivec_vcfsx' must be a constant integer}} expected-error 
{{argument to '__builtin_altivec_vcfux' must be a constant integer}}

hmm, this doesn't exactly look right to me - we know the type of `vsi` so we 
should only be considering the signed case I thought.

However the existing diagnostic for `vui` indicates that it's considering the 
**signed** case, so I guess this is already broken/bad.



Comment at: clang/test/CodeGen/builtins-systemz-zvector-error.c:424
+  vsc = vec_rl_mask(vsc, vuc, idx); // expected-error {{no matching function}} 
\
+// expected-error {{argument to 
'__builtin_s390_verimb' must be a constant integer}} \
+// expected-error {{argument to 
'__builtin_s390_verimh' must be a constant integer}} \

you may want to make these assertions fuzzier and possibly give a count rather 
than repeating them, but I guess the owners of these headers would be best to 
decide



Comment at: clang/test/Index/complete-switch.c:9
 
-// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:4:10 %s | 
FileCheck %s -allow-empty
+// RUN: not %clang_cc1 -fsyntax-only -Xclang -fno-recovery-ast 
-code-completion-at=%s:4:10 %s | FileCheck %s -allow-empty
 // CHECK-NOT: COMPLETION: foo

nit: no need for xclang, this is cc1 already

I guess this is a crash test (and it also doesn't crash with recovery ast)?



Comment at: clang/test/Sema/__try.c:55
+// expected-error{{too few arguments to function call, expected 1, have 
0}} \
+// expected-error{{expected ';' after expression}}
   }

this seems bad, am I missing something?



Comment at: clang/test/Sema/enum.c:104
+  switch (PR7911V); // expected-error {{statement requires expression of 
integer type}} \
+// expected-warning {{switch statement has empty body}} 
expected-note {{put the semicolon on a separate line to silence this warning}}
 }

or just move the semicolon to suppress, pretty sure that's not what's being 
tested here.



Comment at: clang/test/Sema/typo-correction.c:56
+  f(THIS_IS_AN_ERROR,   // expected-error {{use of undeclared identifier 
'THIS_IS_AN_ERROR'}}
+afunction(afunction_)); // expected-error {{use of undeclared identifier 
'afunction_'}}
 }

what's up with this change?
Do we see the LHS is dependent/contains-errors and then give up on correcting 
typos in the RHS?
Should we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89046

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


[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

In D88414#2320859 , @sammccall wrote:

> In D88414#2319161 , @kadircet wrote:
>
>> In D88414#2317106 , @sammccall 
>> wrote:
>>
>>> Now there's lots of usage (which looks good!) i'm finding it a bit hard to 
>>> keep track of what the tree will look like overall.
>>>
>>> At some point it'd be great to:
>>>  a) bind this to an LSP extension so we can see it in editors
>>
>> i was also thinking about it and couldn't decide between a "custom command" 
>> vs "code action".
>>
>> - the former gives a richer interaction, but requires every editor plugin to 
>> implement support.
>> - the latter is a little bit more restrictive but doesn't require a bunch of 
>> extra work.
>>
>> I am happy to go with the "code action" approach initially. WDYT? (not in 
>> the scope of this patch)
>
> I'm pretty leery about code action because it's not at all context-sensitive 
> (not even per-file).

Another slighty silly reason: because of layering, a code action is going to 
have a hard time getting at `ClangdLSPServer`'s profile (or even 
`ClangdServer`'s).
Whereas a custom method will be implemented at that layer.

(We could work around this in various ways, but I think we'll create a bit of a 
mess)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88414

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


[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1234
 
+void ClangdLSPServer::profile(bool Detailed) {
+  if (!trace::enabled())

ClangdLSPServer does own things other than ClangdServer (e.g. the CDB), though 
we don't yet profile them.
Does it make sense to split this into ClangdLSPServer::profile (with the usual 
signature, and maybe public?) and ClangdLSPServer::maybeExportMemoryProfile()?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1234
 
+void ClangdLSPServer::profile(bool Detailed) {
+  if (!trace::enabled())

sammccall wrote:
> ClangdLSPServer does own things other than ClangdServer (e.g. the CDB), 
> though we don't yet profile them.
> Does it make sense to split this into ClangdLSPServer::profile (with the 
> usual signature, and maybe public?) and 
> ClangdLSPServer::maybeExportMemoryProfile()?
Detailed is always false, drop the parameter? This can't be reused by code 
wanting to e.g. service a code action anyway.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1424
+
+  // Delay profiling for a minute, as serious allocations don't happen at
+  // startup.

As written this isn't clearly true or false (what exactly is "startup"?)
Maybe just "Delay first profile until we've finished warming up"?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:165
+  /// requests.
+  std::chrono::steady_clock::time_point NoProfileBefore;
+

or NextProfileTime? (to avoid the negation)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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


[PATCH] D88413: [clangd] Add a metric for tracking memory usage

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:68
 
+  /// Records total memory usage of each node under "memory_usage" metric.
+  /// Labels are edges on the path joined with ".", starting with \p RootName.

can we make this a non-member function and take the metric to write into as a 
parameter?

Putting this here for reuse seems OK to me but it's not an essential part of 
MemoryTree itself.
In particular I'd like to avoid baking magic metrics into support/.

(I know we have one already for fallback latency recording, but that seems much 
costlier to avoid as there are many callsites. Here we expect... 2?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88413

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


[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:557
   // :-(
   llvm::SmallString<256> QName;
   for (const auto  : IncludeFiles)

kadircet wrote:
> this one is no longer needed.
whoops, fixed in 6ee47f552ba7654a0997c8deb71f65d0d91f4a28


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

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


[PATCH] D88814: [clangd] Enable partial namespace matches for workspace symbols

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.

Yeah, we can fairly easily make this tighter if it's noisy. My suspicion is 
people will rather complain about ranking than these results being totally 
irrelevant.
Nice that we now have workspace/symbols users that can send this feedback!




Comment at: clang-tools-extra/clangd/FindSymbols.cpp:47
+bool approximateScopeMatch(llvm::StringRef Scope, llvm::StringRef Query) {
+  // An empty query always matches the scope.
+  if (Query.empty())

there's no need for these to be special cases, the loop handles them correctly

It simplifies the asserts slightly, but seems like false economy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88814

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


[PATCH] D89098: [clang] Fix returning the underlying VarDecl as top-level decl for VarTemplateDecl.

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Is this https://github.com/clangd/clangd/issues/554 ? :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89098

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


[PATCH] D89098: [clang] Fix returning the underlying VarDecl as top-level decl for VarTemplateDecl.

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice catch, LG with readability nits




Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:107
 
+TEST(ParsedASTTest, VarTemplateDecl) {
+  TestTU TU;

(you could fold this into the above test if you want)



Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:112
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+  ElementsAre(AllOf(DeclNamed("X"), WithTemplateArgs("";
+}

Is WithTemplateArgs here testing what you want (args vs params)?
Seems like it would be clearer to assert on DeclKind



Comment at: clang/lib/Parse/ParseDecl.cpp:2212
   // initialize it.
   ThisDecl = VT->getTemplatedDecl();
+  VarTemplateD = VT;

The purpose of the renaming is less than obvious here.

Can we rename VarTemplateDecl to OuterDecl or so? (And change the type to Decl)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89098

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


[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

In D88417#2319307 , @kadircet wrote:

> Bad news, I was testing this with remote-index, hence background-index was 
> turned off. Unfortunately traversing all of the slabs in `FileSymbols` takes 
> quite a while in this case (~15ms for LLVM).
>
> I don't think it is feasible to do this on every notification now, as this 
> implies an extra 15ms latency for interactive requests like code 
> completion/signature help due to the delay between didChange notification and 
> codeCompletion request.

Yeah, 15ms is a lot.
Staring at the code, I think this is our unfortunate index allocation strategy: 
for each source file in the project, we have one slab allocator for symbols, 
one for refs, one for relations...
The factor of 3 is sad but the factor of 1 is what's killing us :-)

So we should fix that someday (DBMS?) but for now, let's back off to measuring 
once every several minutes.

The main problem I see is that we're almost guaranteed to be "unlucky" with the 
sample that way.
e.g. delay = 5 min. On startup there's some notification (initialized? 
didOpen?) that happens before any of the serious allocation and resets the 
counter.
So we won't have a realistic memory usage until we hit the 5 minute mark and 
profile again. If many sessions are <5min then our averages are going to be way 
off.

I can't think of a notification that is ubiquitous but only fires after serious 
work has happened. (Well, publishDiagnostics, but that goes in the wrong 
direction and so runs on the wrong thread).

What about something like a 5 minute throttle, but have ClangdLSPServer's 
constructor set the timestamp to now+1 minute? (Without profiling)




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:152
+  trace::Span Tracer(Detailed ? "ProfileDetailed" : "ProfileBrief");
+  // Exit early if tracing is disabled.
+  if (!Tracer.Args)

I was about to complain that this doesn't work, but it actually does...

This wasn't the intended design of trace/span :-(
The idea was that `Args` would be null if the tracer dynamically wasn't 
interested in event details (e.g. an implementation like CSVMetricsTracer that 
doesn't record event payloads, or a sampling tracer).
In this case !Args would have false positives.

Do you mind adding an explicit trace::enabled() method to Trace.h instead, to 
be more explicit? I'd like to fix the Trace api.
(Else leave as-is and I can do this as a followup)



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

kadircet wrote:
> sammccall wrote:
> > (Sorry, I suspect we discussed this and I forgot)
> > Is there a reason at this point to put knowledge of the core metric in 
> > trace:: rather than define it here locally in ClangdLSPServer?
> > (Sorry, I suspect we discussed this and I forgot)
> 
> Not really.
> 
> > Is there a reason at this point to put knowledge of the core metric in 
> > trace:: rather than define it here locally in ClangdLSPServer?
> 
> `ClangdLSPServer` didnt feel like the appropriate place for that logic. 
> Moreover other embedders of ClangdServer could benefit from traversal logic 
> if it is defined in a lower level than ClangdLSPServer.
(Sorry, I guess D88413 was really the place for this discussion)

> ClangdLSPServer didnt feel like the appropriate place for that logic.

To me, putting this in ClangdLSPServer feels like "this isn't part of the 
central mission here, it could be split out for coherence/reuse".
Whereas putting it in support/Trace feels like "this is a layering violation".

> other embedders of ClangdServer could benefit from traversal logic

If exporting memorytree->metric with the path as the label is something we want 
to reuse, that could be a function in MemoryTree.h (taking the metric as 
parameter) or we can include the generic traversal function you proposed 
earlier. Even though they're both in Support, layering MemoryTree above Tracer 
seems more appropriate than the other way around.
My instinct is this is premature generalization and having a traversal in each 
embedder is fine, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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


[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

2020-10-09 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

In D88414#2319161 , @kadircet wrote:

> In D88414#2317106 , @sammccall wrote:
>
>> Now there's lots of usage (which looks good!) i'm finding it a bit hard to 
>> keep track of what the tree will look like overall.
>>
>> At some point it'd be great to:
>>  a) bind this to an LSP extension so we can see it in editors
>
> i was also thinking about it and couldn't decide between a "custom command" 
> vs "code action".
>
> - the former gives a richer interaction, but requires every editor plugin to 
> implement support.
> - the latter is a little bit more restrictive but doesn't require a bunch of 
> extra work.
>
> I am happy to go with the "code action" approach initially. WDYT? (not in the 
> scope of this patch)

I'm pretty leery about code action because it's not at all context-sensitive 
(not even per-file).
That also doesn't give us any way to specify detail vs summary if we want that.
I'd suggest a custom LSP method (unless there's some reason to prefer 
`executeCommand`? Seems like additional indirection for no benefit).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88414

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


[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG69daa368cad3: [clangd] Disambiguate overloads of std::move 
for header insertion. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1280,10 +1280,27 @@
   Language.CPlusPlus = true;
   Includes.addSystemHeadersMapping(Language);
   CollectorOpts.Includes = 
-  runSymbolCollector("namespace std { class string {}; }", /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  Contains(AllOf(QName("std::string"), DeclURI(TestHeaderURI),
- IncludeHeader("";
+  runSymbolCollector(
+  R"cpp(
+  namespace std {
+class string {};
+// Move overloads have special handling.
+template  T&& move(T&&);
+template  O move(I, I, O);
+  }
+  )cpp",
+  /*Main=*/"");
+  for (const auto  : Symbols)
+llvm::errs() << S.Scope << S.Name << " in " << S.IncludeHeaders.size()
+ << "\n";
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  QName("std"),
+  AllOf(QName("std::string"), DeclURI(TestHeaderURI),
+IncludeHeader("")),
+  AllOf(Labeled("move(T &&)"), IncludeHeader("")),
+  AllOf(Labeled("move(I, I, O)"), IncludeHeader("";
 }
 
 TEST_F(SymbolCollectorTest, IWYUPragma) {
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -36,9 +36,9 @@
   // Usual standard library symbols are mapped correctly.
   EXPECT_EQ("", CI.mapHeader("path/vector.h", "std::vector"));
   EXPECT_EQ("", CI.mapHeader("path/stdio.h", "std::printf"));
-  // std::move is ambiguous, currently mapped only based on path
-  EXPECT_EQ("", CI.mapHeader("libstdc++/bits/move.h", "std::move"));
-  EXPECT_EQ("path/utility.h", CI.mapHeader("path/utility.h", "std::move"));
+  // std::move is ambiguous, currently always mapped to 
+  EXPECT_EQ("",
+CI.mapHeader("libstdc++/bits/stl_algo.h", "std::move"));
   // Unknown std symbols aren't mapped.
   EXPECT_EQ("foo/bar.h", CI.mapHeader("foo/bar.h", "std::notathing"));
   // iosfwd declares some symbols it doesn't own.
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -131,7 +131,7 @@
   void processRelations(const NamedDecl , const SymbolID ,
 ArrayRef Relations);
 
-  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  llvm::Optional getIncludeHeader(const Symbol , FileID);
   bool isSelfContainedHeader(FileID);
   // Heuristically headers that only want to be included via an umbrella.
   static bool isDontIncludeMeHeader(llvm::StringRef);
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -557,11 +557,9 @@
   llvm::SmallString<256> QName;
   for (const auto  : IncludeFiles)
 if (const Symbol *S = Symbols.find(Entry.first)) {
-  QName = S->Scope;
-  QName.append(S->Name);
-  if (auto Header = getIncludeHeader(QName, Entry.second)) {
+  if (auto Header = getIncludeHeader(*S, Entry.second)) {
 Symbol NewSym = *S;
-NewSym.IncludeHeaders.push_back({*Header, 1});
+NewSym.IncludeHeaders.push_back({std::move(*Header), 1});
 Symbols.insert(NewSym);
   }
 }
@@ -736,8 +734,8 @@
 /// Gets a canonical include (URI of the header or  or "header") for
 /// header of \p FID (which should usually be the *expansion* file).
 /// Returns None if includes should not be inserted for this file.
-llvm::Optional
-SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) {
+llvm::Optional SymbolCollector::getIncludeHeader(const Symbol ,
+  FileID FID) {
   const SourceManager  = ASTCtx->getSourceManager();
   const FileEntry 

[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall marked an inline comment as done.
sammccall added a comment.

In D5#2314596 , @kadircet wrote:

> Thanks, LGTM! As you mentioned I believe `std::move` is common enough to 
> deserve the special casing.

Common enough and also literally the only case AFAIK (hopefully the committee 
is friendly enough not to add more in future).
That combination pushes me towards preferring this hack at least for now...




Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:563
   if (auto Header = getIncludeHeader(QName, Entry.second)) {
+// Hack: there are two std::move() overloads from different headers.
+// CanonicalIncludes returns the common one-arg one from .

kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > i think this should live inside `CanonicalIncludes`. What about changing 
> > > `mapHeader` to take in an `llvm::Optional NumParams` or 
> > > `llvm::Optional Signature` ?
> > > 
> > > That way we can actually get rid of the ambiguity caused by all of the 
> > > overloads. I am not sure if number of parameters is always going to be 
> > > enough tho so `Signature` might be a safer bet with some canonicalization 
> > > so that we can match the version in cppreference.
> > > 
> > > (I would prefer the solution above, but I am also fine with moving this 
> > > into `SymbolCollector::getIncludeHeader` with a FIXME.)
> > That's kind of the point of this patch, I think this one special case isn't 
> > worth making that library more complicated.
> > 
> > Would also be happy with just always suggesting , or never 
> > suggesting anything, though.
> Makes sense, I was suggesting `SymbolCollector::getIncludeHeader` rather than 
> `SymbolCollector::finish` to keep the logic in here less complicated. But I 
> am fine if you don't want to make changes to the singature. (It is 
> unfortunate that `getIncludeHeader` is a private member instead of a free 
> helper in here anyways.)
Oh right, of course. Done.

Yeah it's unfortunate, it's because isSelfContainedHeader is an expensive check 
that we have to cache, so there's a bunch of state it needs access to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

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


[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 296734.
sammccall marked an inline comment as done.
sammccall added a comment.

Refactor the hack into a more appropriate place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1280,10 +1280,27 @@
   Language.CPlusPlus = true;
   Includes.addSystemHeadersMapping(Language);
   CollectorOpts.Includes = 
-  runSymbolCollector("namespace std { class string {}; }", /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  Contains(AllOf(QName("std::string"), DeclURI(TestHeaderURI),
- IncludeHeader("";
+  runSymbolCollector(
+  R"cpp(
+  namespace std {
+class string {};
+// Move overloads have special handling.
+template  T&& move(T&&);
+template  O move(I, I, O);
+  }
+  )cpp",
+  /*Main=*/"");
+  for (const auto  : Symbols)
+llvm::errs() << S.Scope << S.Name << " in " << S.IncludeHeaders.size()
+ << "\n";
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  QName("std"),
+  AllOf(QName("std::string"), DeclURI(TestHeaderURI),
+IncludeHeader("")),
+  AllOf(Labeled("move(T &&)"), IncludeHeader("")),
+  AllOf(Labeled("move(I, I, O)"), IncludeHeader("";
 }
 
 TEST_F(SymbolCollectorTest, IWYUPragma) {
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -36,9 +36,9 @@
   // Usual standard library symbols are mapped correctly.
   EXPECT_EQ("", CI.mapHeader("path/vector.h", "std::vector"));
   EXPECT_EQ("", CI.mapHeader("path/stdio.h", "std::printf"));
-  // std::move is ambiguous, currently mapped only based on path
-  EXPECT_EQ("", CI.mapHeader("libstdc++/bits/move.h", "std::move"));
-  EXPECT_EQ("path/utility.h", CI.mapHeader("path/utility.h", "std::move"));
+  // std::move is ambiguous, currently always mapped to 
+  EXPECT_EQ("",
+CI.mapHeader("libstdc++/bits/stl_algo.h", "std::move"));
   // Unknown std symbols aren't mapped.
   EXPECT_EQ("foo/bar.h", CI.mapHeader("foo/bar.h", "std::notathing"));
   // iosfwd declares some symbols it doesn't own.
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -131,7 +131,7 @@
   void processRelations(const NamedDecl , const SymbolID ,
 ArrayRef Relations);
 
-  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  llvm::Optional getIncludeHeader(const Symbol , FileID);
   bool isSelfContainedHeader(FileID);
   // Heuristically headers that only want to be included via an umbrella.
   static bool isDontIncludeMeHeader(llvm::StringRef);
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -557,11 +557,9 @@
   llvm::SmallString<256> QName;
   for (const auto  : IncludeFiles)
 if (const Symbol *S = Symbols.find(Entry.first)) {
-  QName = S->Scope;
-  QName.append(S->Name);
-  if (auto Header = getIncludeHeader(QName, Entry.second)) {
+  if (auto Header = getIncludeHeader(*S, Entry.second)) {
 Symbol NewSym = *S;
-NewSym.IncludeHeaders.push_back({*Header, 1});
+NewSym.IncludeHeaders.push_back({std::move(*Header), 1});
 Symbols.insert(NewSym);
   }
 }
@@ -736,8 +734,8 @@
 /// Gets a canonical include (URI of the header or  or "header") for
 /// header of \p FID (which should usually be the *expansion* file).
 /// Returns None if includes should not be inserted for this file.
-llvm::Optional
-SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) {
+llvm::Optional SymbolCollector::getIncludeHeader(const Symbol ,
+  FileID FID) {
   const SourceManager  = ASTCtx->getSourceManager();
   const FileEntry *FE = SM.getFileEntryForID(FID);
   if (!FE || FE->getName().empty())
@@ -746,10 +744,18 

[PATCH] D88814: [clangd] Enable partial namespace matches for workspace symbols

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:44
 
+// Returns true if \p Query can be found as a sub-scope inside \p Scope.
+bool approximateScopeMatch(llvm::StringRef Scope,

kadircet wrote:
> sammccall wrote:
> > I had a little trouble following this...
> > It seems a little simpler (fewer vars to track) if we avoid the up-front 
> > split on scopes.
> > 
> > ```
> > assert(Scope.empty() || Scope.endswith("::")); // or handle in some way
> > // Walk through Scope, consuming matching tokens from Query.
> > StringRef First;
> > while (!Scope.empty() && !Query.empty()) {
> >   tie(First, Scope) = Scope.split("::");
> >   if (Query.front() == First)
> > Query = Query.drop_front();
> > }
> > return Query.empty(); // all components of query matched
> > ```
> > 
> > in fact we can avoid preprocessing query too:
> > 
> > ```
> > // Query is just a StringRef
> > assert(Scope.empty() || Scope.endswith("::")); // or handle in some way
> > assert(Query.empty() || Query.endswith("::"));
> > 
> > // Walk through Scope, consuming matching tokens from Query.
> > StringRef First;
> > while (!Scope.empty() && !Query.empty()) {
> >   tie(First, Scope) = Scope.split("::");
> >   Query.consume_front(StringRef(First.data(), First.size() + 2) /*Including 
> > ::*/);
> > }
> > return Query.empty(); // all components of query matched
> > ```
> Yes but they would do different things. I believe the confusion is caused by 
> usage of `sub-scope` without a clear definition.  The codes you've suggested 
> are performing sub-sequence matches rather than sub-string(i.e. we are 
> looking for a contigious segment in `Scope` that matches `Query`).
> 
> I believe a query of the form `a::c::` shouldn't be matched by `a::b::c::`. I 
> can try simplifying the logic, but it would be nice to agree on the behaviour 
> :D.
> 
> Sorry if I miscommunicated this so far.
Ah right, I was indeed misreading the code. Let's have some definitions...

given query `a::b::Foo` with scope `a::b::`

| | a::b:: | `W::a::b::` | `a::X::b::` | `a::b::Y` |
| exact | * | | | |
| prefix |*| | | * |
| suffix | *|* | | |
| substring | * | * | | * |
| subsequence | * | * | * | * |

These support correcting different types of "errors":
 - exact: none
 - prefix: may omit namespaces immediately before Foo
 - suffix: query may be rooted anywhere (other than global ns)
 - substring: query rooted anywhere, omit namespaces before Foo
 - subsequence: may omit any component

We know "exact" is too strict.

I think "prefix" and by extension "substring" aren't particularly compelling 
rules as the "immediately before Foo" requirement is arbitrary.
Why does `a::b::Foo` match `a::b::c::Foo` and not `a::c::b::Foo`? In each case 
we've omitted a namespace inside the query, the only difference is what it's 
sandwiched between.

Suffix makes intuitive sense, it accepts strings that make sense *somewhere* in 
the codebase.
Subsequence makes intuitive sense too: you're allowed to forget uninteresting 
components, similar to how fuzzy-match lets you omit uninteresting words.

I'd prefer one of those and don't really mind which - I'd assumed subsequence 
was intended. Suffix is way easier to implement though :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88814

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


[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

this is at least going to take some important locks, hopefully only briefly.

We should watch the timing here carefully and consider guarding it - apart from 
the minimum time interval we discussed, we could have a check whether metric 
tracing is actually enabled in a meaningful way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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


[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep 
track of what the tree will look like overall.

At some point it'd be great to:
 a) bind this to an LSP extension so we can see it in editors
 b) add a lit test that calls that extension method and includes a dump of the 
output


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88414

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


[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

(Sorry, I suspect we discussed this and I forgot)
Is there a reason at this point to put knowledge of the core metric in trace:: 
rather than define it here locally in ClangdLSPServer?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:168
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}

this change is a bit puzzling - makes it look like there are some cases where 
we specifically want/don't want to record. why?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:171
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (auto Handler = Notifications.lookup(Method))

on the flip side processing cancellations as fast as possible seems like it 
might be important.

Maybe just move the recording of memory usage to the happy case? (Notification 
that we have a handler for, after the handler).



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:177
+
+// Record memory usage after each memory usage. This currently takes <1ms,
+// so it is safe to do frequently.

after each notification?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:179
+// so it is safe to do frequently.
+trace::Span Tracer("RecordMemoryUsage");
+MemoryTree MT;

maybe move this into a tiny function? It's self-contained, a bit distracting 
from the fairly important core logic here, and we may well want to do it 
conditionally or in more/different places in future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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


[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.

Still LGTM, commit it already :-D




Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:26
+
+size_t MemoryTree::self() const { return Size; }
+} // namespace clangd

nit: could inline this consistent with addUsage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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


[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.

LGTM, thanks!




Comment at: clang-tools-extra/clangd/support/MemoryTree.h:57
+  /// Returns total number of bytes used by this sub-tree. Performs a 
traversal.
+  size_t total() const;
+

oops, we should probably also expose self?



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return (Name); }
+

kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > actually, why do these return pointers rather than references?
> > reading call sites, `child()` might be both more fluent and more accurate 
> > than `addChild` - we're not calling it for the side effect and there may or 
> > may not be one.
> > actually, why do these return pointers rather than references?
> 
> It is to make usage with `auto` safer, as it won't capture ref by default and 
> make a copy. e.g. `auto child = MT.child(); child.addusage(1)` won't work as 
> expected.
> 
> I suppose we can make the object non-copyable and prevent such misuse.
Oh, right - yes ref but noncopyable seems even better to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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


[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:563
   if (auto Header = getIncludeHeader(QName, Entry.second)) {
+// Hack: there are two std::move() overloads from different headers.
+// CanonicalIncludes returns the common one-arg one from .

kadircet wrote:
> i think this should live inside `CanonicalIncludes`. What about changing 
> `mapHeader` to take in an `llvm::Optional NumParams` or 
> `llvm::Optional Signature` ?
> 
> That way we can actually get rid of the ambiguity caused by all of the 
> overloads. I am not sure if number of parameters is always going to be enough 
> tho so `Signature` might be a safer bet with some canonicalization so that we 
> can match the version in cppreference.
> 
> (I would prefer the solution above, but I am also fine with moving this into 
> `SymbolCollector::getIncludeHeader` with a FIXME.)
That's kind of the point of this patch, I think this one special case isn't 
worth making that library more complicated.

Would also be happy with just always suggesting , or never suggesting 
anything, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

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


[PATCH] D88875: [clangd] Add basic keyword-name-validation in rename.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:416
+auto Results = clangd::rename(
+{Pos, /*NewName=*/"__clangd_rename_dummy", InpAST->AST, File,
+ RenameOpts.AllowCrossFile ? nullptr : Index, RenameOpts});

oh, reserved name is clever :-)

Though it occurs to me - we could actually warn on rename to a reserved name!
(We can still do this and just ignore __clangd_*)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88875

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


[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Up until now, we relied on matching the filename.
This depends on unstable details of libstdc++ and doesn't work well on other
stdlibs. Also we'd like to remove it (see D88204 
).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D5

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1280,10 +1280,27 @@
   Language.CPlusPlus = true;
   Includes.addSystemHeadersMapping(Language);
   CollectorOpts.Includes = 
-  runSymbolCollector("namespace std { class string {}; }", /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  Contains(AllOf(QName("std::string"), DeclURI(TestHeaderURI),
- IncludeHeader("";
+  runSymbolCollector(
+  R"cpp(
+  namespace std {
+class string {};
+// Move overloads have special handling.
+template  T&& move(T&&);
+template  O move(I, I, O);
+  }
+  )cpp",
+  /*Main=*/"");
+  for (const auto  : Symbols)
+llvm::errs() << S.Scope << S.Name << " in " << S.IncludeHeaders.size()
+ << "\n";
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  QName("std"),
+  AllOf(QName("std::string"), DeclURI(TestHeaderURI),
+IncludeHeader("")),
+  AllOf(Labeled("move(T &&)"), IncludeHeader("")),
+  AllOf(Labeled("move(I, I, O)"), IncludeHeader("";
 }
 
 TEST_F(SymbolCollectorTest, IWYUPragma) {
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -36,9 +36,9 @@
   // Usual standard library symbols are mapped correctly.
   EXPECT_EQ("", CI.mapHeader("path/vector.h", "std::vector"));
   EXPECT_EQ("", CI.mapHeader("path/stdio.h", "std::printf"));
-  // std::move is ambiguous, currently mapped only based on path
-  EXPECT_EQ("", CI.mapHeader("libstdc++/bits/move.h", "std::move"));
-  EXPECT_EQ("path/utility.h", CI.mapHeader("path/utility.h", "std::move"));
+  // std::move is ambiguous, currently always mapped to 
+  EXPECT_EQ("",
+CI.mapHeader("libstdc++/bits/stl_algo.h", "std::move"));
   // Unknown std symbols aren't mapped.
   EXPECT_EQ("foo/bar.h", CI.mapHeader("foo/bar.h", "std::notathing"));
   // iosfwd declares some symbols it doesn't own.
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -560,6 +560,11 @@
   QName = S->Scope;
   QName.append(S->Name);
   if (auto Header = getIncludeHeader(QName, Entry.second)) {
+// Hack: there are two std::move() overloads from different headers.
+// CanonicalIncludes returns the common one-arg one from .
+if (S->Name == "move" && S->Scope == "std::" &&
+S->Signature.contains(','))
+  *Header = "";
 Symbol NewSym = *S;
 NewSym.IncludeHeaders.push_back({*Header, 1});
 Symbols.insert(NewSym);
Index: clang-tools-extra/clangd/index/CanonicalIncludes.cpp
===
--- clang-tools-extra/clangd/index/CanonicalIncludes.cpp
+++ clang-tools-extra/clangd/index/CanonicalIncludes.cpp
@@ -90,6 +90,8 @@
 static const auto *Symbols = new llvm::StringMap({
 #define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name, #Header},
 #include "StdSymbolMap.inc"
+// There are two std::move()s, this is by far the most common.
+SYMBOL(move, std::, )
 #undef SYMBOL
 });
 StdSymbolMapping = Symbols;


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1280,10 +1280,27 @@
   Language.CPlusPlus = true;
   Includes.addSystemHeadersMapping(Language);
   CollectorOpts.Includes = 
-  runSymbolCollector("namespace std { class string {}; }", /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  

[PATCH] D88814: [clangd] Enable partial namespace matches for workspace symbols

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:44
 
+// Returns true if \p Query can be found as a sub-scope inside \p Scope.
+bool approximateScopeMatch(llvm::StringRef Scope,

I had a little trouble following this...
It seems a little simpler (fewer vars to track) if we avoid the up-front split 
on scopes.

```
assert(Scope.empty() || Scope.endswith("::")); // or handle in some way
// Walk through Scope, consuming matching tokens from Query.
StringRef First;
while (!Scope.empty() && !Query.empty()) {
  tie(First, Scope) = Scope.split("::");
  if (Query.front() == First)
Query = Query.drop_front();
}
return Query.empty(); // all components of query matched
```

in fact we can avoid preprocessing query too:

```
// Query is just a StringRef
assert(Scope.empty() || Scope.endswith("::")); // or handle in some way
assert(Query.empty() || Query.endswith("::"));

// Walk through Scope, consuming matching tokens from Query.
StringRef First;
while (!Scope.empty() && !Query.empty()) {
  tie(First, Scope) = Scope.split("::");
  Query.consume_front(StringRef(First.data(), First.size() + 2) /*Including 
::*/);
}
return Query.empty(); // all components of query matched
```



Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:262
 
+TEST(WorkspaceSymbols, RankingPartilNamespace) {
+  TestTU TU;

Partil -> Partial


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88814

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


[PATCH] D88844: [clangd] Add `score` extension to workspace/symbol response.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3cb1220709fa: [clangd] Add `score` extension to 
workspace/symbol response. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D88844?vs=296263=296400#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88844

Files:
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/symbols.test

Index: clang-tools-extra/clangd/test/symbols.test
===
--- clang-tools-extra/clangd/test/symbols.test
+++ clang-tools-extra/clangd/test/symbols.test
@@ -23,7 +23,8 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "uri": "file://{{.*}}/vector.h"
 # CHECK-NEXT:},
-# CHECK-NEXT:"name": "vector"
+# CHECK-NEXT:"name": "vector",
+# CHECK-NEXT:"score": {{.*}}
 # CHECK-NEXT:  }
 # CHECK-NEXT:]
 # CHECK-NEXT:}
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -1015,6 +1015,14 @@
 
   /// The name of the symbol containing this symbol.
   std::string containerName;
+
+  /// The score that clangd calculates to rank the returned symbols.
+  /// This excludes the fuzzy-matching score between `name` and the query.
+  /// (Specifically, the last ::-separated component).
+  /// This can be used to re-rank results as the user types, using client-side
+  /// fuzzy-matching (that score should be multiplied with this one).
+  /// This is a clangd extension, set only for workspace/symbol responses.
+  llvm::Optional score;
 };
 llvm::json::Value toJSON(const SymbolInformation &);
 llvm::raw_ostream <<(llvm::raw_ostream &, const SymbolInformation &);
@@ -1175,11 +1183,11 @@
   /// Indicates if this item is deprecated.
   bool deprecated = false;
 
-  /// This is Clangd extension.
-  /// The score that Clangd calculates to rank completion items. This score can
-  /// be used to adjust the ranking on the client side.
-  /// NOTE: This excludes fuzzy matching score which is typically calculated on
-  /// the client side.
+  /// The score that clangd calculates to rank the returned completions.
+  /// This excludes the fuzzy-match between `filterText` and the partial word.
+  /// This can be used to re-rank results as the user types, using client-side
+  /// fuzzy-matching (that score should be multiplied with this one).
+  /// This is a clangd extension.
   float score = 0.f;
 
   // TODO: Add custom commitCharacters for some of the completion items. For
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -662,12 +662,15 @@
 }
 
 llvm::json::Value toJSON(const SymbolInformation ) {
-  return llvm::json::Object{
+  llvm::json::Object O{
   {"name", P.name},
   {"kind", static_cast(P.kind)},
   {"location", P.location},
   {"containerName", P.containerName},
   };
+  if (P.score)
+O["score"] = *P.score;
+  return std::move(O);
 }
 
 llvm::raw_ostream <<(llvm::raw_ostream ,
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -96,12 +96,13 @@
   return;
 }
 
-SymbolKind SK = indexSymbolKindToSymbolKind(Sym.SymInfo.Kind);
-std::string Scope = std::string(Sym.Scope);
-llvm::StringRef ScopeRef = Scope;
-ScopeRef.consume_back("::");
-SymbolInformation Info = {(Sym.Name + Sym.TemplateSpecializationArgs).str(),
-  SK, *Loc, std::string(ScopeRef)};
+llvm::StringRef Scope = Sym.Scope;
+Scope.consume_back("::");
+SymbolInformation Info;
+Info.name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
+Info.kind = indexSymbolKindToSymbolKind(Sym.SymInfo.Kind);
+Info.location = *Loc;
+Info.containerName = Scope.str();
 
 SymbolQualitySignals Quality;
 Quality.merge(Sym);
@@ -121,6 +122,8 @@
 dlog("FindSymbols: {0}{1} = {2}\n{3}{4}\n", Sym.Scope, Sym.Name, Score,
  Quality, Relevance);
 
+// Exposed score excludes fuzzy-match component, for client-side re-ranking.
+Info.score = Score / Relevance.NameMatch;
 Top.push({Score, std::move(Info)});
   });
   for (auto  : std::move(Top).items())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D88415: [clangd] Introduce memory usage dumping to TUScheduler, for Preambles and ASTCache

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1445
+void TUScheduler::attachMemoryUsage(MemoryTree ) const {
+  IdleASTs->attachMemoryUsage(*MT.addChild("ast_cahe"));
+  // Otherwise preambles are stored on disk and we only keep filename in 
memory.

should we group by file instead? The fact that the AST-cache is owned by 
TUScheduler and not ASTWorker seems like a confusing detail that it might be 
better to omit



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1450
+for (const auto  : fileStats())
+  Preambles.addDetail(Elem.first())
+  ->addUsage(Elem.second.UsedBytesPreamble);

why aren't we also using the UsedBytesAST estimate here, and instead walking 
through the cache directly?



Comment at: clang-tools-extra/clangd/TUScheduler.h:316
 
+  void attachMemoryUsage(MemoryTree ) const;
+

(raised naming of these functions in another patch, however that ends up let's 
ensure they match)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88415

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


[PATCH] D88875: [clangd] Add basic keyword-name-validation in rename.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:124
+
+  // name validation.
+  RenameToKeywords,

nit: not sure we need a separate section for these, I can imagine at most 
keyword/conflict/shadow



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:215
+case ReasonToReject::RenameToKeywords:
+  return "symbol is renamed to a reserved keyword";
 }

nit: either "reserved [word|identifier|name]" or "keyword", but not both

This could be friendlier, what about "the chosen name is a keyword"?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:479
 return makeError(ReasonToReject::AmbiguousSymbol);
+  // Empty name is legal -- this allows rename API to return all occurrences
+  // even the name is unknown.

Now that prepareRename only exposes ranges rather than edits, do we still need 
this empty special case, or can we go back to using "clangd_rename_dummy" or so?

(Sorry for the back and forth here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88875

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


[PATCH] D88881: [clangd] Add a NewName optional parameter to clangdServer::prepareRename.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Can you add a bit more context to the commit message?

And should we expose this as an extension on `textDocument/prepareRename`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D1

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


[PATCH] D88881: [clangd] Add a NewName optional parameter to clangdServer::prepareRename.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:797
   Server->prepareRename(
-  Params.textDocument.uri.file(), Params.position, Opts.Rename,
+  Params.textDocument.uri.file(), Params.position, /*NewName*/ "",
+  Opts.Rename,

please make this optional rather than using "" as a sentinel value. The empty 
string is a plausible value here (though of course will always fail)



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:404
  const RenameOptions ,
  Callback CB) {
+  auto Action = [Pos, File = File.str(), CB = std::move(CB),

we can fail already here if the name is specified and empty


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D1

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


[PATCH] D84322: [AST][RecoveryExpr] Part3: Suppress spurious "typecheck_cond_expect_scalar" diagnostic

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/AST/ast-dump-recovery.c:75
+
+  // conditional operator
+  float f;

add to comment: (comparison is invalid)



Comment at: clang/test/AST/ast-dump-recovery.c:83
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
+  ptr > f ? ptr : f;
 }

again - parens



Comment at: clang/test/Sema/error-dependence.c:18
+  // type is required" is not emitted.
+  ptr > f ? ptr : f; // expected-error {{invalid operands to binary 
expression}}
+}

hokein wrote:
> sammccall wrote:
> > nit: parens would help me understand here :-)
> > 
> > I don't find this example compelling because we should know that "ptr > f" 
> > is a boolean.
> > 
> > Can we just have `undefined ? ptr : f`, expecting the only diag to be the 
> > undeclared ident?
> > I don't find this example compelling because we should know that "ptr > f" 
> > is a boolean.
> 
> this is a reasonable impression, but in this case, there is no 
> binary-operator `>` -- operands `ptr`, `f` have different types, and invalid 
> for binary operator, instead we build a recovery-expr.
> 
> so the AST looks like (added in the dump-recovery.c test as well)
> 
> ```
>   ConditionalOperator> '' contains-errors
> | |-RecoveryExpr  '' contains-errors lvalue
> | | |-DeclRefExpr  'int *' lvalue Var 0x8fdb620 'ptr' 'int *'
> | | `-DeclRefExpr  'float' lvalue Var 0x8ffd388 'f' 'float'
> | |-DeclRefExpr  'int *' lvalue Var 0x8fdb620 'ptr' 'int *'
> | `-DeclRefExpr  'float' lvalue Var 0x8ffd388 'f' 'float'
> ``` 
> 
> > Can we just have undefined ? ptr : f, expecting the only diag to be the 
> > undeclared ident?
> 
> no unfortunately. the whole statement is being dropped (we don't preserve 
> this in C/C++). 
`undefined() ? ptr : f` then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84322

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


[PATCH] D86959: [clang-format] Fix formatting of _Atomic() qualifier

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Sorry to be really late here, but this patch regressed some macro definitions 
like:

  #define lambda [](const decltype(x) ) {}

which now formats as

  #define lambda [](const decltype(x) & ptr) {}

(extra space around ampersand)

It looks like the generalization of decltype into a tokentype on the parens 
isn't catching all the cases it used to.
This isn't really my area - any idea why this might be?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86959

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


[PATCH] D88472: [clangd] Don't set the Underlying bit on targets of UsingDecls.

2020-10-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, thanks for all the iterations here.

I do want to solve the regression but we can land without that.




Comment at: clang-tools-extra/clangd/FindTarget.cpp:345
 } else if (const UsingDecl *UD = dyn_cast(D)) {
   for (const UsingShadowDecl *S : UD->shadows())
+add(S->getUnderlyingDecl(), Flags);

maybe a comment here "// no Underlying as this is a non-renaming alias"



Comment at: clang-tools-extra/clangd/FindTarget.cpp:357
ValueFilter)) {
-add(Target, Flags | Rel::Underlying);
+add(Target, Flags);
   }

and here



Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1121
   namespace ns { class [[Foo]] {}; }
-  using ns::F^oo;
+  using ns::[[F^oo]];
 )cpp",

I'd like to make a plan to solve this regression (we can do it here or in a 
followup patch, happy to hack on that).

What do you think about the following heuristic:
 - if go-to-definition yields multiple results
 - and a particular result touches the cursor*
 - (optional) and that result is marked with Alias
 - then drop it from the set
It means we still keep some special-case code, but it's not complex and fairly 
intuitive I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88472

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


[PATCH] D88844: [clangd] Add `score` extension to workspace/symbol response.

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

The protocol doesn't really incorporate ranking.
As with code completion, most clients respect what the server sends, but
VSCode re-ranks items, with predictable results.
See https://github.com/clangd/vscode-clangd/issues/81

There's no filterText field so we may be unable to construct a good workaround.
But expose the score so we may be able to do this on the client in future.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88844

Files:
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/symbols.test

Index: clang-tools-extra/clangd/test/symbols.test
===
--- clang-tools-extra/clangd/test/symbols.test
+++ clang-tools-extra/clangd/test/symbols.test
@@ -23,7 +23,8 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "uri": "file://{{.*}}/vector.h"
 # CHECK-NEXT:},
-# CHECK-NEXT:"name": "vector"
+# CHECK-NEXT:"name": "vector",
+# CHECK-NEXT:"score": {{.*}}
 # CHECK-NEXT:  }
 # CHECK-NEXT:]
 # CHECK-NEXT:}
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -1015,6 +1015,14 @@
 
   /// The name of the symbol containing this symbol.
   std::string containerName;
+
+  /// The score that clangd calculates to rank the returned symbols.
+  /// This excludes the fuzzy-matching score between `name` and the query.
+  /// (Specifically, the last ::-separated component).
+  /// This can be used to re-rank results as the user types, using client-side
+  /// fuzzy-matching (that score should be multiplied with this one).
+  /// This is a clangd extension, set only for workspace/symbol responses.
+  llvm::Optional score;
 };
 llvm::json::Value toJSON(const SymbolInformation &);
 llvm::raw_ostream <<(llvm::raw_ostream &, const SymbolInformation &);
@@ -1175,11 +1183,11 @@
   /// Indicates if this item is deprecated.
   bool deprecated = false;
 
-  /// This is Clangd extension.
-  /// The score that Clangd calculates to rank completion items. This score can
-  /// be used to adjust the ranking on the client side.
-  /// NOTE: This excludes fuzzy matching score which is typically calculated on
-  /// the client side.
+  /// The score that clangd calculates to rank the returned completions.
+  /// This excludes the fuzzy-match between `filterText` and the partial word.
+  /// This can be used to re-rank results as the user types, using client-side
+  /// fuzzy-matching (that score should be multiplied with this one).
+  /// This is a clangd extension.
   float score = 0.f;
 
   // TODO: Add custom commitCharacters for some of the completion items. For
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -662,12 +662,15 @@
 }
 
 llvm::json::Value toJSON(const SymbolInformation ) {
-  return llvm::json::Object{
+  llvm::json::Object O{
   {"name", P.name},
   {"kind", static_cast(P.kind)},
   {"location", P.location},
   {"containerName", P.containerName},
   };
+  if (P.score)
+O["score"] = *P.score;
+  return std::move(O);
 }
 
 llvm::raw_ostream <<(llvm::raw_ostream ,
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -96,12 +96,13 @@
   return;
 }
 
-SymbolKind SK = indexSymbolKindToSymbolKind(Sym.SymInfo.Kind);
-std::string Scope = std::string(Sym.Scope);
-llvm::StringRef ScopeRef = Scope;
-ScopeRef.consume_back("::");
-SymbolInformation Info = {(Sym.Name + Sym.TemplateSpecializationArgs).str(),
-  SK, *Loc, std::string(ScopeRef)};
+llvm::StringRef Scope = Sym.Scope;
+Scope.consume_back("::");
+SymbolInformation Info;
+Info.name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
+Info.kind = indexSymbolKindToSymbolKind(Sym.SymInfo.Kind);
+Info.location = *Loc;
+Info.containerName = Scope.str();
 
 SymbolQualitySignals Quality;
 Quality.merge(Sym);
@@ -121,6 +122,7 @@
 dlog("FindSymbols: {0}{1} = {2}\n{3}{4}\n", Sym.Scope, Sym.Name, Score,
  Quality, Relevance);
 
+Info.score = Score / Relevance.NameMatch;
 Top.push({Score, std::move(Info)});
   });
   for (auto  : std::move(Top).items())
___
cfe-commits mailing list

[PATCH] D88822: [clangd] Describe non-handling of most IWYU pragmas. NFC

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG95262ee2be75: [clangd] Describe non-handling of most IWYU 
pragmas. NFC (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88822

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.h


Index: clang-tools-extra/clangd/index/CanonicalIncludes.h
===
--- clang-tools-extra/clangd/index/CanonicalIncludes.h
+++ clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -71,6 +71,21 @@
 ///
 /// Currently it only supports IWYU private pragma:
 /// 
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-private
+///
+/// We ignore other pragmas:
+/// - keep: this is common but irrelevant: we do not currently remove includes
+/// - export: this is common and potentially interesting, there are three 
cases:
+///* Points to a public header (common): we can suppress include2 if you
+///  already have include1. Only marginally useful.
+///* Points to a private header annotated with `private` (somewhat 
commmon):
+///  Not incrementally useful as we support private.
+///* Points to a private header without pragmas (rare). This is a reversed
+///  private pragma, and is valuable but too rare to be worthwhile.
+/// - no_include: this is about as common as private, but only affects the
+///   current file, so the value is smaller. We could add support.
+/// - friend: this is less common than private, has implementation 
difficulties,
+///   and affects behavior in a limited scope.
+/// - associated: extremely rare
 std::unique_ptr
 collectIWYUHeaderMaps(CanonicalIncludes *Includes);
 


Index: clang-tools-extra/clangd/index/CanonicalIncludes.h
===
--- clang-tools-extra/clangd/index/CanonicalIncludes.h
+++ clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -71,6 +71,21 @@
 ///
 /// Currently it only supports IWYU private pragma:
 /// https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-private
+///
+/// We ignore other pragmas:
+/// - keep: this is common but irrelevant: we do not currently remove includes
+/// - export: this is common and potentially interesting, there are three cases:
+///* Points to a public header (common): we can suppress include2 if you
+///  already have include1. Only marginally useful.
+///* Points to a private header annotated with `private` (somewhat commmon):
+///  Not incrementally useful as we support private.
+///* Points to a private header without pragmas (rare). This is a reversed
+///  private pragma, and is valuable but too rare to be worthwhile.
+/// - no_include: this is about as common as private, but only affects the
+///   current file, so the value is smaller. We could add support.
+/// - friend: this is less common than private, has implementation difficulties,
+///   and affects behavior in a limited scope.
+/// - associated: extremely rare
 std::unique_ptr
 collectIWYUHeaderMaps(CanonicalIncludes *Includes);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D84226: [AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery.

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:14381
+  OpLoc, CurFPFeatureOverrides());
+switch (Opc) {
+case BO_Assign:

now the only thing that differs between the cases is the type. You could 
consider putting the type in a variable, and factoring the Create after the 
switch.



Comment at: clang/lib/Sema/SemaExpr.cpp:14393
+case BO_NE:
+
+case BO_LAnd:

(why the line break here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84226

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


[PATCH] D88814: [clangd] Enable partial namespace matches for workspace symbols

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:74
 
   auto Names = splitQualifiedName(Query);
 

Add a comment here (or elsewhere, I guess) about how qualified names are 
handled.

- exact namespace: boosted on the index side
- approximate namespace (e.g. substring): included using "all scopes" logic
- non-matching namespace (no subtring match): filtered out from index-provided 
results



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:83
+  // Boost symbols from desired namespace.
+  if (!Req.AnyScope || !Names.first.empty())
 Req.Scopes = {std::string(Names.first)};

This expression doesn't make sense (except in context of the above code). 
Variables are cheap!

```
LeadingColons = consume_front("::");
Req.AnyScope = !LeadingColons
if (LeadingColons || !Names.first.empty())
  ...
```



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:86
   if (Limit)
 Req.Limit = Limit;
   TopN Top(

Given the dynamic filter, we should request a greater multiple here (this time 
if anyscope && Names.first.empty is the right logic!)

This gives us a second class of regression :-) but we can tune the multiple to 
control it. I'd suggest 5 or so



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:93
+llvm::StringRef ScopeRef = Scope;
+// Fuzzfind might return symbols from irrelevant namespaces if query was 
not
+// fully-qualified, drop those.

nit: fuzzyfind



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:95
+// fully-qualified, drop those.
+if (!ScopeRef.contains(Names.first))
+  return;

Pull out a `approximateScopeMatch(scope, query)` or so function?

Substring isn't quite right - fine for now with a fixme, but might as well pull 
out the function now so we don't make a mess when the code grows.



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:111
 Quality.merge(Sym);
 SymbolRelevanceSignals Relevance;
 Relevance.Name = Sym.Name;

Would be nice to incorporate exact vs approximate scope match here: people 
complain a lot when an exact string match ranks below an approximate one.

I don't think ScopeDistance works as-is, because it assumes the reference point 
(query) is absolute.
You could consider NeedsFixIts or applying a multiplier to NameMatch (0.3?) or 
InBaseClass (all of these are a stretch I guess)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88814

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


[PATCH] D84226: [AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery.

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/AST/ASTContext.h:668
 
+  // clang's C-only codepath doesn't support dependent types yet, it is used to
+  // perform early typo correction for C.

This is a bit hard to follow.
I think it's two separate sentences, and the second is something like
"If this condition is false, typo correction must be performed eagerly rather 
than delayed in many places, as it makes use of dependent types".



Comment at: clang/lib/Sema/SemaExpr.cpp:14365
+   "Should only occur in error-recovery path.");
+if (BinaryOperator::isCompoundAssignmentOp(Opc))
+  return CompoundAssignOperator::Create(

isAssignmentOp instead? including = itself



Comment at: clang/lib/Sema/SemaExpr.cpp:14383
+Context.IntTy, VK_RValue, OK_Ordinary,
+OpLoc, CurFPFeatureOverrides());
+default:

comma could get the RHS :-)
May get some mileage for comma operators in macros, not sure though.

(Other aspect is value category: fortunately *in C* comma is an rvalue)



Comment at: clang/test/AST/ast-dump-recovery.c:45
   int* ptr;
-  // FIXME: the top-level expr should be a binary operator.
-  // CHECK:  ImplicitCastExpr {{.*}} contains-errors 
-  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors lvalue
-  // CHECK-NEXT:   |-DeclRefExpr {{.*}} 'ptr' 'int *'
-  // CHECK-NEXT:   `-RecoveryExpr {{.*}}
-  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'some_func'
+  // CHECK: BinaryOperator {{.*}} contains-errors '='
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'ptr' 'int *'

`'int *' lvalue contains-errors '='`



Comment at: clang/test/AST/ast-dump-recovery.c:52
+  int compoundOp;
+  // CHECK: CompoundAssignOperator {{.*}} '+='
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'compoundOp'

`'int' lvalue contains-errors '+='`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84226

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


[PATCH] D88822: [clangd] Describe non-handling of most IWYU pragmas. NFC

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88822

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.h


Index: clang-tools-extra/clangd/index/CanonicalIncludes.h
===
--- clang-tools-extra/clangd/index/CanonicalIncludes.h
+++ clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -71,6 +71,21 @@
 ///
 /// Currently it only supports IWYU private pragma:
 /// 
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-private
+///
+/// We ignore other pragmas:
+/// - keep: this is common but irrelevant: we do not currently remove includes
+/// - export: this is common and potentially interesting, there are three 
cases:
+///* Points to a public header (common): we can suppress include2 if you
+///  already have include1. Only marginally useful.
+///* Points to a private header annotated with `private` (somewhat 
commmon):
+///  Not incrementally useful as we support private.
+///* Points to a private header without pragmas (rare). This is a reversed
+///  private pragma, and is valuable but too rare to be worthwhile.
+/// - no_include: this is about as common as private, but only affects the
+///   current file, so the value is smaller. We could add support.
+/// - friend: this is less common than private, has implementation 
difficulties,
+///   and affects behavior in a limited scope.
+/// - associated: extremely rare
 std::unique_ptr
 collectIWYUHeaderMaps(CanonicalIncludes *Includes);
 


Index: clang-tools-extra/clangd/index/CanonicalIncludes.h
===
--- clang-tools-extra/clangd/index/CanonicalIncludes.h
+++ clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -71,6 +71,21 @@
 ///
 /// Currently it only supports IWYU private pragma:
 /// https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-private
+///
+/// We ignore other pragmas:
+/// - keep: this is common but irrelevant: we do not currently remove includes
+/// - export: this is common and potentially interesting, there are three cases:
+///* Points to a public header (common): we can suppress include2 if you
+///  already have include1. Only marginally useful.
+///* Points to a private header annotated with `private` (somewhat commmon):
+///  Not incrementally useful as we support private.
+///* Points to a private header without pragmas (rare). This is a reversed
+///  private pragma, and is valuable but too rare to be worthwhile.
+/// - no_include: this is about as common as private, but only affects the
+///   current file, so the value is smaller. We could add support.
+/// - friend: this is less common than private, has implementation difficulties,
+///   and affects behavior in a limited scope.
+/// - associated: extremely rare
 std::unique_ptr
 collectIWYUHeaderMaps(CanonicalIncludes *Includes);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return (Name); }
+

sammccall wrote:
> actually, why do these return pointers rather than references?
reading call sites, `child()` might be both more fluent and more accurate than 
`addChild` - we're not calling it for the side effect and there may or may not 
be one.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:49
+  /// with dots(".").
+  void traverseTree(llvm::function_ref

sammccall wrote:
> nit: need to be explicit whether this is self or subtree (or could pass both)
I think it'd be worth having `size_t total()` with a comment that it traverses 
the tree.

We have places where we only want this info (e.g. log messages) and it's 
probably nice in unit tests too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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


[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/Background.cpp:419
+  IndexedSymbols.attachMemoryUsage(*MT.addChild("symbols"));
+  MT.addChild("index")->addUsage(estimateMemoryUsage());
+}

Hmm, we should be careful now, estimateMemoryUsage() is "total" while addUsage 
takes "self".

We're not overriding estimateMemoryUsage here to include IndexedSymbols, but 
this is probably just an oversight. Consider calling 
SwapIndex::estimateMemoryUsage() explicitly to guard/communicate against this.

In the long-run, we should put attachMemoryUsage on SymbolIndex and either 
remove estimateMemoryUsage() or make it non-virtual (`MemoryTree R; 
attachMemoryUsage(R); return R.total()`)



Comment at: clang-tools-extra/clangd/index/Background.h:177
 
+  void attachMemoryUsage(MemoryTree ) const;
+

As a conventional name for these, profile() or so might be slightly more 
evocative. And I think we should push this into progressively more places (that 
currently have ad-hoc "estimate" accessors) so the brevity is reasonable I 
think.

(If not, I'd even slightly prefer "record"/"measure" over "attach" as 
emphasizing the high-level operation rather than the data structure used can 
help readability)



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:380
+  for (const auto  : SymbolsSnapshot)
+MT.addDetail(SymSlab.first())->addUsage(SymSlab.second->bytes());
+  for (const auto  : RefsSnapshot)

addChild("symbols"/"refs"/"relations")?
This seems like really useful information



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:457
+void FileIndex::attachMemoryUsage(MemoryTree ) const {
+  PreambleSymbols.attachMemoryUsage(*MT.addChild("preamble_symbols"));
+  MT.addChild("preamble_index")->addUsage(PreambleIndex.estimateMemoryUsage());

addChild("preamble").addChild("symbols")?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88414

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


[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:35
+
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return (Name); }

Maybe mention that child pointers are invalidated by future addChild/addDetail



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return (Name); }
+

actually, why do these return pointers rather than references?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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


[PATCH] D87350: [AST][RecoveryExpr] Fix a crash on undeduced type.

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.

LG, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87350

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


[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Looks good - some concerns that the traversal isn't flexible enough but we can 
address that later once it's used.




Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:10
+std::string ComponentName) const {
+  CB(Size, ComponentName);
+  if (!ComponentName.empty())

If you're only going to pass one size, I'd think total (rather than self) is 
the most meaningful.
This is easier if you do postorder of course...



Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:11
+  CB(Size, ComponentName);
+  if (!ComponentName.empty())
+ComponentName += '.';

if you pass ComponentName as a StringRef you can avoid lots of allocations by 
using the same underlying string (which will grow and shrink, but not 
reallocate much).
You'll need a recursion helper with a different signature though.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:33
+  /// If Alloc is nullptr, tree is in brief mode and will ignore detail edges.
+  MemoryTree(llvm::BumpPtrAllocator *Alloc = nullptr) : Alloc(Alloc) {}
+

nit: I'd call this parameter DetailAlloc to hint at what's stored there



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:41
+  MemoryTree *addDetail(llvm::StringRef Name) {
+return Alloc ? (llvm::StringSaver(*Alloc).save(Name)) : this;
+  }

nit: `Name.copy(*Alloc)` is a little more direct



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:49
+  /// with dots(".").
+  void traverseTree(llvm::function_ref

nit: need to be explicit whether this is self or subtree (or could pass both)



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:31
+  /// detail to root, and doesn't report any descendants.
+  void traverseTree(bool CollapseDetails,
+llvm::function_ref sammccall wrote:
> > sammccall wrote:
> > > In earlier discussions we talked about avoiding the cost of *assembling* 
> > > the detailed tree, not just summarizing it at output time.
> > > 
> > > This requires `CollapseDetails` to be passed into the profiling function, 
> > > which in turn suggests making it part of a tree and passing a reference 
> > > to the tree in. e.g. an API like
> > > 
> > > ```
> > > class MemoryTree {
> > >   MemoryTree(bool Detailed);
> > >   MemoryTree* addChild(StringLiteral); // no copy
> > >   MemoryTree* addDetail(StringRef); // returns `this` unless detailed
> > >   void addUsage(size_t);
> > > }
> > > ```
> > It's hard to evaluate a tree-traversal API without its usages... how will 
> > this be used?
> you can find some in https://reviews.llvm.org/D88414
I guess you mean D88417?

This works well when exporting all nodes somewhere keyed by dotted path, but 
it's pretty awkward+inefficient if you want to - totally hypothetical example 
here - dump the memory tree as a JSON structure over RPC.

So I think we need a more flexible API, at which point... maybe we should punt, 
expose `const DenseMap () const`, and move the 
traverse implementation to the metrics exporter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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


[PATCH] D88810: [clangd] Add isKeyword function.

2020-10-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:638
+  // Keywords are initialized in constructor.
+  clang::IdentifierTable KeywordsTable(LangOpts);
+  return KeywordsTable.find(NewName) != KeywordsTable.end();

this builds a pretty decent sized hashtable for each call, because LangOpts 
isn't constant.

We could consider exposing getKeywordStatus from IdentifierTable.h, then we 
could build a StringMap (once, statically) and then call 
getKeywordStatus to apply the per-langopts logic.
In fact that StringMap might also be generally nice to expose in 
TokenKinds.h... as long as there are no conflicts.

Or we could have cache map from LangOpts to IdentifierTable, since we won't see 
many sets in practice.

Or maybe it doesn't matter, I suppose!



Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:794
+  LangOptions LangOpts;
+  LangOpts.CPlusPlus11 = true;
+  EXPECT_TRUE(isKeyword("int", LangOpts));

demonstrate some sensitivity to langopts?

e.g. set CPlusPlus20 to true then check co_await


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88810

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


[PATCH] D88634: [clangd] Extend the rename API.

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.

LG, thanks!




Comment at: clang-tools-extra/clangd/refactor/Rename.h:62
+  // Edits for the current main file.
+  Edit LocalChanges;
+  // Complete edits for the rename, including LocalChanges.

this could just be a vector, as we don't have a use case for the 
replacements and it avoids the issue of what to replace with. Up to you though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88634

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


[PATCH] D88721: [clangd][lit] Update document-link.test to respect custom resource-dir locations

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

In D88721#2308361 , @ArcsinX wrote:

> In D88721#2308296 , @sammccall wrote:
>
>>> As far as Windows accepts forward and back slashes
>>
>> Note we don't rely on windows itself for this support.
>> All access through `llvm::sys::fs` APIs on windows ultimately goes through 
>> `widenPath` to convert to UTF-16, and this substitutes slashes.
>> So LLVM tools do always support `/` on windows and it's fairly common to 
>> rely on this for tests (abstraction is hard in lit tests).
>
> I am not sure that understood you correctly, but `widenPath` does nothing for 
> me, if I pass mixed-slash path to it.

Ack, you're right. I've looked at this before, and was looking at this line:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Windows/Path.inc#L103
But in the common case, we've already exited by then, so you're absolutely 
right...

> Code:
>
>   std::string From("C:\\a/b\\c");
>   SmallVector To;
>   llvm::sys::windows::widenPath(From, To);
>   std::wcerr << To.data();
>
> Output:
>
>   C:\a/b\c
>
> So, if we imagine that Windows does not support `/`, then paths with `/` 
> could not be opened with `llvm::sys::fs` API.

But at a higher level, ISTM the purpose of `widenPath` is exactly to produce a 
path that the win32 APIs will accept, including if the input has `/` (as shown 
by the long-path case I was confused by).
So the contract of the llvm functions ensuring that forward slashes are safe 
still seems to be there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88721

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


[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:28
+size_t ChildrenSize = 0;
+for (const auto  : Children) {
+  C.traverseTree(CollapseDetails,

kadircet wrote:
> sammccall wrote:
> > Here the detailed nodes are entirely collapsed. This isn't quite as 
> > powerful as collapsing the detail *edges* only, which is worth considering.
> > 
> > e.g. consider TUScheduler. The natural accounting of resources is (IMO)
> > ```
> > clangd.TUScheduler..AST
> > clangd.TUScheduler..Preamble.InMemory
> > clangd.TUScheduler..Preamble.Inputs
> > ```
> > or something like that.
> > 
> > Instead of aggregating to `clangd.TUScheduler` only, collapsing the 
> > `` edges mean you get `clangd.TUScheduler.AST` etc.
> yes this was an option i hadn't considered. my only hesitation is this likely 
> implies different paths with the same name. current api also doesn't protect 
> against it, but they are a lot less likely as we collapse whole subtrees.
> 
> I believe deleting the edge should also imply aggregating all the paths with 
> the same name, e.g.
> `a.b..c, 5` and `a.b..c, 8` should end up being 
> `a.b.c,13`. WDYT?
Agree with your desired behavior.

If you're willing to make collapsing a property of the tree rather than the 
query, you avoid this problem, because you collapse up-front and make addChild 
idempotent:

```
class Tree {
  BumpPtrAllocator *DetailAllocator; // Null if no detail
  StringMap Children;
  size_t Self = 0;

  Tree* childImpl(StringRef Name) {
return &*Children.try_emplace(Name, DetailAllocator).first;
  }

public:  
  Tree(BumpPtrAllocator *);
  Tree* child(StringLiteral Name) { return childImpl(Name); }
  Tree* detail(StringRef Name) {
return DetailAllocator ? child(DetailAllocator->copy(Name)) : this;
  }
  void add(unsigned Size) { Self += Size; }
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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


[PATCH] D88721: [clangd][lit] Update document-link.test to respect custom resource-dir locations

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

> As far as Windows accepts forward and back slashes

Note we don't rely on windows itself for this support.
All access through `llvm::sys::fs` APIs on windows ultimately goes through 
`widenPath` to convert to UTF-16, and this substitutes slashes.
So LLVM tools do always support `/` on windows and it's fairly common to rely 
on this for tests (abstraction is hard in lit tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88721

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


[PATCH] D88726: [clangd] Make PopulateSwitch a fix.

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG57ac47d78885: [clangd] Make PopulateSwitch a fix. (authored 
by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88726

Files:
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp


Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -53,7 +53,7 @@
   Expected apply(const Selection ) override;
   std::string title() const override { return "Populate switch"; }
   llvm::StringLiteral kind() const override {
-return CodeAction::REFACTOR_KIND;
+return CodeAction::QUICKFIX_KIND;
   }
 
 private:


Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -53,7 +53,7 @@
   Expected apply(const Selection ) override;
   std::string title() const override { return "Populate switch"; }
   llvm::StringLiteral kind() const override {
-return CodeAction::REFACTOR_KIND;
+return CodeAction::QUICKFIX_KIND;
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D88726: [clangd] Make PopulateSwitch a fix.

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

It fixes the -Wswitch warning, though we mark it as a fix even if that is off.
This makes it the "recommended" action on an incomplete switch, which seems OK.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88726

Files:
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp


Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -53,7 +53,7 @@
   Expected apply(const Selection ) override;
   std::string title() const override { return "Populate switch"; }
   llvm::StringLiteral kind() const override {
-return CodeAction::REFACTOR_KIND;
+return CodeAction::QUICKFIX_KIND;
   }
 
 private:


Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -53,7 +53,7 @@
   Expected apply(const Selection ) override;
   std::string title() const override { return "Populate switch"; }
   llvm::StringLiteral kind() const override {
-return CodeAction::REFACTOR_KIND;
+return CodeAction::QUICKFIX_KIND;
   }
 
 private:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D88724: [clangd] Make the tweak filter a parameter to enumerateTweaks. NFC

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

(Required for CodeActionContext.only)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88724

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h

Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -163,11 +163,6 @@
 /// Enable preview of FoldingRanges feature.
 bool FoldingRanges = false;
 
-/// Returns true if the tweak should be enabled.
-std::function TweakFilter = [](const Tweak ) {
-  return !T.hidden(); // only enable non-hidden tweaks.
-};
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
@@ -291,7 +286,9 @@
 llvm::StringLiteral Kind;
   };
   /// Enumerate the code tweaks available to the user at a specified point.
+  /// Tweaks where Filter returns false will not be checked or included.
   void enumerateTweaks(PathRef File, Range Sel,
+   llvm::unique_function Filter,
Callback> CB);
 
   /// Apply the code tweak with a specified \p ID.
@@ -379,8 +376,6 @@
   // If true, preserve the type for recovery AST.
   bool PreserveRecoveryASTType = false;
 
-  std::function TweakFilter;
-
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap>
   CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -180,7 +180,7 @@
   SuggestMissingIncludes(Opts.SuggestMissingIncludes),
   BuildRecoveryAST(Opts.BuildRecoveryAST),
   PreserveRecoveryASTType(Opts.PreserveRecoveryASTType),
-  TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
+  WorkspaceRoot(Opts.WorkspaceRoot),
   // Pass a callback into `WorkScheduler` to extract symbols from a newly
   // parsed file and rebuild the file index synchronously each time an AST
   // is parsed.
@@ -501,13 +501,15 @@
   return std::move(Result);
 }
 
-void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
-   Callback> CB) {
+void ClangdServer::enumerateTweaks(
+PathRef File, Range Sel, llvm::unique_function Filter,
+Callback> CB) {
   // Tracks number of times a tweak has been offered.
   static constexpr trace::Metric TweakAvailable(
   "tweak_available", trace::Metric::Counter, "tweak_id");
   auto Action = [File = File.str(), Sel, CB = std::move(CB),
- this](Expected InpAST) mutable {
+ Filter =
+ std::move(Filter)](Expected InpAST) mutable {
 if (!InpAST)
   return CB(InpAST.takeError());
 auto Selections = tweakSelection(Sel, *InpAST);
@@ -516,11 +518,11 @@
 std::vector Res;
 // Don't allow a tweak to fire more than once across ambiguous selections.
 llvm::DenseSet PreparedTweaks;
-auto Filter = [&](const Tweak ) {
-  return TweakFilter(T) && !PreparedTweaks.count(T.id());
+auto DeduplicatingFilter = [&](const Tweak ) {
+  return Filter(T) && !PreparedTweaks.count(T.id());
 };
 for (const auto  : *Selections) {
-  for (auto  : prepareTweaks(*Sel, Filter)) {
+  for (auto  : prepareTweaks(*Sel, DeduplicatingFilter)) {
 Res.push_back({T->id(), T->title(), T->kind()});
 PreparedTweaks.insert(T->id());
 TweakAvailable.record(1, T->id());
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -50,6 +50,10 @@
 /// per-request, but LSP allows limited/no customizations.
 clangd::CodeCompleteOptions CodeComplete;
 clangd::RenameOptions Rename;
+/// Returns true if the tweak should be enabled.
+std::function TweakFilter = [](const Tweak ) {
+  return !T.hidden(); // only enable non-hidden tweaks.
+};
   };
 
   ClangdLSPServer(Transport , const ThreadsafeFS ,
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1024,7 +1024,15 @@
 return Reply(llvm::json::Array(Commands));
   };
 
-  Server->enumerateTweaks(File.file(), Params.range, std::move(ConsumeActions));
+  

[PATCH] D88427: [clangd] Remove Tweak::Intent, use CodeAction kind directly

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG17747d2ec8ec: [clangd] Remove Tweak::Intent, use CodeAction 
kind directly. NFC (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D88427?vs=294726=295763#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88427

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
  clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
  clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
  clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp

Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -39,7 +39,9 @@
   bool prepare(const Selection ) override;
   Expected apply(const Selection ) override;
   std::string title() const override { return "Swap if branches"; }
-  Intent intent() const override { return Refactor; }
+  llvm::StringLiteral kind() const override {
+return CodeAction::REFACTOR_KIND;
+  }
   bool hidden() const override { return true; }
 
 private:
Index: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
@@ -39,7 +39,9 @@
   bool prepare(const Selection ) override;
   Expected apply(const Selection ) override;
   std::string title() const override;
-  Intent intent() const override { return Refactor; }
+  llvm::StringLiteral kind() const override {
+return CodeAction::REFACTOR_KIND;
+  }
 
 private:
   const UsingDirectiveDecl *TargetDirective = nullptr;
Index: clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
@@ -41,7 +41,9 @@
   bool prepare(const Selection ) override;
   Expected apply(const Selection ) override;
   std::string title() const override { return "Convert to raw string"; }
-  Intent intent() const override { return Refactor; }
+  llvm::StringLiteral kind() const override {
+return CodeAction::REFACTOR_KIND;
+  }
 
 private:
   const clang::StringLiteral *Str = nullptr;
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -52,7 +52,9 @@
   bool prepare(const Selection ) override;
   Expected apply(const Selection ) override;
   std::string title() const override { return "Populate switch"; }
-  Intent intent() const override { return Refactor; }
+  llvm::StringLiteral kind() const override {
+return CodeAction::REFACTOR_KIND;
+  }
 
 private:
   const DeclContext *DeclCtx = nullptr;
Index: clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
@@ -35,7 +35,9 @@
 class ObjCLocalizeStringLiteral : public Tweak {
 public:
   const char *id() const override final;
-  Intent intent() const override { return Intent::Refactor; }
+  llvm::StringLiteral kind() const override {
+return CodeAction::REFACTOR_KIND;
+  }
 
   bool prepare(const Selection ) override;
   Expected apply(const Selection ) override;
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ 

[PATCH] D88721: [clangd][lit] Update document-link.test to respect custom resource-dir locations

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/test/document-link.test:1
-# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+# %resource_dir actually points at builtin_include_dir, go up one directory to
+# reach resource_dir.

nit: i'd drop "to reach resource_dir" to get this to fit on one line
(Remembering that this quirk is almost always uninteresting to people reading 
this test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88721

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


[PATCH] D88634: [clangd] Extend the rename API.

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, introducing a separate field for locally affected ranges is up to you.




Comment at: clang-tools-extra/clangd/ClangdServer.h:277
+  ///
+  /// It is equal to invoke rename with an empty NewName, except that the
+  /// returned resuslt only contains main-file occurrences (for performance).

This seems a little like spelling out too many implementation details, and 
"equal to invoke" isn't quite right grammatically. Maybe just...

"The returned result describes edits in the current file only (all occurrences 
of the target name are simply deleted)".

(Note that if we split out the field for file ranges it could just be 
vector, avoiding the need to explain the replacement with empty string)



Comment at: clang-tools-extra/clangd/ClangdServer.h:278
+  /// It is equal to invoke rename with an empty NewName, except that the
+  /// returned resuslt only contains main-file occurrences (for performance).
   void prepareRename(PathRef File, Position Pos,

nit: resulslt -> result



Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+  Range R;
+  FileEdits Edits;
+};

hokein wrote:
> sammccall wrote:
> > It's slightly awkward to have the half-populated state (may or may not 
> > contain cross-file results, can't tell without the query).
> > 
> > I'd consider redundantly including `Edit LocalChanges` and `FileEdits 
> > GlobalChanges` with the former always set and the latter empty when 
> > returned from preparerename.
> > It's slightly awkward to have the half-populated state (may or may not 
> > contain cross-file results, can't tell without the query).
> 
> I feel this is not too bad, the query is quite trivial, just `Edits.size() > 
> 1`.
> 
> > I'd consider redundantly including Edit LocalChanges and FileEdits 
> > GlobalChanges with the former always set and the latter empty when returned 
> > from preparerename.
> 
> This change seems nice to get changes for main-file, but I think 
> `GlobalChanges` should include LocalChanges (otherwise, client side needs to 
> combine these two when applying the final rename edits)? then we can't leave 
> the later empty while keeping the former set.
> 
> Happy to do the change, but it looks like we don't have usage of 
> `LocalChanges` so far. In prepareRename, we want main-file occurrences, 
> `rename` will always return them regardless of single-file or cross-file 
> rename, so I think `Edits` is efficient. 
> > It's slightly awkward to have the half-populated state (may or may not 
> > contain cross-file results, can't tell without the query).
> 
> I feel this is not too bad, the query is quite trivial, just `Edits.size() > 
> 1`.
This isn't sufficient though: if Edits.size() == 1 then the results may be 
either complete or incomplete. (Sorry my wording above may have been poor, but 
this is the distinction I was referring to).

> > I'd consider redundantly including Edit LocalChanges and FileEdits 
> > GlobalChanges with the former always set and the latter empty when returned 
> > from preparerename.
> 
> This change seems nice to get changes for main-file, but I think 
> `GlobalChanges` should include LocalChanges (otherwise, client side needs to 
> combine these two when applying the final rename edits)? then we can't leave 
> the later empty while keeping the former set.

Sure we can. `// If the full set of changes is unknown, this field is empty.`

> Happy to do the change, but it looks like we don't have usage of 
> `LocalChanges` so far. In prepareRename, we want main-file occurrences, 
> `rename` will always return them regardless of single-file or cross-file 
> rename, so I think `Edits` is efficient. 

Yes, it's possible to implement correct behavior on top of the current API. It 
effectively reuses the same field with different semantics/contracts, and the 
client has enough information to know which contract is in place (and throw 
away the key in the expected singleton map).
However I think this is fragile and harder to understand than providing 
separate fields for the two contracts - using types to distinguish local vs 
global results makes the data harder to misuse. Given we expect this to be used 
by embedders, I think it's worth adding the second field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88634

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


[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

2020-10-02 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Sorry @rsmith @hoy, I've replaced the test with one without such dependencies 
in bc18d8d9b705d31a94c51900c8b18e4feaf9c7fb 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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


[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

2020-10-01 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

Thanks, I think this fails whenever `compile_commands.json` doesn't exist in 
the tree or under `build`.
(Which of course it does in my local checkout...). Reverted, probably need to 
rename/copy the test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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


[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

2020-10-01 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG79fbcbff4173: [clangd] clangd --check: standalone diagnosis 
of common problems (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D88338?vs=295290=295558#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

Files:
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -47,6 +47,11 @@
 
 namespace clang {
 namespace clangd {
+
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   const ClangdLSPServer::Options );
+
 namespace {
 
 using llvm::cl::cat;
@@ -57,6 +62,7 @@
 using llvm::cl::list;
 using llvm::cl::opt;
 using llvm::cl::OptionCategory;
+using llvm::cl::ValueOptional;
 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
@@ -354,6 +360,16 @@
 Hidden,
 };
 
+opt CheckFile{
+"check",
+cat(Misc),
+desc("Parse one file in isolation instead of acting as a language server. "
+ "Useful to investigate/reproduce crashes or configuration problems. "
+ "With --check=, attempts to parse a particular file."),
+init(""),
+ValueOptional,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt PCHStorage{
 "pch-storage",
@@ -541,7 +557,8 @@
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
-  CantRunAsXPCService = 2
+  CantRunAsXPCService = 2,
+  CheckFailed = 3
 };
 
 int main(int argc, char *argv[]) {
@@ -646,7 +663,8 @@
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
-  if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
+  if (llvm::outs().is_displayed() && llvm::errs().is_displayed() &&
+  !CheckFile.getNumOccurrences())
 llvm::errs() << Overview << "\n";
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
@@ -825,6 +843,15 @@
   // Shall we allow to customize the file limit?
   Opts.Rename.AllowCrossFile = CrossFileRename;
 
+  if (CheckFile.getNumOccurrences()) {
+llvm::SmallString<256> Path;
+llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
+log("Entering check mode (no LSP server)");
+return check(Path, TFS, Opts)
+   ? 0
+   : static_cast(ErrorResultCode::CheckFailed);
+  }
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
@@ -835,7 +862,7 @@
 TransportLayer = newXPCTransport();
 #else
 llvm::errs() << "This clangd binary wasn't built with XPC support.\n";
-return (int)ErrorResultCode::CantRunAsXPCService;
+return static_cast(ErrorResultCode::CantRunAsXPCService);
 #endif
   } else {
 log("Starting LSP over stdin/stdout");
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -0,0 +1,258 @@
+//===--- Check.cpp - clangd self-diagnostics --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Many basic problems can occur processing a file in clangd, e.g.:
+//  - system includes are not found
+//  - crash when indexing its AST
+// clangd --check provides a simplified, isolated way to reproduce these,
+// with no editor, LSP, threads, background indexing etc to contend with.
+//
+// One important use case is gathering information for bug reports.
+// Another is reproducing crashes, and checking which setting prevent them.
+//
+// It simulates opening a file (determining compile command, parsing, indexing)
+// and then running features at many locations.
+//
+// Currently it adds some basic logging of progress and results.
+// We should consider extending it to also recognize common symptoms and
+// recommend solutions (e.g. standard library installation issues).
+//

[PATCH] D88634: [clangd] Extend the rename API.

2020-10-01 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:408
   return CB(InpAST.takeError());
-auto  = InpAST->AST;
-const auto  = AST.getSourceManager();
-auto Loc = sourceLocationInMainFile(SM, Pos);
-if (!Loc)
-  return CB(Loc.takeError());
-const auto *TouchingIdentifier =
-spelledIdentifierTouching(*Loc, AST.getTokens());
-if (!TouchingIdentifier)
-  return CB(llvm::None); // no rename on non-identifiers.
-
-auto Range = halfOpenToRange(
-SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
-  TouchingIdentifier->endLocation()));
-
-if (RenameOpts.AllowCrossFile)
-  // FIXME: we now assume cross-file rename always succeeds, revisit this.
-  return CB(Range);
-
-// Performing the local rename isn't substantially more expensive than
-// doing an AST-based check, so we just rename and throw away the results.
-auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts,
-   /*GetDirtyBuffer=*/nullptr});
-if (!Changes) {
+clangd::FileIndex EmptyIndex;
+// prepareRename is latency-sensitive:

the empty index is weird here - can we pass nullptr?
Currently nullptr leads to an error in the !crossfile case, but I think we can 
give rename(Index=nullptr, RenameOpts.CrossFile=true) the behavior we want.

(otherwise, if we need an empty index use MemIndex())



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:411
+//  - for single-file rename, performing rename isn't substantially more
+//expensive than doing an AST-based check;
+//  - for cross-file rename, we deliberately pass an empty index to save 
the

mention "the index is used to see if the local rename is complete"?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:416
+auto Results = clangd::rename(
+{Pos, "dummy", InpAST->AST, File,
+ RenameOpts.AllowCrossFile ?  : Index, RenameOpts});

we're now returning the "dummy" string to the caller, so we should document it 
somewhere (or ideally just make it the empty string and document that)



Comment at: clang-tools-extra/clangd/ClangdServer.h:277
+  ///
+  /// The returned result may be incomplete as it only contains occurrences 
from
+  /// the current main file.

nit: drop "may be incomplete as it"?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:500
   if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
-return FileEdits(
-{std::make_pair(RInputs.MainFilePath,
-Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
+return RenameResults{
+CurrentIdentifier,

nit: I'd find this more readable as a default construction followed by 
assignments to the fields



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:507
 
-  FileEdits Results;
+  FileEdits Edits;
   // Renameable safely guards us that at this point we are renaming a local

and again here - declare the whole struct first and fill it in?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:58
 
+struct RenameResults {
+  // The range of the symbol that the user can attempt to rename.

nit: I'd suggest RenameResult singular, consistent with CodeCompleteResult and 
ReferencesResult



Comment at: clang-tools-extra/clangd/refactor/Rename.h:60
+  // The range of the symbol that the user can attempt to rename.
+  Range R;
+  FileEdits Edits;

Give this a real name... Target?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+  Range R;
+  FileEdits Edits;
+};

It's slightly awkward to have the half-populated state (may or may not contain 
cross-file results, can't tell without the query).

I'd consider redundantly including `Edit LocalChanges` and `FileEdits 
GlobalChanges` with the former always set and the latter empty when returned 
from preparerename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88634

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


[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

2020-10-01 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:237
+
+  StringRef HeaderStem =
+  llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(

this deserves a comment: Not matchingStem: implementation files may have 
compound extensions but headers may not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

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


[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 295290.
sammccall marked 6 inline comments as done.
sammccall added a comment.

address (some of) comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

Files:
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -47,6 +47,11 @@
 
 namespace clang {
 namespace clangd {
+
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   const ClangdLSPServer::Options );
+
 namespace {
 
 using llvm::cl::cat;
@@ -57,6 +62,7 @@
 using llvm::cl::list;
 using llvm::cl::opt;
 using llvm::cl::OptionCategory;
+using llvm::cl::ValueOptional;
 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
@@ -354,6 +360,16 @@
 Hidden,
 };
 
+opt CheckFile{
+"check",
+cat(Misc),
+desc("Parse one file in isolation instead of acting as a language server. "
+ "Useful to investigate/reproduce crashes or configuration problems. "
+ "With --check=, attempts to parse a particular file."),
+init(""),
+ValueOptional,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt PCHStorage{
 "pch-storage",
@@ -541,7 +557,8 @@
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
-  CantRunAsXPCService = 2
+  CantRunAsXPCService = 2,
+  CheckFailed = 3
 };
 
 int main(int argc, char *argv[]) {
@@ -646,7 +663,8 @@
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
-  if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
+  if (llvm::outs().is_displayed() && llvm::errs().is_displayed() &&
+  !CheckFile.getNumOccurrences())
 llvm::errs() << Overview << "\n";
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
@@ -825,6 +843,15 @@
   // Shall we allow to customize the file limit?
   Opts.Rename.AllowCrossFile = CrossFileRename;
 
+  if (CheckFile.getNumOccurrences()) {
+llvm::SmallString<256> Path;
+llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
+log("Entering check mode (no LSP server)");
+return check(Path, TFS, Opts)
+   ? 0
+   : static_cast(ErrorResultCode::CheckFailed);
+  }
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
@@ -835,7 +862,7 @@
 TransportLayer = newXPCTransport();
 #else
 llvm::errs() << "This clangd binary wasn't built with XPC support.\n";
-return (int)ErrorResultCode::CantRunAsXPCService;
+return static_cast(ErrorResultCode::CantRunAsXPCService);
 #endif
   } else {
 log("Starting LSP over stdin/stdout");
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -0,0 +1,259 @@
+//===--- Check.cpp - clangd self-diagnostics --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Many basic problems can occur processing a file in clangd, e.g.:
+//  - system includes are not found
+//  - crash when indexing its AST
+// clangd --check provides a simplified, isolated way to reproduce these,
+// with no editor, LSP, threads, background indexing etc to contend with.
+//
+// One important use case is gathering information for bug reports.
+// Another is reproducing crashes, and checking which setting prevent them.
+//
+// It simulates opening a file (determining compile command, parsing, indexing)
+// and then running features at many locations.
+//
+// Currently it adds some basic logging of progress and results.
+// We should consider extending it to also recognize common symptoms and
+// recommend solutions (e.g. standard library installation issues).
+//
+//===--===//
+
+#include "ClangdLSPServer.h"
+#include "CodeComplete.h"
+#include "GlobalCompilationDatabase.h"
+#include "Hover.h"
+#include "ParsedAST.h"
+#include "Preamble.h"
+#include "SourceCode.h"
+#include "XRefs.h"
+#include 

[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:87
+auto Mangler = CommandMangler::detect();
+// Check for missing resource dir?
+if (Opts.ResourceDir)

kadircet wrote:
> i agree, this would help identifying the case of "clangd binary has been 
> copied to some place without the necessary lib directory".
> 
> but i think we should check for the final `-resource-dir` in the compile 
> command below. as user's compile flags can override whatever default clangd 
> comes up with.
Moved FIXME to the right place.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+Inputs.Opts.ClangTidyOpts =
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");

kadircet wrote:
> IIRC, option providers don't really log anything about success/failure of 
> parsing the config file. what's the point of having this exactly?
This is needed in order to run the correct clang-tidy checks.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:126
+Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+log("Parsing command...");
+Invocation =

kadircet wrote:
> it is clear that we've crashed while parsing compile commands if we don't see 
> `cc1 args are` in the output. so maybe drop this one?
I don't think that's clear - you can find it out by reading the code, but I 
expect people to be running this who aren't reading the code. (They won't be 
able to work out all the details, but they tend to be interested in the broad 
strokes of what's going on).

So generally I've tried to include log statements before each major step.



Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+showErrors(InvocationDiags);
+log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+if (!Invocation) {

kadircet wrote:
> maybe we should also note that this doesn't reflect the final result. as we 
> turn off pchs or dependency file outputting related flags afterwards.
Sure, and we also run clang-tidy, and fiddle with the preamble, and skip 
function bodies...

I think a disclaimer that "running clangd is not equivalent to running clang 
" would be somewhat confusing, as I'm not sure that's a thing that 
anyone would expect to be true.

I expect the cc1 args to be useless to anyone not familiar with clangd 
internals, but they're kind of important to include. Reworded the log message a 
bit, is this enough?



Comment at: clang-tools-extra/clangd/tool/Check.cpp:202
+  }
+  unsigned Definitions = locateSymbolAt(*AST, Pos, ).size();
+  vlog("definition: {0}", Definitions);

kadircet wrote:
> maybe we could print errors if the following has no results for "identifiers" 
> ?
There are lots of ways go-to-def can yield no results (e.g. macros, #ifdef-'d 
sections, templated code we can't resolve)



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:52
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS ,
+   llvm::Optional CompileCommandsPath,

kadircet wrote:
> why not have this in `Check.h` ?
A header seems a bit out of place in tool/ and it doesn't seem necessary, being 
one function with a simple signature and no unit tests.

We'll get a linker error if we ever get this wrong, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338

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


[PATCH] D88567: [clangd] Fix invalid UTF8 when extracting doc comments.

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG216af81c39d1: [clangd] Fix invalid UTF8 when extracting doc 
comments. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D88567?vs=295265=295266#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88567

Files:
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1606,11 +1606,11 @@
   // Extracted from boost/spirit/home/support/char_encoding/iso8859_1.hpp
   // This looks like UTF-8 and fools clang, but has high-ISO-8859-1 comments.
   const char *Header = "int PUNCT = 0;\n"
-   "int types[] = { /* \xa1 */PUNCT };";
+   "/* \xa1 */ int types[] = { /* \xa1 */PUNCT };";
   CollectorOpts.RefFilter = RefKind::All;
   CollectorOpts.RefsInHeaders = true;
   runSymbolCollector(Header, "");
-  EXPECT_THAT(Symbols, Contains(QName("types")));
+  EXPECT_THAT(Symbols, Contains(AllOf(QName("types"), Doc("\xef\xbf\xbd ";
   EXPECT_THAT(Symbols, Contains(QName("PUNCT")));
   // Reference is stored, although offset within line is not reliable.
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "PUNCT").ID, _)));
Index: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "CodeCompletionStrings.h"
+#include "TestTU.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -56,6 +57,14 @@
 "Annotation: Ano\n\nIs this brief?");
 }
 
+TEST_F(CompletionStringTest, GetDeclCommentBadUTF8) {
+  //  is not a valid byte here, should be replaced by encoded .
+  auto TU = TestTU::withCode("/*x\xffy*/ struct X;");
+  auto AST = TU.build();
+  EXPECT_EQ("x\xef\xbf\xbdy",
+getDeclComment(AST.getASTContext(), findDecl(AST, "X")));
+}
+
 TEST_F(CompletionStringTest, MultipleAnnotations) {
   Builder.AddAnnotation("Ano1");
   Builder.AddAnnotation("Ano2");
Index: clang-tools-extra/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/clangd/CodeCompletionStrings.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "llvm/Support/JSON.h"
 #include 
 #include 
 
@@ -86,7 +87,12 @@
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc =
   RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
-  return looksLikeDocComment(Doc) ? Doc : "";
+  if (!looksLikeDocComment(Doc))
+return "";
+  // Clang requires source to be UTF-8, but doesn't enforce this in comments.
+  if (!llvm::json::isUTF8(Doc))
+Doc = llvm::json::fixUTF8(Doc);
+  return Doc;
 }
 
 void getSignature(const CodeCompletionString , std::string *Signature,


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1606,11 +1606,11 @@
   // Extracted from boost/spirit/home/support/char_encoding/iso8859_1.hpp
   // This looks like UTF-8 and fools clang, but has high-ISO-8859-1 comments.
   const char *Header = "int PUNCT = 0;\n"
-   "int types[] = { /* \xa1 */PUNCT };";
+   "/* \xa1 */ int types[] = { /* \xa1 */PUNCT };";
   CollectorOpts.RefFilter = RefKind::All;
   CollectorOpts.RefsInHeaders = true;
   runSymbolCollector(Header, "");
-  EXPECT_THAT(Symbols, Contains(QName("types")));
+  EXPECT_THAT(Symbols, Contains(AllOf(QName("types"), Doc("\xef\xbf\xbd ";
   EXPECT_THAT(Symbols, Contains(QName("PUNCT")));
   // Reference is stored, although offset within line is not reliable.
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "PUNCT").ID, _)));
Index: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
+++ 

[PATCH] D88567: [clangd] Fix invalid UTF8 when extracting doc comments.

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

In D88567#2303332 , @kadircet wrote:

> thanks, LGTM!
>
> Should we also have another test for SymbolCollector, to ensure we don't 
> regress this somehow in the future?

We had a SymbolCollector test for the boost case so I modified it to add a doc 
comment.




Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:93
+  // Clang requires source to be UTF-8, but doesn't enforce this in comments.
+  if (!llvm::json::isUTF8(Doc))
+Doc = llvm::json::fixUTF8(Doc);

kadircet wrote:
> it is always surprising to have these helpers in json library :D (just 
> talking out loud)
Yeah. They're just wrappers around functions from `ConvertUTF.h`.
Do you want a patch to move them there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88567

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


[PATCH] D88567: [clangd] Fix invalid UTF8 when extracting doc comments.

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88567

Files:
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp


Index: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "CodeCompletionStrings.h"
+#include "TestTU.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -56,6 +57,14 @@
 "Annotation: Ano\n\nIs this brief?");
 }
 
+TEST_F(CompletionStringTest, GetDeclCommentBadUTF8) {
+  //  is not a valid byte here, should be replaced by encoded .
+  auto TU = TestTU::withCode("/*x\xffy*/ struct X;");
+  auto AST = TU.build();
+  EXPECT_EQ("x\xef\xbf\xbdy",
+getDeclComment(AST.getASTContext(), findDecl(AST, "X")));
+}
+
 TEST_F(CompletionStringTest, MultipleAnnotations) {
   Builder.AddAnnotation("Ano1");
   Builder.AddAnnotation("Ano2");
Index: clang-tools-extra/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/clangd/CodeCompletionStrings.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "llvm/Support/JSON.h"
 #include 
 #include 
 
@@ -86,7 +87,12 @@
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc =
   RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
-  return looksLikeDocComment(Doc) ? Doc : "";
+  if (!looksLikeDocComment(Doc))
+return "";
+  // Clang requires source to be UTF-8, but doesn't enforce this in comments.
+  if (!llvm::json::isUTF8(Doc))
+Doc = llvm::json::fixUTF8(Doc);
+  return Doc;
 }
 
 void getSignature(const CodeCompletionString , std::string *Signature,


Index: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "CodeCompletionStrings.h"
+#include "TestTU.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -56,6 +57,14 @@
 "Annotation: Ano\n\nIs this brief?");
 }
 
+TEST_F(CompletionStringTest, GetDeclCommentBadUTF8) {
+  //  is not a valid byte here, should be replaced by encoded .
+  auto TU = TestTU::withCode("/*x\xffy*/ struct X;");
+  auto AST = TU.build();
+  EXPECT_EQ("x\xef\xbf\xbdy",
+getDeclComment(AST.getASTContext(), findDecl(AST, "X")));
+}
+
 TEST_F(CompletionStringTest, MultipleAnnotations) {
   Builder.AddAnnotation("Ano1");
   Builder.AddAnnotation("Ano2");
Index: clang-tools-extra/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/clangd/CodeCompletionStrings.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "llvm/Support/JSON.h"
 #include 
 #include 
 
@@ -86,7 +87,12 @@
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc =
   RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
-  return looksLikeDocComment(Doc) ? Doc : "";
+  if (!looksLikeDocComment(Doc))
+return "";
+  // Clang requires source to be UTF-8, but doesn't enforce this in comments.
+  if (!llvm::json::isUTF8(Doc))
+Doc = llvm::json::fixUTF8(Doc);
+  return Doc;
 }
 
 void getSignature(const CodeCompletionString , std::string *Signature,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D88489: [clangd] Mark code action as "preferred" if it's the sole quickfix action

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8392685c2b9f: [clangd] Mark code action as 
preferred if its the sole quickfix action (authored by 
sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88489

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test


Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
+# CHECK-NEXT:"isPreferred": true,
 # CHECK-NEXT:"kind": "quickfix",
 # CHECK-NEXT:"title": "change 'union' to 'struct'"
 # CHECK-NEXT:  }
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -952,6 +952,13 @@
   /// The diagnostics that this code action resolves.
   llvm::Optional> diagnostics;
 
+  /// Marks this as a preferred action. Preferred actions are used by the
+  /// `auto fix` command and can be targeted by keybindings.
+  /// A quick fix should be marked preferred if it properly addresses the
+  /// underlying error. A refactoring should be marked preferred if it is the
+  /// most reasonable choice of actions to take.
+  bool isPreferred = false;
+
   /// The workspace edit this code action performs.
   llvm::Optional edit;
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -740,6 +740,8 @@
 CodeAction["kind"] = *CA.kind;
   if (CA.diagnostics)
 CodeAction["diagnostics"] = llvm::json::Array(*CA.diagnostics);
+  if (CA.isPreferred)
+CodeAction["isPreferred"] = true;
   if (CA.edit)
 CodeAction["edit"] = *CA.edit;
   if (CA.command)
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -411,6 +411,8 @@
 Main.codeActions.emplace();
 for (const auto  : D.Fixes)
   Main.codeActions->push_back(toCodeAction(Fix, File));
+if (Main.codeActions->size() == 1)
+  Main.codeActions->front().isPreferred = true;
   }
   if (Opts.SendDiagnosticCategory && !D.Category.empty())
 Main.category = D.Category;
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1007,6 +1007,20 @@
 for (const auto  : *Tweaks)
   Actions.push_back(toCodeAction(T, File, Selection));
 
+// If there's exactly one quick-fix, call it "preferred".
+// We never consider refactorings etc as preferred.
+CodeAction *OnlyFix = nullptr;
+for (auto  : Actions) {
+  if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) {
+if (OnlyFix) {
+  OnlyFix->isPreferred = false;
+  break;
+}
+Action.isPreferred = true;
+OnlyFix = 
+  }
+}
+
 if (SupportsCodeAction)
   return Reply(llvm::json::Array(Actions));
 std::vector Commands;


Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
+# CHECK-NEXT:"isPreferred": true,
 # CHECK-NEXT:"kind": "quickfix",
 # CHECK-NEXT:"title": "change 'union' to 'struct'"
 # CHECK-NEXT:  }
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -952,6 +952,13 @@
   /// The diagnostics that this code action resolves.
   llvm::Optional> diagnostics;
 
+  /// Marks this as a preferred action. Preferred actions are used by the
+  /// `auto fix` command and can be targeted by keybindings.
+  /// A quick fix should be marked preferred if it properly addresses the
+  /// 

[PATCH] D88470: [clangd] Extract options struct for ClangdLSPServer. NFC

2020-09-30 Thread Sam McCall via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7ba0779fbb41: [clangd] Extract options struct for 
ClangdLSPServer. NFC (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D88470?vs=294907=295197#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88470

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp

Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -8,11 +8,9 @@
 
 #include "Annotations.h"
 #include "ClangdLSPServer.h"
-#include "CodeComplete.h"
 #include "LSPClient.h"
 #include "Protocol.h"
 #include "TestFS.h"
-#include "refactor/Rename.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
 #include "llvm/ADT/StringRef.h"
@@ -36,13 +34,14 @@
 
 class LSPTest : public ::testing::Test, private clangd::Logger {
 protected:
-  LSPTest() : LogSession(*this) {}
+  LSPTest() : LogSession(*this) {
+ClangdServer::Options  = Opts;
+Base = ClangdServer::optsForTest();
+  }
 
   LSPClient () {
 EXPECT_FALSE(Server.hasValue()) << "Already initialized";
-Server.emplace(Client.transport(), FS, CCOpts, RenameOpts,
-   /*CompileCommandsDir=*/llvm::None, /*UseDirBasedCDB=*/false,
-   /*ForcedOffsetEncoding=*/llvm::None, Opts);
+Server.emplace(Client.transport(), FS, Opts);
 ServerThread.emplace([&] { EXPECT_TRUE(Server->run()); });
 Client.call("initialize", llvm::json::Object{});
 return Client;
@@ -64,9 +63,7 @@
   }
 
   MockFS FS;
-  CodeCompleteOptions CCOpts;
-  RenameOptions RenameOpts;
-  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  ClangdLSPServer::Options Opts;
 
 private:
   // Color logs so we can distinguish them from test output.
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -670,9 +670,11 @@
   if (auto EnvFlags = llvm::sys::Process::GetEnv(FlagsEnvVar))
 log("{0}: {1}", FlagsEnvVar, *EnvFlags);
 
+  ClangdLSPServer::Options Opts;
+  Opts.UseDirBasedCDB = (CompileArgsFrom == FilesystemCompileArgs);
+
   // If --compile-commands-dir arg was invoked, check value and override default
   // path.
-  llvm::Optional CompileCommandsDirPath;
   if (!CompileCommandsDir.empty()) {
 if (llvm::sys::fs::exists(CompileCommandsDir)) {
   // We support passing both relative and absolute paths to the
@@ -686,7 +688,7 @@
  "will be ignored.",
  EC.message());
   } else {
-CompileCommandsDirPath = std::string(Path.str());
+Opts.CompileCommandsDir = std::string(Path.str());
   }
 } else {
   elog("Path specified by --compile-commands-dir does not exist. The "
@@ -694,7 +696,6 @@
 }
   }
 
-  ClangdServer::Options Opts;
   switch (PCHStorage) {
   case PCHStorageFlag::Memory:
 Opts.StorePreamblesInMemory = true;
@@ -744,23 +745,22 @@
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
 
-  clangd::CodeCompleteOptions CCOpts;
-  CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
-  CCOpts.Limit = LimitResults;
+  Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
+  Opts.CodeComplete.Limit = LimitResults;
   if (CompletionStyle.getNumOccurrences())
-CCOpts.BundleOverloads = CompletionStyle != Detailed;
-  CCOpts.ShowOrigins = ShowOrigins;
-  CCOpts.InsertIncludes = HeaderInsertion;
+Opts.CodeComplete.BundleOverloads = CompletionStyle != Detailed;
+  Opts.CodeComplete.ShowOrigins = ShowOrigins;
+  Opts.CodeComplete.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {
-CCOpts.IncludeIndicator.Insert.clear();
-CCOpts.IncludeIndicator.NoInsert.clear();
+Opts.CodeComplete.IncludeIndicator.Insert.clear();
+Opts.CodeComplete.IncludeIndicator.NoInsert.clear();
   }
-  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
-  CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
-  CCOpts.AllScopes = AllScopesCompletion;
-  CCOpts.RunParser = CodeCompletionParse;
-  CCOpts.RankingModel = RankingModel;
-  CCOpts.DecisionForestBase = DecisionForestBase;
+  Opts.CodeComplete.SpeculativeIndexRequest = Opts.StaticIndex;
+  Opts.CodeComplete.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
+  Opts.CodeComplete.AllScopes = AllScopesCompletion;
+  Opts.CodeComplete.RunParser = CodeCompletionParse;
+  

[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches

2020-09-29 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4fb303f340e2: [clangd] Improve PopulateSwitch tweak to work 
on non-empty switches (authored by tdeo, committed by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88434

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

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2829,9 +2829,48 @@
   "unavailable",
   },
   {
-  // Existing enumerators in switch
+  // All enumerators already in switch (unscoped)
   Function,
-  R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+  R""(enum Enum {A,B}; ^switch (A) {case A:break;case B:break;})"",
+  "unavailable",
+  },
+  {
+  // All enumerators already in switch (scoped)
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A:break;case Enum::B:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // Default case in switch
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {default:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // GNU range in switch
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A ... Enum::B:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // Value dependent case expression
+  File,
+  R""(
+enum class Enum {A,B};
+template
+void function() {
+^switch (Enum::A) {case Value:break;}
+}
+  )"",
   "unavailable",
   },
   {
@@ -2867,9 +2906,53 @@
   {
   // Scoped enumeration with multiple enumerators
   Function,
-  R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
-  R""(enum class Enum {A,B}; )""
-  R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {}
+  )"",
+  R""(
+enum class Enum {A,B};
+switch (Enum::A) {case Enum::A:case Enum::B:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators (unscoped)
+  Function,
+  R""(
+enum Enum {A,B,C};
+^switch (A) {case B:break;}
+  )"",
+  R""(
+enum Enum {A,B,C};
+switch (A) {case B:break;case A:case C:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators,
+  // even when using integer literals
+  Function,
+  R""(
+enum Enum {A,B=1,C};
+^switch (A) {case 1:break;}
+  )"",
+  R""(
+enum Enum {A,B=1,C};
+switch (A) {case 1:break;case A:case C:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators (scoped)
+  Function,
+  R""(
+enum class Enum {A,B,C};
+^switch (Enum::A)
+{case Enum::B:break;}
+  )"",
+  R""(
+enum class Enum {A,B,C};
+switch (Enum::A)
+{case Enum::B:break;case Enum::A:case Enum::C:break;}
+  )"",
   },
   {
   // Scoped enumerations in namespace
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -33,12 +33,15 @@
 #include "AST.h"
 #include "Selection.h"
 #include "refactor/Tweak.h"
+#include "support/Logger.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/SmallSet.h"
+#include 
 #include 
 
 namespace clang {
@@ -52,18 +55,16 @@
   Intent intent() const override { return Refactor; }
 
 private:
-  ASTContext *ASTCtx = nullptr;
   const DeclContext *DeclCtx = nullptr;
   const SwitchStmt *Switch = nullptr;
   const CompoundStmt *Body = nullptr;
+  const EnumType *EnumT = nullptr;
   const EnumDecl *EnumD = nullptr;
 };
 
 REGISTER_TWEAK(PopulateSwitch)
 
 bool PopulateSwitch::prepare(const Selection ) {
-  ASTCtx = >getASTContext();
-
   const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
   if (!CA)
 

[PATCH] D88489: [clangd] Mark code action as "preferred" if it's the sole quickfix action

2020-09-29 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: usaxena95.
Herald added subscribers: cfe-commits, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88489

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test


Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
+# CHECK-NEXT:"isPreferred": true,
 # CHECK-NEXT:"kind": "quickfix",
 # CHECK-NEXT:"title": "change 'union' to 'struct'"
 # CHECK-NEXT:  }
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -952,6 +952,13 @@
   /// The diagnostics that this code action resolves.
   llvm::Optional> diagnostics;
 
+  /// Marks this as a preferred action. Preferred actions are used by the
+  /// `auto fix` command and can be targeted by keybindings.
+  /// A quick fix should be marked preferred if it properly addresses the
+  /// underlying error. A refactoring should be marked preferred if it is the
+  /// most reasonable choice of actions to take.
+  bool isPreferred = false;
+
   /// The workspace edit this code action performs.
   llvm::Optional edit;
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -740,6 +740,8 @@
 CodeAction["kind"] = *CA.kind;
   if (CA.diagnostics)
 CodeAction["diagnostics"] = llvm::json::Array(*CA.diagnostics);
+  if (CA.isPreferred)
+CodeAction["isPreferred"] = true;
   if (CA.edit)
 CodeAction["edit"] = *CA.edit;
   if (CA.command)
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -411,6 +411,8 @@
 Main.codeActions.emplace();
 for (const auto  : D.Fixes)
   Main.codeActions->push_back(toCodeAction(Fix, File));
+if (Main.codeActions->size() == 1)
+  Main.codeActions->front().isPreferred = true;
   }
   if (Opts.SendDiagnosticCategory && !D.Category.empty())
 Main.category = D.Category;
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1009,6 +1009,20 @@
 for (const auto  : *Tweaks)
   Actions.push_back(toCodeAction(T, File, Selection));
 
+// If there's exactly one quick-fix, call it "preferred".
+// We never consider refactorings etc as preferred.
+CodeAction *OnlyFix = nullptr;
+for (auto  : Actions) {
+  if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) {
+if (OnlyFix) {
+  OnlyFix->isPreferred = false;
+  break;
+}
+Action.isPreferred = true;
+OnlyFix = 
+  }
+}
+
 if (SupportsCodeAction)
   return Reply(llvm::json::Array(Actions));
 std::vector Commands;


Index: clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
===
--- clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
+++ clang-tools-extra/clangd/test/fixits-embed-in-diagnostic.test
@@ -28,6 +28,7 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
+# CHECK-NEXT:"isPreferred": true,
 # CHECK-NEXT:"kind": "quickfix",
 # CHECK-NEXT:"title": "change 'union' to 'struct'"
 # CHECK-NEXT:  }
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -952,6 +952,13 @@
   /// The diagnostics that this code action resolves.
   llvm::Optional> diagnostics;
 
+  /// Marks this as a preferred action. Preferred actions are used by the
+  /// `auto fix` command and can be targeted by keybindings.
+  /// A quick fix should be marked preferred if it properly addresses the
+  /// underlying error. A refactoring should be marked preferred if it is the
+ 

  1   2   3   4   5   6   7   8   9   10   >