[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

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