sammccall marked 2 inline comments as done.
sammccall added a comment.
In D116502#3217084 <https://reviews.llvm.org/D116502#3217084>, @kadircet wrote:
> We have some logic in AddUsing tweak to determine insertion point based on
> AST. i think it makes sense to migrate it to these helpers too.
Thanks, I'd forgotten about those. I might tackle those next?
> There's some more logic in extract variable/function too. Extract variable
> seems too elaborate as it actually looks at statements, rather than decls and
> extract function is quite simple already as we already figure out current
> function's range and whatnot, but having another usage might help.
Yeah I wasn't sure I could do a good job of generalizing these ones yet (and we
haven't needed it yet).
> We've got some helpers in SourceCode.h to determine insertion point in the
> absence of ASTs. Concepts here and there around an "insertion point" seems to
> be quite different (it's just a sourcelocation here and a set of locations +
> a namespace in SourceCode.h).
> I suppose those two are somewhat hard to merge and serve different purposes,
> so It's better to keep them separate.
For sure ast-based and pseudoparsing-based cases are going to be different
APIs, but I would like to move those into this header too.
The problem I hit was they share private infrastructure (in SourceCode.cpp)
with other functionality (the namespace pseudoparsing for completion, I think).
So it's a bit of work to extract.
================
Comment at: clang-tools-extra/clangd/refactor/InsertionPoint.cpp:134
+ // Fallback: insert at the end of the class. Check if protection matches!
+ if (Loc.isInvalid()) {
+ Loc = InClass.getBraceRange().getEnd();
----------------
kadircet wrote:
> what if we had:
> ```
> class Foo {
> public:
> void foo();
> };
> ```
>
> and wanted to insert a `private` member/field?
> I suppose we should check for the specifier of last decl in `InClass` instead.
Oops, good catch.
In that particular case, you could make a case for inserting at the very top of
the class (no access specifier needed). But some coding styles want the
opposite.
I think using the presence/absence of decls in the chunk before the first
access specifier is a reasonable hint, and it happens to be the easiest thing
to implement :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116502/new/
https://reviews.llvm.org/D116502
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
