Page MenuHomePhabricator

[clangd] Define out-of-line initial apply logic

Authored by kadircet on Oct 22 2019, 3:54 AM.



Initial implementation for apply logic, replaces function body with a
semicolon in source location and copies the full function definition into target

Will handle qualification of return type and function name in following patches
to keep the changes small.

Diff Detail

Event Timeline

kadircet created this revision.Oct 22 2019, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2019, 3:54 AM
hokein added inline comments.Oct 23 2019, 5:24 AM

the function name doesn't reflect what it does, it doesn't move the function, it just returns the source code of the function, I'd call it some name like getFunctionSourceCode.


The FileName is not always absolute, getCorrespondingHeaderOrSource is expecting an absolute file path input.


could we use a better name, Source in the context here has two different meaning: 1) the corresponding .cc file 2) the source of moving function declaration (we called it Source);

Maybe call it CCFile?


maybe just const tooling::Replacement InsertFunctionDef(Contents, *InsertionOffset, 0, *FunDef);, which would save us a InsertLoc.


nit: I think CharSourceRange::getCharRange(Source->getBody()->getSourceRange()) is clearer.


thinking more about this, if this function is inline, we will leave an unnecessary inline keyword after running the code action. No need to address it in the patch, just keep in mind.

can we add more tests? e.g. template functions.

kadircet marked 7 inline comments as done.Oct 23 2019, 7:06 AM
kadircet added inline comments.

added a fixme for inline stuff.

kadircet updated this revision to Diff 226134.Oct 23 2019, 7:06 AM
kadircet marked an inline comment as done.
  • Address comments
  • Handle template parameters when copying function and add tests
hokein added inline comments.Oct 28 2019, 5:47 AM

Could you confirm the code handle template specializations as well? I think getDescribedFunctionTemplate will return null when FD is a specialization, and we still miss the template list.


I think we could simplify the code like:

const auto* TargetFD = FD->getDescribedFunctionTemplate() ? FD->getDescribedFunctionTemplate(): FD;
auto CharRange = toHaleOpenFileRange(.., FD->getSourceRange());

should we use getCanonicalPath in the SourceCode.h?


nit: maybe put the code for calculating the insertion point to a separate function.

kadircet updated this revision to Diff 227108.Oct 30 2019, 8:17 AM
kadircet marked 5 inline comments as done.
  • Address comments

added a unittest

kadircet updated this revision to Diff 230437.Nov 21 2019, 5:33 AM
  • Get rid of raw string literals inside macros
hokein accepted this revision.Nov 25 2019, 4:33 AM

looks good.


this function seems trivial, I'd inline them into call sides for both define inline and outline.

This revision is now accepted and ready to land.Nov 25 2019, 4:33 AM
kadircet updated this revision to Diff 231382.Nov 28 2019, 2:21 AM
kadircet marked an inline comment as done.
  • Address comments
  • Better handling for macros and tests

Build result: FAILURE - Could not check out parent git hash "6a2d56e54fa4a2009787a605607b0df7fe16dd98". 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

This revision was automatically updated to reflect the committed changes.

ApplyTest fails on Windows; probably the usual delayed template parsing thing: