hokein added a comment.
thanks, mostly good. the only concern from my side is about the
`SymbolRole::Implicit`, I think we should get rid of it.
================
Comment at: clang-tools-extra/clangd/index/Ref.h:53
+ // The reference explicitly spells out declaration's name. Such references
can
+ // not come from macro expansions or implicit AST nodes.
+ //
----------------
maybe make sense to include an AST example?
```
class Foo { public: Foo(); };
Foo [[f]]; // there is a reference of `Foo` constructor around `f`, this is a
non-spelled reference obviously.
```
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:191
+ Result |= RefKind::Reference;
+ if (!(Roles & static_cast<unsigned>(index::SymbolRole::Implicit)))
+ Result |= RefKind::Spelled;
----------------
kbobyrev wrote:
> hokein wrote:
> > I was confused at the first time reading the code -- it turns out we reset
> > the flag in the caller.
> >
> > Maybe adjust the function like `RefKind toRefKind(index::SymbolRoleSet
> > Roles, bool isSpelled)`?
> If my understanding is correct, the suggestion is to take `isSpelled`
> argument and apply the `RefKind` flag based on the value of the argument
> instead of using `SymbolRole::Implicit`. Is that right? In this case I would
> be concerned about doing because that increase the amount of code in case
> there is any other index provider that sets the flags itself.
>
> What do you think?
it is not about the amount of code, it is about layering. I think we should not
use the `Implicit`. With the current change, the `SymbolRole::Implicit` comes
from two sources:
1) the flag can be set in libindex;
2) the flag can be set in SymbolCollector (via the post-processing in `finish`)
for 1), the implementation of libindex is not completed, only object-c related
decls get set. I think our aim is to set the `Spelled` flag if 2) is true.
Another way doing that would be keep the `toRefKind` implementation unchanged,
and set the `Spelled` flag afterwards.
================
Comment at: clang-tools-extra/clangd/index/SymbolLocation.h:12
+#include "Protocol.h"
#include "llvm/ADT/StringRef.h"
----------------
I would not do this change, it introduces a new dependency to this header.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:36
Ref Result;
- Result.Kind = RefKind::Reference;
+ Result.Kind = RefKind::Reference | RefKind::Spelled;
Result.Location.Start.setLine(Range.start.line);
----------------
do we need this change in this patch?
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:730
+ AllOf(HaveRanges(AllRanges),
+ RefKindIs(RefKind::Spelled, SpelledRanges)))));
+}
----------------
I found the RefKindIs is hard to follow without reading its implementation, I
think we can do it in another way, retrieve all spelled refs from `Refs`, and
verify them:
```
EXPECT_THAT(SpelledRefs,
UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
AllOf(HaveRanges(Main.ranges("spelled"))), SpelledKind()));
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72746/new/
https://reviews.llvm.org/D72746
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
