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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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());
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1778
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
----------------
hokein wrote:
> can't we merge these into the above `ApplyTest`?
I would rather keep these separate, as these tests tends to get out of control otherwise, e.g. `Hover.All`


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1810
+        })cpp",
+       "a::Foo::Bar foo() { return {}; }\n"},
+      {R"cpp(
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535