[PATCH] D68024: [clangd] Implement GetEligiblePoints

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 5 inline comments as done.
kadircet added inline comments.


================
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:
> 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)
dropping the const


================
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
----------------
ilya-biryukov wrote:
> 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.
agreed, my suggestion was also just for figuring out when a `{` might start a definition, and trigger an event at the corresponding `}`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68024