sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:250
+ case ReasonToReject::UnrenamableLoc:
+ return "no rename at the given location";
}
----------------
I don't know what a user would make of this message :-)
It's useful for debugging the implementation, but all we're saying is that we
generated a list of edits and then our sanity check failed.
This case occurs when the user selected some code that resolved to a renamable
node, but wasn't actually the name of it.
It's basically the same as trying to rename foo with `void foo() {^}`, which
fails with NoSymbolFound.
So I think we should either return NoSymbolFound here or at least use the same
error message.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:758
+
+ // Check the rename-triggering location is actually being renamed.
+ // This is a robust check to avoid surprising rename results -- if the
----------------
nit: robust check -> robustness check
And
"to avoid surprising results if the triggering location *is not actually the
name of the node we identified* (e.g. for broken code)."
rather than just repeating "is not renamed"
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:1098
+ TU.ExtraArgs.push_back("-Xclang");
+ TU.ExtraArgs.push_back("-fno-recovery-ast");
auto AST = TU.build();
----------------
this seems like a weird lever to have to use, and will make it awkward if we
want to test behavior in the presence of recovery AST.
Haha, I remember now, I had a fix to generate recovery initializers in more
cases which probably made tests harder to right.
That code only works if the field can be resolved though. Maybe try `A() :
inva^lid(0) {}` ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101816/new/
https://reviews.llvm.org/D101816
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
