ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM, thanks.
A ton of small NITs too, but they're all small details.
================
Comment at: clang-tools-extra/clangd/Macro.h:1
+//===--- Macro.h -------------------------------------------------*-
C++-*-===//
+//
----------------
NIT: Maybe rename to `CollectMacros.h`?
`Macro.h` looks a bit too generic and may start being a place for more helpers.
However, the current file is pretty well-structured.
================
Comment at: clang-tools-extra/clangd/Macro.h:23
+ llvm::StringSet<> Names;
+ std::vector<Range> Ranges;
+};
----------------
Could you add a comment that we have to convert to `Range` early because source
manager from preamble is not available when we build the AST?
================
Comment at: clang-tools-extra/clangd/Macro.h:26
+
+// Collects macro definitions and expansions in the main file. it is used to:
+// - collect macros in the preamble section of the main file (in
Preamble.cpp)
----------------
NIT: a typo: in the main file. **It** is used ...
================
Comment at: clang-tools-extra/clangd/Macro.h:28
+// - collect macros in the preamble section of the main file (in
Preamble.cpp)
+// - collect macros after the preamble of the main file (in ParsedAST.cpp)
+class CollectMainFileMacros : public PPCallbacks {
----------------
NIT: use `///` to enable doxygen comments
================
Comment at: clang-tools-extra/clangd/Macro.h:65
+ const LangOptions &LangOpts;
+ bool InMainFile = true;
+ MainFileMacros &Out;
----------------
NIT: It's probably safer to set 'no' by default.
Normally the first thing visited by the parser is `<builtin>` buffer, not the
main file.
OTOH, `FileChanged` should be called first in any case, so it does not matter.
================
Comment at: clang-tools-extra/clangd/ParsedAST.h:104
std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
- std::vector<SourceLocation> MainFileMacroExpLocs,
+ MainFileMacros MainFileMacroExpLocs,
std::vector<Decl *> LocalTopLevelDecls, std::vector<Diag> Diags,
----------------
NIT: rename parameter to `Macros`
================
Comment at: clang-tools-extra/clangd/ParsedAST.h:124
- /// The start locations of all macro definitions/expansions spelled **after**
- /// preamble.
- /// Does not include locations from inside other macro expansions.
- std::vector<SourceLocation> MacroIdentifierLocs;
+ /// All macro definitions/expansions in the main file.
+ MainFileMacros Macros;
----------------
NIT: change `definitions/expansions` to `definitions and expansion`
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:34
- std::vector<std::string> takeMainFileMacros() {
- return std::move(MainFileMacros);
- }
+ MainFileMacros takeMainFileMacros() { return std::move(Macros); }
----------------
NIT: rename to `takeMacros`
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:73
+ const clang::LangOptions *LangOpts = nullptr;
SourceManager *SourceMgr = nullptr;
};
----------------
NIT: add **const** to `SourceManager` too, for consistency
(if possible)
================
Comment at: clang-tools-extra/clangd/Preamble.h:61
+
+ PreambleData(PrecompiledPreamble Preamble, std::vector<Diag> Diags,
+ IncludeStructure Includes, MainFileMacros Macros,
----------------
There seems to be no reason for moving constructor to the bottom of the class.
Maybe keep as is?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67496/new/
https://reviews.llvm.org/D67496
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
