Page MenuHomePhabricator

[clangd] don't rename if the triggering loc is not actually being renamed.
ClosedPublic

Authored by hokein on May 3 2021, 11:47 PM.

Diff Detail

Event Timeline

hokein created this revision.May 3 2021, 11:47 PM
hokein requested review of this revision.May 3 2021, 11:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 11:47 PM
sammccall accepted this revision.Tue, Jun 1, 8:32 AM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/Rename.cpp
249

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.

755

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"

clang-tools-extra/clangd/unittests/RenameTests.cpp
1095

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) {} ?

This revision is now accepted and ready to land.Tue, Jun 1, 8:32 AM
hokein updated this revision to Diff 351408.Fri, Jun 11, 4:49 AM

address comments

This revision was landed with ongoing or failed builds.Fri, Jun 11, 4:56 AM
This revision was automatically updated to reflect the committed changes.
hokein marked 3 inline comments as done.