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