hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:134
+ // although it might be a bigger effort.
+ const auto *FieldParent = dyn_cast<CXXRecordDecl>(Field->getParent())
+ ->getTemplateInstantiationPattern();
----------------
sorry, I thought `Field->getParent()` returns a `CXXRecordDecl`.
I think this is dangerous for C, getParent returns a `RecordDecl`, then
dyn_cast returns null, and we will access a nullptr.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:142
+ for (const FieldDecl *Candidate : FieldParent->fields()) {
+ if (Field->getFieldIndex() == Candidate->getFieldIndex()) {
+ assert(Field->getLocation() == Candidate->getLocation() &&
----------------
kbobyrev wrote:
> hokein wrote:
> > I think we could also check the equality of their names.
> Yes, that is what @sammccall proposed too, but names not simple `unsigned`
> numbers, checking for that is just more expensive.
I think I meant to use `Field->getDeclName() == Candidate->getDeclName()`,
`DeclarationName` comparison is cheap, just comparing with the underlying
pointer -- clang will unique all identifiers in the `IdentifierTable`.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:613
+ )cpp",
+ R"cpp(
+ template<typename T>
----------------
can you add a template partial testcase? something like
```
template <typename T, typename U> struct Foo { static T Variable; };
template <typename T> struct Foo<T, bool> {
static T Variable;
};
void test() {
Foo<int, bool>::Variable = 5;
}
```
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:575
+ T Variable[42];
+ U Another;
+
----------------
kbobyrev wrote:
> hokein wrote:
> > `Another` seems to be not needed. I'd suggest removing it to make the
> > testcase as tense as possible.
> In this case there is only one field and I'd be happy to check that the
> correct field gets renamed in case there are two of them (this is when the
> index comparison is checked). Otherwise this would pass without the index
> checking (there is one field, we don't check if it's actually the one we need
> and it gets renamed regardless).
I see, for index comparison. this makes sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91952/new/
https://reviews.llvm.org/D91952
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
