sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:91
+ DeclRelation::Alias | DeclRelation::TemplatePattern)) {
+ // If the cursor is at the underlying CXXRecordDecl of the
+ // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need
to
----------------
why is this code being moved?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:453
+ if (!Offset) {
+ return makeError(ReasonToReject::UsedOutsideFile);
+ }
----------------
so getting an invalid location from the editor isn't the same thing as the
symbol being used outside the file! Here we already have an Error to return.
```
if (!Offset)
return Offset.takeError();
```
but really there's a helper to do this:
```
SourceLocation Loc = sourceLocationInMainFile(SM, RInputs.Pos);
if (!Loc)
return Loc.takeError();
const syntax::Token *IdentifierToken = spelledIdentifierTouching(Loc,
AST.getTokens());
```
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:456
+ SourceLocation TokenLocation = SM.getComposedLoc(FID, *Offset);
+ const auto IdentifierToken =
+ syntax::spelledIdentifierTouching(TokenLocation, AST.getTokens());
----------------
nit: auto* for pointers, but spelling out the type here would be clearer I think
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:458
+ syntax::spelledIdentifierTouching(TokenLocation, AST.getTokens());
+ // There should be exactly one identifier token the cursor is pointing to.
+ // Renames should only triggered on identifiers.
----------------
this is an invariant of the function we're calling, not this one - no need to
document it (next line is good though)
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:462
+ return makeError(ReasonToReject::NoSymbolFound);
+ SourceLocation SourceLocationBeg =
+ SM.getMacroArgExpandedLocation(IdentifierToken->location());
----------------
This unwrapping doesn't seem necessary, - it doesn't make a difference for
macros (which we don't support anyway), nor for selectiontree. Just inline
usages of IdentifierToken->location() below?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71247/new/
https://reviews.llvm.org/D71247
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
