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