[PATCH] D59528: [clang-tidy] Expand modular headers for PPCallbacks

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.h:156
+  virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                                   Preprocessor *ModuleExpanderPP) {}
+
----------------
Please document that `ModuleExpanderPP` also provides events from the main file (until I read the implementation in the check, I thought the check should subscribe to both preprocessors).


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:19
+public:
+  // Stores FileEntry for which contents are to be recorded later.
+  void addFileToRecord(const FileEntry *File) { FilesToRecord.insert(File); }
----------------
Three slashes in doc comments (everywhere in the patch).


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:20
+  // Stores FileEntry for which contents are to be recorded later.
+  void addFileToRecord(const FileEntry *File) { FilesToRecord.insert(File); }
+
----------------
"Records that a given file entry is needed for replaying callbacks."

"addNecessaryFile"?


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:23
+  // Records content for a file and adds it to the FileSystem.
+  void recordContent(const FileEntry *File,
+                     const SrcMgr::ContentCache *ContentCache,
----------------
"recordFileContent"?


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:24
+  void recordContent(const FileEntry *File,
+                     const SrcMgr::ContentCache *ContentCache,
+                     llvm::vfs::InMemoryFileSystem &InMemoryFs) {
----------------
`const &` for `File` and `ContentCache` ?


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:38
+                           ContentCache->getRawBuffer()->getBuffer()));
+    // Remove file since we have successfully recorded its contents.
+    FilesToRecord.erase(File);
----------------
"remove the file from the set of necessary files..."


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:71
+
+  // Switch of header modules in the new preprocessor.
+  LangOpts.Modules = false;
----------------
of => off

However I don't see value in a comment that repeats the source code.


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:115
+        auto *F = IF.getFile();
+        Recorder->addFileToRecord(F);
+      });
----------------
`Recorder->addFileToRecord(IF.getFile())`

The intermediate variable doesn't add anything...


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:171
+// Just parse to the corresponding location to generate the same callback for
+// the target_callbacks_.
+void ExpandModularHeadersPPCallbacks::Ident(SourceLocation Loc, StringRef) {
----------------
What's `target_callbacks_`?  It is only mentioned in comments.


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h:9
+//
+// Defines the ExpandModularHeadersPPCallbacks class that handles #includes of
+// modular headers, traverses all transitively included headers in non-modular
----------------
This looks like a documentation comment for the class itself, I think it should be moved there.


================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h:43
+
+  /// \brief Users can get expanded PPCallbacks by registering their callback
+  /// handlers in the preprocessor instance returned by this method.
----------------
"Returns the preprocessor that provides callbacks for contents of modular headers.

This preprocessor is separate from the one used by the rest of the compiler."



================
Comment at: clang-tools-extra/test/clang-tidy/expand-modular-headers-ppcallbacks.cpp:31
+// CHECK-MESSAGES: c.h:2:9: warning: invalid case style for macro definition 'c' [readability-identifier-naming]
+// CHECK-MESSAGES: c.h:2:9: note: FIX-IT applied suggested code changes
----------------
Please also add a check for a diagnostic coming from the main file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59528