Page MenuHomePhabricator

[clangd] Define out-of-line qualify return value
ClosedPublic

Authored by kadircet on Nov 21 2019, 5:18 AM.

Details

Summary

Return type might need qualification if insertion context doesn't have
the same decls visible as the source context.

This patch adds qualification for return value to cover such cases.

Diff Detail

Event Timeline

kadircet created this revision.Nov 21 2019, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2019, 5:18 AM
kadircet updated this revision to Diff 230841.Nov 25 2019, 12:32 AM
  • Move TargetContext generation logic to a separate function

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

hokein added inline comments.Nov 29 2019, 2:11 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
80

could you add some documentations, e.g. what's the requirement for the input TargetNS?

I'm not sure Synthesize is clear here, maybe lookupTargetContext or findTargetContext?

88

nit: maybe use early return?

116

could you also update the comments here, mentioning the function handles the return type qualifiers properly?

127

could we use a more meaningful name, maybe AddQualifierEdit? Would be nice to have some comments explaining what the following code does.

159

nit: I think this error message is exposed to users, I'm not sure "see logs for details" is friendly to them.

163

we have similar code in define-inline as well, would be nice to have them in a single place in the long term. probably a FIXME?

182–183

btw, do we need to qualify the function parameters. maybe it is not needed, but would be nice to have some brief comments explaining it.

clang-tools-extra/clangd/unittests/TweakTests.cpp
1778

can't we merge these into the above ApplyTest?

1810

oh, this is very tricky case (I think you meant to test the public members), note that Bar and foo are private member of Foo, we can't move the body out of the class Foo, for this case I think we should disallow the tweak.

No need to do it in this patch, but please update the test here (to test public members).

kadircet updated this revision to Diff 231511.Nov 29 2019, 3:01 AM
kadircet marked 9 inline comments as done.
  • Address comments
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
163

they're quite similar but, different in nature. one of them returns the full function definition, including template parameter lists, whereas the other only operates on function body.

clang-tools-extra/clangd/unittests/TweakTests.cpp
1778

I would rather keep these separate, as these tests tends to get out of control otherwise, e.g. Hover.All

1810

I don't follow, the following compiles nicely:

namespace a {
class Foo {
  class Bar {};
  Bar foo();
};
}  // namespace a

a::Foo::Bar a::Foo::foo() { return {}; }

the problem here is we are not qualifying the function name, which is handled in the follow up patch D70656

hokein accepted this revision.Dec 2 2019, 12:25 PM
hokein added inline comments.
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
163

yes, the only difference is the range, right? the logic of applying replacements, getting correct begin/end offset, and getting the interesting content is the same. we could abstract a function that taking the range into the parameter.

clang-tools-extra/clangd/unittests/TweakTests.cpp
1808

nit: ";"

1810

ah, you are right. I think I was somehow confused with the a::Foo::Bar foo().

This revision is now accepted and ready to land.Dec 2 2019, 12:25 PM
kadircet updated this revision to Diff 231866.Dec 3 2019, 4:10 AM
kadircet marked 5 inline comments as done.
  • Extract replacement applying part to a new function and add a fixme for sharing it with define inline code-action.

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

failed: Clangd Unit Tests._/ClangdTests/DefineOutlineTest.QualifyReturnValue

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

kadircet updated this revision to Diff 232036.Dec 3 2019, 11:19 PM
  • Fix tests

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

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

This revision was automatically updated to reflect the committed changes.