Page MenuHomePhabricator

[clangd] Delete ctor initializers while moving functions out-of-line
ClosedPublic

Authored by kadircet on Dec 9 2019, 2:56 AM.

Details

Summary

Currently we only delete function body from declaration, in addition to
that we should also drop ctor initializers.

Unfortunately CXXConstructorDecl doesn't store the location of : before
initializers, therefore we make use of token buffer to figure out where to start
deletion.

https://github.com/clangd/clangd/issues/220

Diff Detail

Event Timeline

kadircet created this revision.Dec 9 2019, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 2:56 AM

Build result: FAILURE - Could not check out parent git hash "8d52e0ffa278d9047731523cc72ce605f8f336df". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

kadircet edited the summary of this revision. (Show Details)Dec 9 2019, 2:59 AM
hokein added inline comments.Dec 9 2019, 5:04 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
277

There is an alternative here, I think. The end loc of FunctionProtoTypeLoc should point the location after the ), e.g. the Foo()^ : , would it be easier to use it as as the InitStart?

it can be retrieved by CD->getTypeSourceInfo()->getTypeLoc().getEndLoc().

kadircet marked an inline comment as done.Dec 9 2019, 5:47 AM
kadircet added inline comments.
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
277

yes, that would be great if attributes didn't exist. e.g.:

cpp
class Foo {
  Foo() __attribute__((weak)) : z(2) {}
  int z;
};

for the example given above, typeloc.endloc would not include trailing attributes, updated test case according to that.(And added a fixme, since we should not propagate those attributes into definition)

also the approach proposed in this patch would result in minimal changes, by keeping anything before : in declaration site.
if we were to make use of endloc we could end up deleting more tokens in between, even if it would be rare to have tokens in between.

kadircet updated this revision to Diff 232825.Dec 9 2019, 5:47 AM
  • Address comments

Build result: fail - 60625 tests passed, 1 failed and 726 were skipped.

failed: LLVM.CodeGen/RISCV/mir-target-flags.ll

Log files: console-log.txt, CMakeCache.txt

hokein accepted this revision.Dec 9 2019, 7:22 AM
hokein added inline comments.
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
271

nit: the same, llvm::find_if.

277

ah, I see, that makes sense.

This revision is now accepted and ready to land.Dec 9 2019, 7:22 AM
kadircet updated this revision to Diff 232877.Dec 9 2019, 9:43 AM
kadircet marked an inline comment as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.

Build result: pass - 60655 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt