[PATCH] D68024: [clangd] Implement GetEligiblePoints

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

A few go-by comments from me, Haojian should have better context here!



================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:655
+lex(llvm::StringRef Code, const format::FormatStyle &Style,
+    llvm::function_ref<void(const clang::Token &, const Position)> A) {
   // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
----------------
Yay! Thanks! Less templates, more type-checking!

Could you rename 'A' to something more specific, though?
Not that there is no nice type name to guide the readers, `A` feels a bit confusing.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:655
+lex(llvm::StringRef Code, const format::FormatStyle &Style,
+    llvm::function_ref<void(const clang::Token &, const Position)> A) {
   // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
----------------
ilya-biryukov wrote:
> Yay! Thanks! Less templates, more type-checking!
> 
> Could you rename 'A' to something more specific, though?
> Not that there is no nice type name to guide the readers, `A` feels a bit confusing.
why `const Position` and not `const Position&`?
top-level const in function parameters is weird, it's basically ignored (except at the definition site)


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:674
   llvm::StringMap<unsigned> Identifiers;
-  lex(Content, Style, [&](const clang::Token &Tok) {
+  lex(Content, Style, [&](const clang::Token &Tok, const Position) {
     switch (Tok.getKind()) {
----------------
Should this be `const Position&` or `Position`?


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:996
+  std::vector<std::string> Enclosing = {""};
+  // FIXME: In addition to namespaces try to generate events for function
+  // definitions as well. One might use a closing parantheses(")" followed by an
----------------
If we were to do this, I'd rather try matching brace structure and only report `closing brace that is probably end of the top-level decl(function, struct, etc)` events.

Everything else seems super hard to get right.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:270
+  /// It will be “a::b” for both carrot locations.
+  std::string CurrentNamespace;
+  /// Offsets into the code marking eligible points to insert a function
----------------
Do we ever name anonymous namespace here? How do we represent them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68024