ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:225
tooling::Replacements FilteredChanges;
- for (const tooling::SymbolOccurrence &Rename :
- findOccurrencesWithinFile(AST, RenameDecl)) {
- // Currently, we only support normal rename (one range) for C/C++.
- // FIXME: support multiple-range rename for objective-c methods.
- if (Rename.getNameRanges().size() > 1)
- continue;
- // We shouldn't have conflicting replacements. If there are conflicts, it
- // means that we have bugs either in clangd or in Rename library, therefore
- // we refuse to perform the rename.
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+ // We shouldn't have conflicting replacements or replacements from
different
----------------
hokein wrote:
> ilya-biryukov wrote:
> > This seems to assume all occurrences are outside macros.
> > Won't it break in presence of macros?
> > Do we have tests when the renamed token is:
> > - inside macro body
> > - inside macro arguments
> > ?
> if Loc is a macro location, tooling::Replacement will use the spelling
> location, so if the spelling location is in the main file, the code is
> correct, we have test cases for it.
>
> but we will fail on cases like below, we should fix this, note that this
> issue exists even before this patch. Updated the comment here.
>
> ```
> // foo.h
> #define MACRO foo
>
> // foo.cc
> void f() {
> int fo^o = 2;
> MACRO;
> }
> ```
Renaming the spelling location seems like a bad idea in this case, I would
argue we shouldn't rename in macro bodies (see the relevant comment on the
test-case, let's move discussion there maybe?)
It would also be good to handle rename in macros outside
`tooling::Replacement`, it's an important decision and I we should not blindly
stick to the default behavior of `tooling::Replacement` without explicitly
explaining why it's the best thing for rename.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:202
+ R"cpp(
+ #define moo [[foo]]
+ int [[fo^o]]() { return 42; }
----------------
I would argue we should never rename in macro bodies.
That might cause problems in other uses of the macro as the token might refer
to a completely different thing in some other place.
It's also a pretty rare case, so failing here instead of breaking code seems
like the proper trade-off.
Are we keen on supporting this? If yes, why?
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:234
+ template <typename T>
+ class [[F^oo]] {
+ public:
----------------
Could we also check partial and full specializations work?
```
template <>
class Foo<bool> {};
template <class T>
class Foo<T*> {};
Foo<int> x;
Foo<bool> y;
Foo<int*> z;
```
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:313
+ R"cpp(
+ #define NAMESPACE namespace A
+ NAMESPACE {
----------------
Why does this have to be a macro? This test does not seem to test macros in any
way?
Maybe inline the macro?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69934/new/
https://reviews.llvm.org/D69934
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
