[clang-tools-extra] [clangd] Check for editsNearCursor client capability under experimental capabilities (PR #114699)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/114699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Check for editsNearCursor client capability under experimental capabilities (PR #114699)
HighCommander4 wrote: > (No action required) – we have other extensions in clangd e.g. > `references.container` `offsetEncoding`. Do we plan to do the same thing for > them? Good question; I was initially thinking of doing it as needed / when someone asks for it. But maybe it would be better to add them all up front, so when a need for another one arises, the support is already in a clangd release. I can do that in a follow-up patch. https://github.com/llvm/llvm-project/pull/114699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] fix extract-to-function for overloaded operators (PR #81640)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/81640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] fix extract-to-function for overloaded operators (PR #81640)
@@ -190,6 +190,14 @@ F (extracted();)
}]]
)cpp";
EXPECT_EQ(apply(CompoundFailInput), "unavailable");
+
+ ExtraArgs.push_back("-std=c++14");
+ // FIXME: Expressions are currently not extracted
+ EXPECT_EQ(apply(R"cpp(
+void sink(int);
+void call() { sink([[1+1]]); }
HighCommander4 wrote:
I was thinking more specifically about entire expression-statements, e.g.
`[[stream << 42;]]`.
Extraction of arbitrary subexpressions would be nice as well, but likely more
involved; there is a work in progress [here](https://reviews.llvm.org/D140619).
(We can keep this test case too.)
https://github.com/llvm/llvm-project/pull/81640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] fix extract-to-function for overloaded operators (PR #81640)
@@ -104,9 +104,12 @@ bool isRootStmt(const Node *N) {
// Root statement cannot be partially selected.
if (N->Selected == SelectionTree::Partial)
return false;
- // Only DeclStmt can be an unselected RootStmt since VarDecls claim the
entire
- // selection range in selectionTree.
- if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get())
+ // A DeclStmt can be an unselected RootStmt since VarDecls claim the entire
+ // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a
+ // binary operation can be unselected because it's children claim the entire
+ // selection range in the selection tree (e.g. <<).
+ if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get()
&&
HighCommander4 wrote:
Do we need to make a corresponding change
[here](https://searchfox.org/llvm/rev/30213e99b86a078c9d472264c7edeea9aa638dd4/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp#141-146)?
The comment suggests that it's special-casing `DeclStmt` for the same reason
as this code.
https://github.com/llvm/llvm-project/pull/81640
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Check for editsNearCursor client capability under experimental capabilities (PR #114699)
https://github.com/HighCommander4 created
https://github.com/llvm/llvm-project/pull/114699
This is done to support clients which only support adding custom
(language-specific or server-specific) capabilities under 'experimental'.
Fixes https://github.com/clangd/clangd/issues/2201
>From 52a5625e0806c2a70b6ca728f9f3e42d88b83021 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Sun, 3 Nov 2024 02:42:46 -0500
Subject: [PATCH] [clangd] Check for editsNearCursor client capability under
experimental capabilities
This is done to support clients which only support adding custom
(language-specific or server-specific) capabilities under
'experimental'.
Fixes https://github.com/clangd/clangd/issues/2201
---
clang-tools-extra/clangd/Protocol.cpp | 10 ++
1 file changed, 10 insertions(+)
diff --git a/clang-tools-extra/clangd/Protocol.cpp
b/clang-tools-extra/clangd/Protocol.cpp
index c08f80442eaa06..5a303123b5ce84 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -504,6 +504,16 @@ bool fromJSON(const llvm::json::Value &Params,
ClientCapabilities &R,
P.field("offsetEncoding")))
return false;
}
+
+ if (auto *Experimental = O->getObject("experimental")) {
+if (auto *TextDocument = Experimental->getObject("textDocument")) {
+ if (auto *Completion = TextDocument->getObject("completion")) {
+if (auto EditsNearCursor = Completion->getBoolean("editsNearCursor"))
+ R.CompletionFixes |= *EditsNearCursor;
+ }
+}
+ }
+
return true;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Extend call hierarchy for field and non-local variables (PR #113900)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/113900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Extend call hierarchy for field and non-local variables (PR #113900)
https://github.com/HighCommander4 approved this pull request. https://github.com/llvm/llvm-project/pull/113900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Extend call hierarchy for field and non-local variables (PR #113900)
@@ -2238,7 +2238,10 @@ prepareCallHierarchy(ParsedAST &AST, Position Pos,
PathRef TUPath) {
for (const NamedDecl *Decl : getDeclAtPosition(AST, *Loc, {})) {
if (!(isa(Decl) &&
cast(Decl)->isFunctionOrMethod()) &&
-Decl->getKind() != Decl::Kind::FunctionTemplate)
+!(Decl->getKind() == Decl::Kind::Var &&
+ !cast(Decl)->isLocalVarDecl()) &&
+Decl->getKind() != Decl::Kind::FunctionTemplate &&
HighCommander4 wrote:
nit: can can you keep the function template check next to the function check,
since they are related?
https://github.com/llvm/llvm-project/pull/113900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Extend call hierarchy for field and non-local variables (PR #113900)
https://github.com/HighCommander4 requested changes to this pull request. Thanks for the patch! https://github.com/llvm/llvm-project/pull/113900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Extend call hierarchy for field and non-local variables (PR #113900)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/113900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] fix extract-to-function for overloaded operators (PR #81640)
HighCommander4 wrote: Do I understand correctly that this is a partial fix for https://github.com/clangd/clangd/issues/1254 in that it addresses the issue with overloaded operators in particular, but it still leaves in place the limitation that a **single** expression statement (of any kind) cannot be extracted? If so, would you mind adding an "unavailable" test case for the latter case, with some sort of "TODO, we'd like to relax this limitation" comment? https://github.com/llvm/llvm-project/pull/81640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (PR #111616)
@@ -2292,9 +2289,26 @@ incomingCalls(const CallHierarchyItem &Item, const
SymbolIndex *Index) {
Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
auto It = CallsIn.find(Caller.ID);
assert(It != CallsIn.end());
-if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
+if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
+ SymbolLocation CallerLoc =
+ Caller.Definition ? Caller.Definition : Caller.CanonicalDeclaration;
+ std::vector FromRanges;
+ for (const SymbolLocation &L : It->second) {
+if (StringRef{L.FileURI} != StringRef{CallerLoc.FileURI}) {
+ elog("incomingCalls: call location not in same file as caller, this "
HighCommander4 wrote:
Thanks for the example! This is exactly the sort of case the hardening was
intended to catch, though I didn't think of it.
The case raises an interesting question: what **should** our behaviour be for
an example like this, where the function definition is spread across multiple
files?
The [protocol
representation](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#callHierarchy_incomingCalls)
only allows specifying one file per caller:
```ts
export interface CallHierarchyIncomingCall {
/**
* The item that makes the call.
*/
from: CallHierarchyItem;
/**
* The ranges at which the calls appear. This is relative to the caller
* denoted by [`this.from`](#CallHierarchyIncomingCall.from).
*/
fromRanges: Range[];
}
```
Here, `from.uri` is the only thing that encodes a file; the ranges in
`fromRanges` encode a pair of line/column positions only, and are assumed to be
in the same file as `from.uri`.
https://github.com/llvm/llvm-project/pull/111616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (PR #111616)
@@ -2272,18 +2273,14 @@ incomingCalls(const CallHierarchyItem &Item, const
SymbolIndex *Index) {
// Initially store the ranges in a map keyed by SymbolID of the caller.
// This allows us to group different calls with the same caller
// into the same CallHierarchyIncomingCall.
- llvm::DenseMap> CallsIn;
+ llvm::DenseMap> CallsIn;
// We can populate the ranges based on a refs request only. As we do so, we
// also accumulate the container IDs into a lookup request.
LookupRequest ContainerLookup;
Index->refs(Request, [&](const Ref &R) {
-auto Loc = indexToLSPLocation(R.Location, Item.uri.file());
-if (!Loc) {
- elog("incomingCalls failed to convert location: {0}", Loc.takeError());
- return;
-}
-auto It = CallsIn.try_emplace(R.Container, std::vector{}).first;
-It->second.push_back(Loc->range);
+auto It =
+CallsIn.try_emplace(R.Container, std::vector{}).first;
+It->second.push_back(R.Location);
HighCommander4 wrote:
Good catch, thanks. I figured `FileURI` would point to storage inside the ref
slab which would stay alive for the lifetime of the index, but I do see now
that the [comment
above](https://searchfox.org/llvm/rev/91c11574e87b5c0f434688edac01e9580ef99a92/clang-tools-extra/clangd/index/Index.h#137)
of `SymbolIndex::refs()` says "The returned result must be deep-copied if it's
used outside Callback", so while it may work for some index implementations we
can't rely on it.
https://github.com/llvm/llvm-project/pull/111616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
HighCommander4 wrote: Thanks. The buildkite failure looks unrelated (it's a `clang-move` test, and this patch only touches `clangd` code), so I will go ahead and merge this. https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak handle member function templates (PR #112345)
HighCommander4 wrote: It looks like it was a deliberate design choice to disable this tweak for templates: https://reviews.llvm.org/D85310. cc @kadircet, @hokein for any thoughts https://github.com/llvm/llvm-project/pull/112345 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
@@ -1497,6 +1497,47 @@ TEST(DefaultArguments, Smoke) {
ExpectedHint{"A: ", "explicit", Left});
}
+TEST(DefaultArguments, WithoutParameterNames) {
+ Config Cfg;
+ Cfg.InlayHints.Parameters = false; // To test just default args this time
+ Cfg.InlayHints.DeducedTypes = false;
+ Cfg.InlayHints.Designators = false;
+ Cfg.InlayHints.BlockEnd = false;
+
+ Cfg.InlayHints.DefaultArguments = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ const auto *Code = R"cpp(
+struct Baz {
+ Baz(float a = 3 //
++ 2);
+};
+struct Foo {
+ Foo(int, Baz baz = //
+ Baz{$unnamed[[}]]
HighCommander4 wrote:
nit: unnamed --> abbreviated? (the parameter itself is named here)
https://github.com/llvm/llvm-project/pull/95712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
https://github.com/HighCommander4 approved this pull request. Thanks for the update and for spotting and fixing the added issues. I agree that keeping `DefaultArguments` orthogonal to `Parameters` is a good choice. https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
HighCommander4 wrote: >3. I was getting `Property DefaultArguments` is not allowed in > `config.yaml`. Is this a schema issue? I wasn't able to find where to update > this That sort of diagnostic is likely produced by a YAML plugin, which uses a schema from https://github.com/SchemaStore/schemastore. I do file a SchemaStore issue once per clangd release about updating the clangd schema, here's the one for clangd 19: https://github.com/SchemaStore/schemastore/issues/4022. If you'd like to file one about `DefaultArguments` sooner, or even submit a PR, please feel free. https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak handle member functions (PR #95235)
HighCommander4 wrote: @5chmidti since you've already looked at this, I'd be happy for you to approve this if you're comfortable doing so. https://github.com/llvm/llvm-project/pull/95235 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
@@ -1681,6 +1681,15 @@ enum class InlayHintKind {
/// This is a clangd extension.
BlockEnd = 4,
+ /// An inlay hint that is for a default argument.
+ ///
+ /// An example of a parameter hint for a default argument:
+ ///void foo(bool A = true);
+ ///foo(^);
+ /// Adds an inlay hint "A = true".
HighCommander4 wrote:
Update comment to `A: true`
https://github.com/llvm/llvm-project/pull/95712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
@@ -1465,6 +1469,34 @@ TEST(TypeHints, DefaultTemplateArgs) {
ExpectedHint{": A", "binding"});
}
+TEST(DefaultArguments, Smoke) {
+ Config Cfg;
+ Cfg.InlayHints.Parameters =
+ true; // To test interplay of parameters and default parameters
+ Cfg.InlayHints.DeducedTypes = false;
+ Cfg.InlayHints.Designators = false;
+ Cfg.InlayHints.BlockEnd = false;
+
+ Cfg.InlayHints.DefaultArguments = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ const auto *Code = R"cpp(
+int foo(int A = 4) { return A; }
+int bar(int A, int B = 1, bool C = foo($default1[[)]]) { return A; }
+int A = bar($explicit[[2]]$default2[[)]];
+
+void baz(int = 5) { if (false) baz($unnamed[[)]]; };
+ )cpp";
+
+ assertHints(InlayHintKind::DefaultArgument, Code,
HighCommander4 wrote:
It would be nice to have one check where the token the hint is attached to is
an R-brace rather than an R-paren
https://github.com/llvm/llvm-project/pull/95712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
@@ -372,6 +381,25 @@ maybeDropCxxExplicitObjectParameters(ArrayRef Params) {
return Params;
}
+template
+std::string joinAndTruncate(R &&Range, size_t MaxLength,
HighCommander4 wrote:
Now that the function has only one call site, can we drop the
`GetAsStringFunction` parameter and just assume the elements are themselves
strings?
https://github.com/llvm/llvm-project/pull/95712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
@@ -709,7 +738,8 @@ class InlayHintVisitor : public
RecursiveASTVisitor {
private:
using NameVec = SmallVector;
- void processCall(Callee Callee, llvm::ArrayRef Args) {
+ void processCall(Callee Callee, SourceRange RParenOrBraceRange,
HighCommander4 wrote:
I think it would be clearer for the `SourceRange RParenOrBraceRange` parameter
to be `SourceLocation RParenOrBraceLoc` instead.
Both call sites in fact pass a `SourceLocation`, which currently gets
implicitly converted to a singleton range.
I'd rather we do that conversion inside the function's implementation, and make
it explicit with `SourceRange{RParenOrBraceLoc}`.
https://github.com/llvm/llvm-project/pull/95712
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add inlay hints for default function arguments (PR #95712)
https://github.com/HighCommander4 commented: Thanks for the update! The patch is looking pretty good, just some minor comments remaining and then it should be good to go. https://github.com/llvm/llvm-project/pull/95712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (PR #111616)
@@ -2292,9 +2289,26 @@ incomingCalls(const CallHierarchyItem &Item, const
SymbolIndex *Index) {
Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
auto It = CallsIn.find(Caller.ID);
assert(It != CallsIn.end());
-if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
+if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
+ SymbolLocation CallerLoc =
+ Caller.Definition ? Caller.Definition : Caller.CanonicalDeclaration;
+ std::vector FromRanges;
+ for (const SymbolLocation &L : It->second) {
+if (StringRef{L.FileURI} != StringRef{CallerLoc.FileURI}) {
+ elog("incomingCalls: call location not in same file as caller, this "
HighCommander4 wrote:
Should we assert as well?
https://github.com/llvm/llvm-project/pull/111616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (PR #111616)
@@ -2292,9 +2289,26 @@ incomingCalls(const CallHierarchyItem &Item, const
SymbolIndex *Index) {
Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
auto It = CallsIn.find(Caller.ID);
assert(It != CallsIn.end());
-if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
+if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
+ SymbolLocation CallerLoc =
+ Caller.Definition ? Caller.Definition : Caller.CanonicalDeclaration;
+ std::vector FromRanges;
+ for (const SymbolLocation &L : It->second) {
+if (StringRef{L.FileURI} != StringRef{CallerLoc.FileURI}) {
+ elog("incomingCalls: call location not in same file as caller, this "
+ "is unexpected");
+ continue;
+}
+Range R;
HighCommander4 wrote:
Note: I didn't use `indexToLSPLocation()` here because the `URI::resolve`
operation that it does seems like unnecessary work here. I could factor out the
part that converts the range into a function to avoid duplicating it here.
https://github.com/llvm/llvm-project/pull/111616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Harden incomingCalls() against possible misinterpretation of a range as pertaining to the wrong file (PR #111616)
https://github.com/HighCommander4 created
https://github.com/llvm/llvm-project/pull/111616
I don't have a concrete motivating scenario here, just something I noticed
during code reading:
`CallHierarchyIncomingCall::fromRanges` are interpreted as ranges in the same
file as the `CallHierarchyItem` representing the caller
(`CallHierarchyIncomingCall::from`).
In the server implementation, we populate them based on `Ref::Location`, taking
only the range and discarding the file, and associate them with a
`CallHierarchyItem` representing `Ref::Container`.
Now, logically it **should** be the case that the definition location of the
symbol referred to by `Ref::Container` is in the same file as the `Ref`
itself... but I don't think anything guarantees this, and if for some reason
this doesn't hold, we are effectively taking ranges from one file and
interpreting them as being in another file.
The patch adds a check for this and discards ranges which are not in fact in
the same file.
>From 88eed085ab89e53a4fe2bd66bbf108ed2c0f64a0 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 8 Oct 2024 21:43:55 -0400
Subject: [PATCH] [clangd] Harden incomingCalls() against possible
misinterpretation of a range as pertaining to the wrong file
---
clang-tools-extra/clangd/XRefs.cpp | 34 +-
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clangd/XRefs.cpp
b/clang-tools-extra/clangd/XRefs.cpp
index f94cadeffaa298..cca5035fb74bd4 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -63,6 +63,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
@@ -2272,18 +2273,14 @@ incomingCalls(const CallHierarchyItem &Item, const
SymbolIndex *Index) {
// Initially store the ranges in a map keyed by SymbolID of the caller.
// This allows us to group different calls with the same caller
// into the same CallHierarchyIncomingCall.
- llvm::DenseMap> CallsIn;
+ llvm::DenseMap> CallsIn;
// We can populate the ranges based on a refs request only. As we do so, we
// also accumulate the container IDs into a lookup request.
LookupRequest ContainerLookup;
Index->refs(Request, [&](const Ref &R) {
-auto Loc = indexToLSPLocation(R.Location, Item.uri.file());
-if (!Loc) {
- elog("incomingCalls failed to convert location: {0}", Loc.takeError());
- return;
-}
-auto It = CallsIn.try_emplace(R.Container, std::vector{}).first;
-It->second.push_back(Loc->range);
+auto It =
+CallsIn.try_emplace(R.Container, std::vector{}).first;
+It->second.push_back(R.Location);
ContainerLookup.IDs.insert(R.Container);
});
@@ -2292,9 +2289,26 @@ incomingCalls(const CallHierarchyItem &Item, const
SymbolIndex *Index) {
Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
auto It = CallsIn.find(Caller.ID);
assert(It != CallsIn.end());
-if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
+if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
+ SymbolLocation CallerLoc =
+ Caller.Definition ? Caller.Definition : Caller.CanonicalDeclaration;
+ std::vector FromRanges;
+ for (const SymbolLocation &L : It->second) {
+if (StringRef{L.FileURI} != StringRef{CallerLoc.FileURI}) {
+ elog("incomingCalls: call location not in same file as caller, this "
+ "is unexpected");
+ continue;
+}
+Range R;
+R.start.line = L.Start.line();
+R.start.character = L.Start.column();
+R.end.line = L.End.line();
+R.end.character = L.End.column();
+FromRanges.push_back(R);
+ }
Results.push_back(
- CallHierarchyIncomingCall{std::move(*CHI), std::move(It->second)});
+ CallHierarchyIncomingCall{std::move(*CHI), std::move(FromRanges)});
+}
});
// Sort results by name of container.
llvm::sort(Results, [](const CallHierarchyIncomingCall &A,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add ArgumentLists config option under Completion (PR #111322)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/111322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add ArgumentLists config option under Completion (PR #111322)
HighCommander4 wrote: This was originally submitted by @MK-Alias and reviewed by me at https://github.com/llvm/llvm-project/pull/108005. The only changes I've made relative to #108005 is to remove unrelated formatting changes (per my last outstanding review comment there) and reword the commit message. I will therefore go ahead and merge this without additional review. https://github.com/llvm/llvm-project/pull/111322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: I've resubmitted this as https://github.com/llvm/llvm-project/pull/111322. https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add ArgumentLists config option under Completion (PR #111322)
https://github.com/HighCommander4 created
https://github.com/llvm/llvm-project/pull/111322
The new config option is a more flexible version of
--function-arg-placeholders, allowing users more detailed control of what is
inserted in argument list position when clangd completes the name of a function
in a function call context.
Fixes https://github.com/llvm/llvm-project/issues/63565
>From b0c222e3a67c6ec320ae3582d8034c5562e8fe94 Mon Sep 17 00:00:00 2001
From: MK-Alias
Date: Sun, 6 Oct 2024 20:19:55 -0400
Subject: [PATCH] [clangd] Add ArgumentLists config option under Completion
The new config option is a more flexible version of
--function-arg-placeholders, allowing users more detailed control
of what is inserted in argument list position when clangd completes
the name of a function in a function call context.
Fixes https://github.com/llvm/llvm-project/issues/63565
---
clang-tools-extra/clangd/ClangdServer.cpp | 1 +
clang-tools-extra/clangd/CodeComplete.cpp | 26 +--
clang-tools-extra/clangd/CodeComplete.h | 9 ---
clang-tools-extra/clangd/Config.h | 14 ++
clang-tools-extra/clangd/ConfigCompile.cpp| 15 +++
clang-tools-extra/clangd/ConfigFragment.h | 8 ++
clang-tools-extra/clangd/ConfigYAML.cpp | 4 +++
clang-tools-extra/clangd/tool/ClangdMain.cpp | 18 +
.../clangd/unittests/CodeCompleteTests.cpp| 25 --
9 files changed, 101 insertions(+), 19 deletions(-)
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp
b/clang-tools-extra/clangd/ClangdServer.cpp
index e910a80ba0bae9..9b38be04e7ddd7 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -451,6 +451,7 @@ void ClangdServer::codeComplete(PathRef File, Position Pos,
CodeCompleteOpts.MainFileSignals = IP->Signals;
CodeCompleteOpts.AllScopes = Config::current().Completion.AllScopes;
+CodeCompleteOpts.ArgumentLists =
Config::current().Completion.ArgumentLists;
// FIXME(ibiryukov): even if Preamble is non-null, we may want to check
// both the old and the new version in case only one of them matches.
CodeCompleteResult Result = clangd::codeComplete(
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp
b/clang-tools-extra/clangd/CodeComplete.cpp
index 89eee392837af4..adca462b53f0a0 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -21,6 +21,7 @@
#include "AST.h"
#include "CodeCompletionStrings.h"
#include "Compiler.h"
+#include "Config.h"
#include "ExpectedTypes.h"
#include "Feature.h"
#include "FileDistance.h"
@@ -350,8 +351,7 @@ struct CodeCompletionBuilder {
CodeCompletionContext::Kind ContextKind,
const CodeCompleteOptions &Opts,
bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
- : ASTCtx(ASTCtx),
-EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
+ : ASTCtx(ASTCtx), ArgumentLists(Opts.ArgumentLists),
IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
Completion.Deprecated = true; // cleared by any non-deprecated overload.
add(C, SemaCCS, ContextKind);
@@ -561,6 +561,15 @@ struct CodeCompletionBuilder {
}
std::string summarizeSnippet() const {
+/// localize ArgumentLists tests for better readability
+const bool None = ArgumentLists == Config::ArgumentListsPolicy::None;
+const bool Open =
+ArgumentLists == Config::ArgumentListsPolicy::OpenDelimiter;
+const bool Delim = ArgumentLists ==
Config::ArgumentListsPolicy::Delimiters;
+const bool Full =
+ArgumentLists == Config::ArgumentListsPolicy::FullPlaceholders ||
+(!None && !Open && !Delim); // <-- failsafe: Full is default
+
if (IsUsingDeclaration)
return "";
auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
@@ -568,7 +577,7 @@ struct CodeCompletionBuilder {
// All bundles are function calls.
// FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
// we need to complete 'forward<$1>($0)'.
- return "($0)";
+ return None ? "" : (Open ? "(" : "($0)");
if (Snippet->empty())
return "";
@@ -607,7 +616,7 @@ struct CodeCompletionBuilder {
return "";
}
}
-if (EnableFunctionArgSnippets)
+if (Full)
return *Snippet;
// Replace argument snippets with a simplified pattern.
@@ -622,9 +631,9 @@ struct CodeCompletionBuilder {
bool EmptyArgs = llvm::StringRef(*Snippet).ends_with("()");
if (Snippet->front() == '<')
-return EmptyArgs ? "<$1>()$0" : "<$1>($0)";
+return None ? "" : (Open ? "<" : (EmptyArgs ? "<$1>()$0" :
"<$1>($0)"));
if (Snippet->front() == '(')
-return EmptyArgs ? "()" : "($0)";
+return None ? "" : (Open ? "(" : (EmptyArgs ? "()" : "($0)"));
return
[clang-tools-extra] [clang-tidy] Avoid capturing a local variable in a static lambda in UseRangesCheck (PR #111282)
HighCommander4 wrote: Also requested backport to 19.x in https://github.com/llvm/llvm-project/issues/111317. https://github.com/llvm/llvm-project/pull/111282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Avoid capturing a local variable in a static lambda in UseRangesCheck (PR #111282)
HighCommander4 wrote: > For this specific case, where a static lambda captures a local variable, I > think we could enhance Clang to detect this kind of use-after-free bug. Yep, good idea. I filed https://github.com/llvm/llvm-project/issues/111316 about this. https://github.com/llvm/llvm-project/pull/111282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Avoid capturing a local variable in a static lambda in UseRangesCheck (PR #111282)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/111282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Simplify ternary expressions with std::optional::value_or (NFC) (PR #111309)
https://github.com/HighCommander4 approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/111309 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add checks to convert std library iterator algorithms into c++20 or boost ranges (PR #97764)
HighCommander4 wrote: > It crashes most likely because a local variable is captured in the static > lambda. > > https://github.com/llvm/llvm-project/blob/bf895c714e1f8a51c1e565a75acf60bf7197be51/clang-tools-extra/clang-tidy/boost/UseRangesCheck.cpp#L208 Nice find! That does seem to be the problem. I've put up a fix at https://github.com/llvm/llvm-project/pull/111282. https://github.com/llvm/llvm-project/pull/97764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Avoid capturing a local variable in a static lambda in UseRangesCheck (PR #111282)
https://github.com/HighCommander4 created
https://github.com/llvm/llvm-project/pull/111282
Fixes https://github.com/llvm/llvm-project/issues/109367
>From d7ec29dc8852c4ae8b239daff11acc42caf4d544 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Sun, 6 Oct 2024 01:45:35 -0400
Subject: [PATCH] [clang-tidy] Avoid capturing a local variable in a static
lambda in UseRangesCheck
Fixes https://github.com/llvm/llvm-project/issues/109367
---
.../clang-tidy/boost/UseRangesCheck.cpp| 18 +-
.../clangd/unittests/ClangdLSPServerTests.cpp | 16
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/boost/UseRangesCheck.cpp
b/clang-tools-extra/clang-tidy/boost/UseRangesCheck.cpp
index 4022ea0cdaf5ee..e45687fde6d9f6 100644
--- a/clang-tools-extra/clang-tidy/boost/UseRangesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/boost/UseRangesCheck.cpp
@@ -204,7 +204,7 @@ utils::UseRangesCheck::ReplacerMap
UseRangesCheck::getReplacerMap() const {
ReplacerMap Results;
static const Signature SingleSig = {{0}};
static const Signature TwoSig = {{0}, {2}};
- static const auto AddFrom =
+ const auto AddFrom =
[&Results](llvm::IntrusiveRefCntPtr Replacer,
std::initializer_list Names, StringRef Prefix) {
llvm::SmallString<64> Buffer;
@@ -214,17 +214,17 @@ utils::UseRangesCheck::ReplacerMap
UseRangesCheck::getReplacerMap() const {
}
};
- static const auto AddFromStd =
- [](llvm::IntrusiveRefCntPtr Replacer,
- std::initializer_list Names) {
+ const auto AddFromStd =
+ [&](llvm::IntrusiveRefCntPtr Replacer,
+ std::initializer_list Names) {
AddFrom(Replacer, Names, "std");
};
- static const auto AddFromBoost =
- [](llvm::IntrusiveRefCntPtr Replacer,
- std::initializer_list<
- std::pair>>
- NamespaceAndNames) {
+ const auto AddFromBoost =
+ [&](llvm::IntrusiveRefCntPtr Replacer,
+ std::initializer_list<
+ std::pair>>
+ NamespaceAndNames) {
for (auto [Namespace, Names] : NamespaceAndNames)
AddFrom(Replacer, Names,
SmallString<64>{"boost", (Namespace.empty() ? "" : "::"),
diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index 75a140767035b2..49a94045ea4878 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -262,6 +262,22 @@ TEST_F(LSPTest, ClangTidyRename) {
EXPECT_EQ(Params, std::vector{llvm::json::Value(std::move(ExpectedEdit))});
}
+TEST_F(LSPTest, ClangTidyCrash_Issue109367) {
+ // This test requires clang-tidy checks to be linked in.
+ if (!CLANGD_TIDY_CHECKS)
+return;
+ Opts.ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
+ llvm::StringRef) {
+ClangTidyOpts.Checks = {"-*,boost-use-ranges"};
+ };
+ // Check that registering the boost-use-ranges checker's matchers
+ // on two different threads does not cause a crash.
+ auto &Client = start();
+ Client.didOpen("a.cpp", "");
+ Client.didOpen("b.cpp", "");
+ Client.sync();
+}
+
TEST_F(LSPTest, IncomingCalls) {
Annotations Code(R"cpp(
void calle^e(int);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add checks to convert std library iterator algorithms into c++20 or boost ranges (PR #97764)
HighCommander4 wrote: FYI, the new `boost-use-ranges` check is pretty crashy in clangd (https://github.com/llvm/llvm-project/issues/109037, https://github.com/llvm/llvm-project/issues/109367, https://github.com/clangd/clangd/issues/2173, https://github.com/clangd/clangd/issues/2151). https://github.com/llvm/llvm-project/pull/97764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: > > but it confuses things when "git blame" says that the last commit that > > touched some unrelated > > And it's not confusing if "git blame" says that this same part of code is > there in a "clang-format commit". Instead of it's original? I would say that's less confusing, because you know you can just skip that sort of commit in the blame. I'm not trying to be difficult here; "please remove unrelated formatting changes" is something I've been told by other reviewers in response to my own patches. https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
https://github.com/HighCommander4 requested changes to this pull request. Thanks, this is looking pretty good. My only remaining request is to please split the formatting changes out. I know it's the fault of the code not being clang-format clean, and I'm happy to merge them in a separate PR, but it confuses things when "git blame" says that the last commit that touched some unrelated test / code line is this one about `ArgumentLists`. https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -230,6 +230,10 @@ class Parser {
if (auto AllScopes = boolValue(N, "AllScopes"))
F.AllScopes = *AllScopes;
});
+Dict.handle("ArgumentLists", [&](Node &N) {
+ if (auto ArgumentLists = scalarValue(N, "ArgumentLists"))
HighCommander4 wrote:
(this comment remains to be addressed)
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: > `FlagConfigProvider` is pushed in the vector after `.clangd` config in > [CLangdMain.cpp](https://github.com/llvm/llvm-project/blob/a4586bd2d4fa7d6c0100893496a9383fd581e2e9/clang-tools-extra/clangd/tool/ClangdMain.cpp#L926) > Then > [ConfigProvider.cpp](https://github.com/llvm/llvm-project/blob/5ac97d397c2088c3ac0a113506e57ab9b1e69ac8/clang-tools-extra/clangd/ConfigProvider.cpp#L154) > just iterates over it, which means that `FlagConfigProvider` is evaluated > **after** `.clangd` and will override it! Thanks for catching that; I was under the mistaken impression that it was the other way around! I think it would be a nicer user experience if the config file took precedence over compile flags... but since that's not the case today for existing flags (like `--background-index`), I think it's appropriate for this patch to remain consistent with the existing behaviour. In other words, I think what the current version of the patch is doing is fine. https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Collect comments from function definitions into the index (PR #67802)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/67802 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Collect comments from function definitions into the index (PR #67802)
https://github.com/HighCommander4 approved this pull request. Thanks! The patch looks good to me. And the index size measurements reported in [this comment](https://github.com/llvm/llvm-project/pull/67802#issuecomment-1923778262) look good as well, thank you for taking them. https://github.com/llvm/llvm-project/pull/67802 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Collect comments from function definitions into the index (PR #67802)
https://github.com/HighCommander4 requested changes to this pull request. https://github.com/llvm/llvm-project/pull/67802 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Collect comments from function definitions into the index (PR #67802)
@@ -451,8 +451,17 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
if (LastCheckedRedecl) {
if (LastCheckedRedecl == Redecl) {
LastCheckedRedecl = nullptr;
+continue;
HighCommander4 wrote:
The fix for #108145 has merged now, so let's go ahead and drop this hunk
https://github.com/llvm/llvm-project/pull/67802
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
https://github.com/HighCommander4 closed https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
HighCommander4 wrote: Added release note. I put it under "Bug Fixes to AST Handling" which seemed like a good fit. https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/108475
>From 1df68534d086e20572f2371239826d6b3514e58b Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 10 Sep 2024 22:34:55 -0400
Subject: [PATCH] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any
redecl with a comment
The previous implementation had a bug where, if it was called on a
Decl later in the redecl chain than `LastCheckedDecl`, it could
incorrectly skip and overlook a Decl with a comment.
The patch addresses this by only using `LastCheckedDecl` if the input
Decl `D` is on the path from the first (canonical) Decl to
`LastCheckedDecl`.
An alternative that was considered was to start the iteration from
the (canonical) Decl, however this ran into problems with the
modelling of explicit template specializations in the AST where
the canonical Decl can be unusual. With the current solution, if no
Decls were checked yet, we prefer to check the input Decl over the
canonical one.
---
clang/docs/ReleaseNotes.rst | 2 +
clang/lib/AST/ASTContext.cpp | 22 -
clang/unittests/AST/CMakeLists.txt| 1 +
clang/unittests/AST/RawCommentForDeclTest.cpp | 98 +++
4 files changed, 119 insertions(+), 4 deletions(-)
create mode 100644 clang/unittests/AST/RawCommentForDeclTest.cpp
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b8816d7b555e87..3f146cb9247a78 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -408,6 +408,8 @@ Bug Fixes to AST Handling
^
- Fixed a crash that occurred when dividing by zero in complex integer
division. (#GH55390).
+- Fixed a bug in ``ASTContext::getRawCommentForAnyRedecl()`` where the
function could
+ sometimes incorrectly return null even if a comment was present. (#GH108145)
Miscellaneous Bug Fixes
^^^
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index ebd4a41ee6367a..85b3984940ffc2 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -440,12 +440,26 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
}
// Any redeclarations of D that we haven't checked for comments yet?
- // We can't use DenseMap::iterator directly since it'd get invalid.
- auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-return CommentlessRedeclChains.lookup(CanonicalD);
+ const Decl *LastCheckedRedecl = [&]() {
+const Decl *LastChecked = CommentlessRedeclChains.lookup(CanonicalD);
+bool CanUseCommentlessCache = false;
+if (LastChecked) {
+ for (auto *Redecl : CanonicalD->redecls()) {
+if (Redecl == D) {
+ CanUseCommentlessCache = true;
+ break;
+}
+if (Redecl == LastChecked)
+ break;
+ }
+}
+// FIXME: This could be improved so that even if CanUseCommentlessCache
+// is false, once we've traversed past CanonicalD we still skip ahead
+// LastChecked.
+return CanUseCommentlessCache ? LastChecked : nullptr;
}();
- for (const auto Redecl : D->redecls()) {
+ for (const Decl *Redecl : D->redecls()) {
assert(Redecl);
// Skip all redeclarations that have been checked previously.
if (LastCheckedRedecl) {
diff --git a/clang/unittests/AST/CMakeLists.txt
b/clang/unittests/AST/CMakeLists.txt
index 40d2e1ff77a601..bfa6082a6ffa4b 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -34,6 +34,7 @@ add_clang_unittest(ASTTests
NamedDeclPrinterTest.cpp
ProfilingTest.cpp
RandstructTest.cpp
+ RawCommentForDeclTest.cpp
RecursiveASTVisitorTest.cpp
SizelessTypesTest.cpp
SourceLocationTest.cpp
diff --git a/clang/unittests/AST/RawCommentForDeclTest.cpp
b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00..c81e56bf7e6b0c
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,98 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
HighCommander4 wrote: (Rebased) https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/108475
>From d224e1b658f56bf741cebf8dc5b2914716d9f47b Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 10 Sep 2024 22:34:55 -0400
Subject: [PATCH] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any
redecl with a comment
The previous implementation had a bug where, if it was called on a
Decl later in the redecl chain than `LastCheckedDecl`, it could
incorrectly skip and overlook a Decl with a comment.
The patch addresses this by only using `LastCheckedDecl` if the input
Decl `D` is on the path from the first (canonical) Decl to
`LastCheckedDecl`.
An alternative that was considered was to start the iteration from
the (canonical) Decl, however this ran into problems with the
modelling of explicit template specializations in the AST where
the canonical Decl can be unusual. With the current solution, if no
Decls were checked yet, we prefer to check the input Decl over the
canonical one.
---
clang/lib/AST/ASTContext.cpp | 22 -
clang/unittests/AST/CMakeLists.txt| 1 +
clang/unittests/AST/RawCommentForDeclTest.cpp | 98 +++
3 files changed, 117 insertions(+), 4 deletions(-)
create mode 100644 clang/unittests/AST/RawCommentForDeclTest.cpp
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index ebd4a41ee6367a..85b3984940ffc2 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -440,12 +440,26 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
}
// Any redeclarations of D that we haven't checked for comments yet?
- // We can't use DenseMap::iterator directly since it'd get invalid.
- auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-return CommentlessRedeclChains.lookup(CanonicalD);
+ const Decl *LastCheckedRedecl = [&]() {
+const Decl *LastChecked = CommentlessRedeclChains.lookup(CanonicalD);
+bool CanUseCommentlessCache = false;
+if (LastChecked) {
+ for (auto *Redecl : CanonicalD->redecls()) {
+if (Redecl == D) {
+ CanUseCommentlessCache = true;
+ break;
+}
+if (Redecl == LastChecked)
+ break;
+ }
+}
+// FIXME: This could be improved so that even if CanUseCommentlessCache
+// is false, once we've traversed past CanonicalD we still skip ahead
+// LastChecked.
+return CanUseCommentlessCache ? LastChecked : nullptr;
}();
- for (const auto Redecl : D->redecls()) {
+ for (const Decl *Redecl : D->redecls()) {
assert(Redecl);
// Skip all redeclarations that have been checked previously.
if (LastCheckedRedecl) {
diff --git a/clang/unittests/AST/CMakeLists.txt
b/clang/unittests/AST/CMakeLists.txt
index 40d2e1ff77a601..bfa6082a6ffa4b 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -34,6 +34,7 @@ add_clang_unittest(ASTTests
NamedDeclPrinterTest.cpp
ProfilingTest.cpp
RandstructTest.cpp
+ RawCommentForDeclTest.cpp
RecursiveASTVisitorTest.cpp
SizelessTypesTest.cpp
SourceLocationTest.cpp
diff --git a/clang/unittests/AST/RawCommentForDeclTest.cpp
b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00..c81e56bf7e6b0c
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,98 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const FoundComment &C) {
+return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+ << ", Comment: " << C.Comment << "}";
+ }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+ CollectCommentsAction(std::vector &Comments)
+ : Comments(Comments) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ llvm::StringRef) override {
+CI.getLangOpts().CommentOpts.ParseAllComments = true;
+return std::make_unique(*this);
+ }
+
+ std::vect
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/108475
>From 2b14e8063c21e32d771c3f82ec9fc2319a24d5a6 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 10 Sep 2024 22:34:55 -0400
Subject: [PATCH] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any
redecl with a comment
The previous implementation had a bug where, if it was called on a
Decl later in the redecl chain than `LastCheckedDecl`, it could
incorrectly skip and overlook a Decl with a comment.
The patch addresses this by only using `LastCheckedDecl` if the input
Decl `D` is on the path from the first (canonical) Decl to
`LastCheckedDecl`.
An alternative that was considered was to start the iteration from
the (canonical) Decl, however this ran into problems with the
modelling of explicit template specializations in the AST where
the canonical Decl can be unusual. With the current solution, if no
Decls were checked yet, we prefer to check the input Decl over the
canonical one.
---
clang/lib/AST/ASTContext.cpp | 22 -
clang/unittests/AST/CMakeLists.txt| 1 +
clang/unittests/AST/RawCommentForDeclTest.cpp | 98 +++
3 files changed, 117 insertions(+), 4 deletions(-)
create mode 100644 clang/unittests/AST/RawCommentForDeclTest.cpp
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a4e6d3b108c8a5..356f98af288d74 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -439,12 +439,26 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
}
// Any redeclarations of D that we haven't checked for comments yet?
- // We can't use DenseMap::iterator directly since it'd get invalid.
- auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-return CommentlessRedeclChains.lookup(CanonicalD);
+ const Decl *LastCheckedRedecl = [&]() {
+const Decl *LastChecked = CommentlessRedeclChains.lookup(CanonicalD);
+bool CanUseCommentlessCache = false;
+if (LastChecked) {
+ for (auto *Redecl : CanonicalD->redecls()) {
+if (Redecl == D) {
+ CanUseCommentlessCache = true;
+ break;
+}
+if (Redecl == LastChecked)
+ break;
+ }
+}
+// FIXME: This could be improved so that even if CanUseCommentlessCache
+// is false, once we've traversed past CanonicalD we still skip ahead
+// LastChecked.
+return CanUseCommentlessCache ? LastChecked : nullptr;
}();
- for (const auto Redecl : D->redecls()) {
+ for (const Decl *Redecl : D->redecls()) {
assert(Redecl);
// Skip all redeclarations that have been checked previously.
if (LastCheckedRedecl) {
diff --git a/clang/unittests/AST/CMakeLists.txt
b/clang/unittests/AST/CMakeLists.txt
index dcc9bc0f39ac2c..79ad8a28f2b33c 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -33,6 +33,7 @@ add_clang_unittest(ASTTests
NamedDeclPrinterTest.cpp
ProfilingTest.cpp
RandstructTest.cpp
+ RawCommentForDeclTest.cpp
RecursiveASTVisitorTest.cpp
SizelessTypesTest.cpp
SourceLocationTest.cpp
diff --git a/clang/unittests/AST/RawCommentForDeclTest.cpp
b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00..c81e56bf7e6b0c
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,98 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const FoundComment &C) {
+return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+ << ", Comment: " << C.Comment << "}";
+ }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+ CollectCommentsAction(std::vector &Comments)
+ : Comments(Comments) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ llvm::StringRef) override {
+CI.getLangOpts().CommentOpts.ParseAllComments = true;
+return std::make_unique(*this);
+ }
+
+ std::vect
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (PR #108475)
HighCommander4 wrote: Note, I also updated the commit message to reflect the new fix approach. https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote: Thanks for the reviews! I'll add the release note shortly (need to update to a newer baseline first). https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
@@ -0,0 +1,99 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const FoundComment &C) {
+return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+ << ", Comment: " << C.Comment << "}";
+ }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+ CollectCommentsAction(std::vector &Comments)
+ : Comments(Comments) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ llvm::StringRef) override {
+CI.getLangOpts().CommentOpts.ParseAllComments = true;
+return std::make_unique(*this);
+ }
+
+ std::vector &Comments;
+
+private:
+ class Consumer : public clang::ASTConsumer {
+ private:
+CollectCommentsAction &Action;
+
+ public:
+Consumer(CollectCommentsAction &Action) : Action(Action) {}
+~Consumer() override {}
HighCommander4 wrote:
The implicitly generated one does work. I think I just copied this from an
existing test which had it. Removed now.
https://github.com/llvm/llvm-project/pull/108475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
@@ -440,14 +440,23 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
// Any redeclarations of D that we haven't checked for comments yet?
// We can't use DenseMap::iterator directly since it'd get invalid.
- auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-return CommentlessRedeclChains.lookup(CanonicalD);
- }();
+ const Decl *LastCheckedRedecl = CommentlessRedeclChains.lookup(CanonicalD);
+ bool CanUseCommentlessCache = false;
+ if (LastCheckedRedecl) {
+for (auto *Redecl : CanonicalD->redecls()) {
+ if (Redecl == D) {
+CanUseCommentlessCache = true;
+break;
+ }
+ if (Redecl == LastCheckedRedecl)
+break;
+}
+ }
HighCommander4 wrote:
Done, thanks
https://github.com/llvm/llvm-project/pull/108475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/108475
>From 2b14e8063c21e32d771c3f82ec9fc2319a24d5a6 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 10 Sep 2024 22:34:55 -0400
Subject: [PATCH] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any
redecl with a comment
The previous implementation had a bug where, if it was called on a
Decl later in the redecl chain than `LastCheckedDecl`, it could
incorrectly skip and overlook a Decl with a comment.
The patch addresses this by only using `LastCheckedDecl` if the input
Decl `D` is on the path from the first (canonical) Decl to
`LastCheckedDecl`.
An alternative that was considered was to start the iteration from
the (canonical) Decl, however this ran into problems with the
modelling of explicit template specializations in the AST where
the canonical Decl can be unusual. With the current solution, if no
Decls were checked yet, we prefer to check the input Decl over the
canonical one.
---
clang/lib/AST/ASTContext.cpp | 22 -
clang/unittests/AST/CMakeLists.txt| 1 +
clang/unittests/AST/RawCommentForDeclTest.cpp | 98 +++
3 files changed, 117 insertions(+), 4 deletions(-)
create mode 100644 clang/unittests/AST/RawCommentForDeclTest.cpp
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a4e6d3b108c8a5..356f98af288d74 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -439,12 +439,26 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
}
// Any redeclarations of D that we haven't checked for comments yet?
- // We can't use DenseMap::iterator directly since it'd get invalid.
- auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-return CommentlessRedeclChains.lookup(CanonicalD);
+ const Decl *LastCheckedRedecl = [&]() {
+const Decl *LastChecked = CommentlessRedeclChains.lookup(CanonicalD);
+bool CanUseCommentlessCache = false;
+if (LastChecked) {
+ for (auto *Redecl : CanonicalD->redecls()) {
+if (Redecl == D) {
+ CanUseCommentlessCache = true;
+ break;
+}
+if (Redecl == LastChecked)
+ break;
+ }
+}
+// FIXME: This could be improved so that even if CanUseCommentlessCache
+// is false, once we've traversed past CanonicalD we still skip ahead
+// LastChecked.
+return CanUseCommentlessCache ? LastChecked : nullptr;
}();
- for (const auto Redecl : D->redecls()) {
+ for (const Decl *Redecl : D->redecls()) {
assert(Redecl);
// Skip all redeclarations that have been checked previously.
if (LastCheckedRedecl) {
diff --git a/clang/unittests/AST/CMakeLists.txt
b/clang/unittests/AST/CMakeLists.txt
index dcc9bc0f39ac2c..79ad8a28f2b33c 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -33,6 +33,7 @@ add_clang_unittest(ASTTests
NamedDeclPrinterTest.cpp
ProfilingTest.cpp
RandstructTest.cpp
+ RawCommentForDeclTest.cpp
RecursiveASTVisitorTest.cpp
SizelessTypesTest.cpp
SourceLocationTest.cpp
diff --git a/clang/unittests/AST/RawCommentForDeclTest.cpp
b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00..c81e56bf7e6b0c
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,98 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const FoundComment &C) {
+return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+ << ", Comment: " << C.Comment << "}";
+ }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+ CollectCommentsAction(std::vector &Comments)
+ : Comments(Comments) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ llvm::StringRef) override {
+CI.getLangOpts().CommentOpts.ParseAllComments = true;
+return std::make_unique(*this);
+ }
+
+ std::vect
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
@@ -444,7 +444,7 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
return CommentlessRedeclChains.lookup(CanonicalD);
}();
- for (const auto Redecl : D->redecls()) {
+ for (const auto Redecl : CanonicalD->redecls()) {
HighCommander4 wrote:
(The part that makes less sense to me is, why does the explicit specialization
have a redeclaration at all in a case like
```c++
/// Aaa.
template
void foo(T aaa, U bbb);
/// Bbb.
template<>
void foo(int aaa, int bbb);
```
There is only one declaration in the code.)
https://github.com/llvm/llvm-project/pull/108475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
@@ -444,7 +444,7 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
return CommentlessRedeclChains.lookup(CanonicalD);
}();
- for (const auto Redecl : D->redecls()) {
+ for (const auto Redecl : CanonicalD->redecls()) {
HighCommander4 wrote:
> regarding `getSourceRange()` relying on `TemplateParameterListsInfo`, which
> is really peculiar
FWIW, that part actually makes sense to me. For a function/method declaration
that looks like this:
```
template <...>
ReturnType FunctionName(Parameters...);
```
the answer to the question "where does the source range begin?" is indeed "if
there is a template parameter list, where does it begin?"
https://github.com/llvm/llvm-project/pull/108475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote: Buildkite is green with this approach! Graduated patch from "Draft" state. https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 ready_for_review https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
@@ -444,7 +444,7 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
return CommentlessRedeclChains.lookup(CanonicalD);
}();
- for (const auto Redecl : D->redecls()) {
+ for (const auto Redecl : CanonicalD->redecls()) {
HighCommander4 wrote:
Thank you for the suggestion! It indeed feels safer to tweak
`getRawCommentsForAnyRedecl()` like this than to muck around with the modeling
of template specializations :laughing:
https://github.com/llvm/llvm-project/pull/108475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/108475
>From 1df905728da591bae0acf231e2d7c1f7492d43f3 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 10 Sep 2024 22:34:55 -0400
Subject: [PATCH] [AST] Ensure getRawCommentsForAnyRedecl() does not miss any
redecl with a comment
The previous implementation had a bug where, if it was called on a
Decl later in the redecl chain than `LastCheckedDecl`, it could
incorrectly skip and overlook a Decl with a comment.
The patch addresses this by only using `LastCheckedDecl` if the input
Decl `D` is on the path from the first (canonical) Decl to
`LastCheckedDecl`.
An alternative that was considered was to start the iteration from
the (canonical) Decl, however this ran into problems with the
modelling of explicit template specializations in the AST where
the canonical Decl can be unusual. With the current solution, if no
Decls were checked yet, we prefer to check the input Decl over the
canonical one.
---
clang/lib/AST/ASTContext.cpp | 19 +++-
clang/unittests/AST/CMakeLists.txt| 1 +
clang/unittests/AST/RawCommentForDeclTest.cpp | 99 +++
3 files changed, 114 insertions(+), 5 deletions(-)
create mode 100644 clang/unittests/AST/RawCommentForDeclTest.cpp
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a4e6d3b108c8a5..c9b9850dbd2b54 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -440,14 +440,23 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
// Any redeclarations of D that we haven't checked for comments yet?
// We can't use DenseMap::iterator directly since it'd get invalid.
- auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-return CommentlessRedeclChains.lookup(CanonicalD);
- }();
+ const Decl *LastCheckedRedecl = CommentlessRedeclChains.lookup(CanonicalD);
+ bool CanUseCommentlessCache = false;
+ if (LastCheckedRedecl) {
+for (auto *Redecl : CanonicalD->redecls()) {
+ if (Redecl == D) {
+CanUseCommentlessCache = true;
+break;
+ }
+ if (Redecl == LastCheckedRedecl)
+break;
+}
+ }
- for (const auto Redecl : D->redecls()) {
+ for (const Decl *Redecl : D->redecls()) {
assert(Redecl);
// Skip all redeclarations that have been checked previously.
-if (LastCheckedRedecl) {
+if (CanUseCommentlessCache && LastCheckedRedecl) {
if (LastCheckedRedecl == Redecl) {
LastCheckedRedecl = nullptr;
}
diff --git a/clang/unittests/AST/CMakeLists.txt
b/clang/unittests/AST/CMakeLists.txt
index dcc9bc0f39ac2c..79ad8a28f2b33c 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -33,6 +33,7 @@ add_clang_unittest(ASTTests
NamedDeclPrinterTest.cpp
ProfilingTest.cpp
RandstructTest.cpp
+ RawCommentForDeclTest.cpp
RecursiveASTVisitorTest.cpp
SizelessTypesTest.cpp
SourceLocationTest.cpp
diff --git a/clang/unittests/AST/RawCommentForDeclTest.cpp
b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00..b811df28127d43
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,99 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const FoundComment &C) {
+return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+ << ", Comment: " << C.Comment << "}";
+ }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+ CollectCommentsAction(std::vector &Comments)
+ : Comments(Comments) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ llvm::StringRef) override {
+CI.getLangOpts().CommentOpts.ParseAllComments = true;
+return std::make_unique(*this);
+ }
+
+ std::vector &Comments;
+
+private:
+ class Consumer : public clang::ASTConsumer {
+ private:
+CollectCommentsAction &Action;
+
+ public:
+Con
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote:
@zyn0217 I've debugged the `DefineInlineTest.AddInline` failure with your new
suggested change, it concerns the following scenario:
header.h
```c++
template void foo();
template <> void foo();
```
source.cpp:
```c++
#include "header.h"
template <> void foo() {}
```
It seems the `FuntionDecl` for the explicit specialization declaration in the
header file gets assigned the template parameter lists of the definition in the
main file, and its `getBeginLoc()` starts returning the main-file location.
https://github.com/llvm/llvm-project/pull/108475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: > As mentions in [this > post](https://github.com/llvm/llvm-project/issues/63565#issuecomment-2337396222): > these to issues remain within vsocde/vscode-clangd: > >* None: after completion an error squiggly line will occur on the > identifier. Would be slightly nicer if it wouldn't. > >* OpenDelimiter: the signature help hover doesn't spawn. It seems that > [Sam's Patch from > #398](https://github.com/clangd/vscode-clangd/pull/398/files) needs to be > reconfigured for this. Could you file a new issue for each of these please? https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -126,11 +126,29 @@ struct Config {
std::vector FullyQualifiedNamespaces;
} Style;
+ /// controls the completion options for argument lists.
+ enum class ArgumentListsOption {
+/// the default value. This will imply FullPlaceholders unless overridden
by
+/// --function-arg-placeholders=0, in which case Delimiters is used.
+/// any other use-case of --function-arg-placeholders is ignored
+UnsetDefault = 0,
+/// nothing, no argument list and also NO Delimiters "()" or "<>"
+None,
+/// open, only opening delimiter "(" or "<"
+OpenDelimiter,
+/// empty pair of delimiters "()" or "<>" (or [legacy] alias 0)
HighCommander4 wrote:
nit: these mentions of legacy aliases can be removed (it's just confusing since
that applies to the command line flag which is defined elsewhere)
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -126,11 +126,29 @@ struct Config {
std::vector FullyQualifiedNamespaces;
} Style;
+ /// controls the completion options for argument lists.
+ enum class ArgumentListsOption {
+/// the default value. This will imply FullPlaceholders unless overridden
by
+/// --function-arg-placeholders=0, in which case Delimiters is used.
+/// any other use-case of --function-arg-placeholders is ignored
+UnsetDefault = 0,
HighCommander4 wrote:
This value isn't necessary.
The way config providers interact, using `BackgroundPolicy` (an existing config
option which also exists as a command-line flag) as an example:
1. A `Config` object is created
2. `FlagsConfigProvider` runs first. If the relevant command-line flag
(`--background-index`) was specified, it
[assigns](https://searchfox.org/llvm/rev/87d56c59f52d033cd7c46d769338b9c47fea4929/clang-tools-extra/clangd/tool/ClangdMain.cpp#703)
the provided value to the `Index.Background` field of the `Config` object
created at step (1). Note, if the flag wasn't provided, this line doesn't run
at all.
3. Then the config provider for the config file runs. The data structure
representing data parsed from the file stores everything [using
`std::optional`](https://searchfox.org/llvm/rev/87d56c59f52d033cd7c46d769338b9c47fea4929/clang-tools-extra/clangd/ConfigFragment.h#182),
which is only populated if the key is present in the config file. That then
[overwrites](https://searchfox.org/llvm/rev/87d56c59f52d033cd7c46d769338b9c47fea4929/clang-tools-extra/clangd/ConfigCompile.cpp#333)
the `Index.Background` field of the same `Config` object, but once again this
line only runs at all if the `optional` was populated.
As a result, no "unset" value is needed; `std::optional` takes care of that.
The default value of `Completion::ArgumentLists` below -- used if neither a
command-line flag nor a config file key was specified -- should be
`FullPlaceholders`.
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -308,6 +309,13 @@ struct Fragment {
/// Whether code completion should include suggestions from scopes that are
/// not visible. The required scope prefix will be inserted.
std::optional> AllScopes;
+/// How to present the argument list between '()' and '<>':
+/// valid values are enum Config::ArgumentListsOption values:
+/// None: Nothing at all
+/// OpenDelimiter: only opening delimiter "(" or "<"
+/// Delimiters: empty pair of delimiters "()" or "<>"
+/// FullPlaceholders: full name of both type and variable
HighCommander4 wrote:
nit: variable --> parameter
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -6,6 +6,7 @@ // //===--===// +#include "../Config.h" HighCommander4 wrote: This can be included without the `../`. (Please alphabetize it among the existing includes.) https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -230,6 +230,10 @@ class Parser {
if (auto AllScopes = boolValue(N, "AllScopes"))
F.AllScopes = *AllScopes;
});
+Dict.handle("ArgumentLists", [&](Node &N) {
+ if (auto ArgumentLists = scalarValue(N, "ArgumentLists"))
HighCommander4 wrote:
since `F.ArgumentLists` is itself an `optional`, this can be simplified to:
`F.ArgumentLists = scalarValue(N, "ArgumentLists");`
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -694,13 +698,20 @@ class FlagsConfigProvider : public config::Provider {
BGPolicy = Config::BackgroundPolicy::Skip;
}
+if (PlaceholderOption == Config::ArgumentListsOption::Delimiters) {
+ ArgumentLists = PlaceholderOption;
+}
+
Frag = [=](const config::Params &, Config &C) {
if (CDBSearch)
C.CompileFlags.CDBSearch = *CDBSearch;
if (IndexSpec)
C.Index.External = *IndexSpec;
if (BGPolicy)
C.Index.Background = *BGPolicy;
+ if (ArgumentLists && C.Completion.ArgumentLists ==
HighCommander4 wrote:
For reasons mentioned above, no need for the second condition here (at this
point, the config provider for the config file has not **yet** assigned
anything to `C`; when it does run later, it will overwrite the field if needed)
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -622,6 +622,31 @@ struct FragmentCompiler {
C.Completion.AllScopes = AllScopes;
});
}
+if (F.ArgumentLists) {
HighCommander4 wrote:
We have a `compileEnum` helper to do this more easily, see [the handling of
`BackgroundPolicy`](https://searchfox.org/llvm/rev/87d56c59f52d033cd7c46d769338b9c47fea4929/clang-tools-extra/clangd/ConfigCompile.cpp#326-334)
as an example
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -2595,10 +2596,10 @@ TEST(SignatureHelpTest, DynamicIndexDocumentation) {
ElementsAre(AllOf(sig("foo() -> int"), sigDoc("Member doc";
}
-TEST(CompletionTest, CompletionFunctionArgsDisabled) {
+TEST(CompletionTest, ArgumentListsNotFullPlaceholders) {
HighCommander4 wrote:
nit: suggestion for slightly more general test name: `ArgumentListsPolicy`
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -504,10 +504,10 @@ struct FragmentCompiler {
auto Fast = isFastTidyCheck(Str);
if (!Fast.has_value()) {
diag(Warning,
- llvm::formatv(
- "Latency of clang-tidy check '{0}' is not known. "
- "It will only run if ClangTidy.FastCheckFilter is Loose or
None",
- Str)
+ llvm::formatv("Latency of clang-tidy check '{0}' is not known. "
HighCommander4 wrote:
nit: I'm seeing some diff hunks containing unrelated formatting changes. If
would be great to remove this as they pollute the blame
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -561,14 +561,32 @@ struct CodeCompletionBuilder {
}
std::string summarizeSnippet() const {
+
+/// localize a ArgList depending on the config. If the config is unset we
+/// will use our local (this) version, else use the one of the config
+const Config::ArgumentListsOption ArgList =
HighCommander4 wrote:
For consistency with our existing `Completion` option (`AllScopes`), let's do
the `Config` lookup
[here](https://searchfox.org/llvm/rev/87d56c59f52d033cd7c46d769338b9c47fea4929/clang-tools-extra/clangd/ClangdServer.cpp#453)
and propagate it into `CodeCompleteOpts`.
That then gets propagated to `CodeCompletionBuilder::ArgumentLists` in the
`CodeCompletionBuilder` constructor, and here we can just use the
`ArgumentLists` member without doing anything further.
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -242,13 +242,16 @@ opt FallbackStyle{
init(clang::format::DefaultFallbackStyle),
};
-opt EnableFunctionArgSnippets{
-"function-arg-placeholders",
-cat(Features),
-desc("When disabled, completions contain only parentheses for "
- "function calls. When enabled, completions also contain "
- "placeholders for method parameters"),
-init(CodeCompleteOptions().EnableFunctionArgSnippets),
+opt PlaceholderOption{
HighCommander4 wrote:
For simplicity and consistency with `--background-index`, let's keep the name
and type of this unchanged
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
@@ -126,11 +126,29 @@ struct Config {
std::vector FullyQualifiedNamespaces;
} Style;
+ /// controls the completion options for argument lists.
+ enum class ArgumentListsOption {
HighCommander4 wrote:
naming suggestion: `ArgumentListsPolicy`, for consistency with other enums
defined in this file
https://github.com/llvm/llvm-project/pull/108005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
https://github.com/HighCommander4 requested changes to this pull request. Thanks! Generally looks pretty good, a few minor comments / cleanups and this should be good to go. https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote: Hmm, quite a few things are failing: ``` Failed Tests (14): Clang :: CXX/drs/cwg28xx.cpp Clang :: CXX/drs/cwg7xx.cpp Clang :: CXX/temp/temp.spec/temp.expl.spec/p2-20.cpp Clang :: CXX/temp/temp.spec/temp.expl.spec/p8.cpp Clang :: Index/index-file.cpp Clang :: PCH/cxx-ms-function-specialization-class-scope.cpp Clang :: PCH/cxx-templates.cpp Clang :: SemaCXX/ms-exception-spec.cpp Clang :: SemaTemplate/explicit-specialization-member.cpp Clang :: SemaTemplate/function-template-specialization.cpp Clang :: SemaTemplate/instantiate-member-specialization.cpp Clang :: SemaTemplate/member-specialization.cpp Clang :: SemaTemplate/ms-function-specialization-class-scope.cpp Clangd Unit Tests :: ./ClangdTests/DefineInlineTest/AddInline ``` https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote: @zyn0217 Thank you for the analysis and suggestion! I updated the patch as suggested, let's see what buildkite says. https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 edited https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 updated
https://github.com/llvm/llvm-project/pull/108475
>From 408259c2e28e4664f0d0c47a6a897c6eb5660f93 Mon Sep 17 00:00:00 2001
From: Nathan Ridge
Date: Tue, 10 Sep 2024 22:34:55 -0400
Subject: [PATCH] [AST] Iterate redecls starting from the canonical one in
getRawCommentsForAnyRedecl()
The intent of the `CommentlessRedeclChains` cache is that if new redecls
have been parsed since the last time getRawCommentsForAnyRedecl() was
called, only the new redecls are checked for comments.
However, redecls are a circular list, and if iteration starts from the
input decl `D`, that could be a new one, and we incorrectly skip it
while traversing the list to `LastCheckedRedecl`.
Starting the iteration from the first (canonical) decl makes the cache
work as intended.
The patch also plugs a hole in the modeling of explicit function
template specializations where the FunctionDecl object created
implicitly during template argument deduction for the explicit
specialization does not store the specialization's template argument
lists.
---
clang/lib/AST/ASTContext.cpp | 2 +-
clang/lib/Sema/SemaTemplate.cpp | 5 +
clang/unittests/AST/CMakeLists.txt| 1 +
clang/unittests/AST/RawCommentForDeclTest.cpp | 99 +++
4 files changed, 106 insertions(+), 1 deletion(-)
create mode 100644 clang/unittests/AST/RawCommentForDeclTest.cpp
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a4e6d3b108c8a5..3735534ef3d3f1 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -444,7 +444,7 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
return CommentlessRedeclChains.lookup(CanonicalD);
}();
- for (const auto Redecl : D->redecls()) {
+ for (const auto Redecl : CanonicalD->redecls()) {
assert(Redecl);
// Skip all redeclarations that have been checked previously.
if (LastCheckedRedecl) {
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index a032e3ec6f6353..9b354052fab4b5 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -10455,6 +10455,11 @@ bool Sema::CheckFunctionTemplateSpecialization(
// Mark the prior declaration as an explicit specialization, so that later
// clients know that this is an explicit specialization.
if (!isFriend) {
+SmallVector TPL;
+for (unsigned I = 0; I < FD->getNumTemplateParameterLists(); ++I)
+ TPL.push_back(FD->getTemplateParameterList(I));
+Specialization->setTemplateParameterListsInfo(Context, TPL);
+
// Since explicit specializations do not inherit '=delete' from their
// primary function template - check if the 'specialization' that was
// implicitly generated (during template argument deduction for partial
diff --git a/clang/unittests/AST/CMakeLists.txt
b/clang/unittests/AST/CMakeLists.txt
index dcc9bc0f39ac2c..79ad8a28f2b33c 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -33,6 +33,7 @@ add_clang_unittest(ASTTests
NamedDeclPrinterTest.cpp
ProfilingTest.cpp
RandstructTest.cpp
+ RawCommentForDeclTest.cpp
RecursiveASTVisitorTest.cpp
SizelessTypesTest.cpp
SourceLocationTest.cpp
diff --git a/clang/unittests/AST/RawCommentForDeclTest.cpp
b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00..b811df28127d43
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,99 @@
+//===- unittests/AST/RawCommentForDeclTestTest.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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+ std::string DeclName;
+ bool IsDefinition;
+ std::string Comment;
+
+ bool operator==(const FoundComment &RHS) const {
+return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+ Comment == RHS.Comment;
+ }
+ friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+ const FoundComment &C) {
+return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+ << ", Comment: " << C.Comment << "}";
+ }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+ CollectCommentsAction(std::vector &Comments)
+ : Comments(Comments) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote: > Buildkite is showing the test `Clang :: > Index/comment-to-html-xml-conversion.cpp` failing. Will investigate. I've been investigating this failure. It's caused by a slight change of behaviour of `ASTContext::getFullComment()` on explicit function template specializations, such as: ```c++ /// Aaa. template void foo(T aaa, U bbb); /// Bbb. template<> void foo(int aaa, int bbb); ``` While the correct comment (`// Bbb.`) is found for the explicit specialization, the returned `FullComment`'s `DeclInfo` does not have its [`TemplateKind`](https://searchfox.org/llvm/rev/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/clang/include/clang/AST/Comment.h#1047) set to `TemplateSpecialization` as expected. This is in turn because the [code that sets this](https://searchfox.org/llvm/rev/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/clang/lib/AST/Comment.cpp#238-243) is conditioned on `FunctionDecl::getNumTemplateParameterLists() != 0`. When `getRawCommentForAnyRedecl()` is called on the `FunctionDecl` for the explicit specialization (whose `getNumTemplateParameterLists()` returns 1), it previously returned the input `FunctionDecl` in its `OriginalDecl` out-parameter, but as a result of my change, it now returns the input decl's canonical decl, whose `getNumTemplateParameterLists()` returns 0, and it's this latter decl that ends up in the check mentioned above. I'm not familiar enough with the AST modeling of template specializations to say what is going wrong here... @gribozavr as the author of the [mentioned check](https://searchfox.org/llvm/rev/3ae71d154e5dfb5e5a5d27b3699b27ce2b55f44d/clang/lib/AST/Comment.cpp#238-243), any advice would be appreciated. https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
https://github.com/HighCommander4 converted_to_draft https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AST] Iterate redecls starting from the canonical one in getRawCommentsForAnyRedecl() (PR #108475)
HighCommander4 wrote: Buildkite is showing the test `Clang :: Index/comment-to-html-xml-conversion.cpp` failing. Will investigate. https://github.com/llvm/llvm-project/pull/108475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: > Is this okay!? Yep. By the way, this was specified in the [original proposal](https://github.com/llvm/llvm-project/issues/63565#issuecomment-1975065771): > Here's a concrete proposal for a config file syntax that would address this > request, https://github.com/clangd/clangd/issues/1252, and supersede > `--function-arg-placeholders`: add a new key **under `Completion`** called > `ArgumentLists`, with the following supported values: https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: > Thanks for the input, a followup question is: Which one takes precedence? > Does the `--function-arg-placeholders` argument take precedence over > `.clangd` config!? Or the other way around!? `.clangd` should take precedence because it's the a more specific setting (command line arguments apply to the whole clangd process, `.clangd` files are scoped to a directory). If you are handling the flag in `FlagsConfigProvider` as suggested, this will be the resulting behaviour will no further logic needed. https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)
HighCommander4 wrote: > Just to be clear, do you want to keep the new `--function-arg-placeholders` > flags and also add it to the `.clangd` config!? Or do you only want the > `.clangd` config and drop the changes to the `--function-arg-placeholders` > flag!? The latter. The only change related to the command-line flag that's needed, is to translate its (still boolean) value into the new (enum-valued) config option; we have a class named [FlagsConfigProvider](https://searchfox.org/llvm/rev/36ad0720de623221e3cc17d30f4173331c099a72/clang-tools-extra/clangd/tool/ClangdMain.cpp#638) which helps with that (so that the rest of the code only needs to care about the config option). https://github.com/llvm/llvm-project/pull/108005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
