[PATCH] D116502: [clangd] Helper for determining member insertion point.

[Sam McCall via Phabricator via cfe-commits]

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