[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:153
+  // Get new begin and end positions for the qualified body.
+  auto OrigFuncRange = toHalfOpenFileRange(
+      SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());
----------------
kadircet wrote:
> hokein wrote:
> > 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?
> 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.
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.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1810
+        })cpp",
+       "a::Foo::Bar foo() { return {}; }\n"},
+      {R"cpp(
----------------
kadircet wrote:
> hokein wrote:
> > 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).
> 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
ah, you are right. I think I was somehow confused with the `a::Foo::Bar foo()`.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2007
+            Bar foo() ;
+          }
+        })cpp",
----------------
nit: ";"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70535/new/

https://reviews.llvm.org/D70535