This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix a crash when renaming operator.
ClosedPublic

Authored by hokein on Sep 16 2019, 1:36 AM.

Details

Summary

The renamelib uses a tricky way to calculate the end location by relying
on decl name, this is incorrect for the overloaded operator (the name is
"operator++" instead of "++"), which will cause out-of-file offset.

We also disable renaming operator symbol, this case is tricky, and
renamelib doesnt handle it properly..

Event Timeline

hokein created this revision.Sep 16 2019, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 1:36 AM
hokein retitled this revision from clangd] Fix a crash when renaming operator. to [clangd] Fix a crash when renaming operator..Sep 16 2019, 1:36 AM
hokein updated this revision to Diff 220289.Sep 16 2019, 1:37 AM

Fix an accident change.

Harbormaster completed remote builds in B38156: Diff 220289.

LGTM to fix the crash. See the comment about more cases that could potentially break, though. Do we handle them gracefully now?

clang-tools-extra/clangd/refactor/Rename.cpp
77

Should we more generally disable renaming all non-identifier names? Each of them seems to require special treatment.

A few examples that come to mind:

  • conversion operators, e.g. operator int();
  • user-defined literals, e.g. std::chrono::seconds operator ""s(int seconds);
  • destructors;
ilya-biryukov accepted this revision.Sep 16 2019, 2:40 AM
This revision is now accepted and ready to land.Sep 16 2019, 2:40 AM
hokein marked 2 inline comments as done.Sep 16 2019, 2:57 AM
hokein added inline comments.
clang-tools-extra/clangd/refactor/Rename.cpp
77

I think so, that would be safer (this should be done in a separate patch), it seems that some cases are being handled by the rename library already.

  • for constructor and destructor, the renamelib handle them well;
  • for operator int()/user-defined literals, the renamelib doesn't find a corresponding decl, means that we will return "no symbol found error", though the message is a bit confusing;
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.