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