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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
