[PATCH] D83680: [clang-tidy] Refactor IncludeInserter

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp:55
 IncludeSorter &IncludeInserter::getOrCreate(FileID FileID) {
+  assert(SourceMgr && "SourceMgr shouldn't be null");
   // std::unique_ptr is cheap to construct, so force a construction now to save
----------------
I think it might be useful to add something like `; did you remember to call registerPreprocessor()?` to the assertion message, as that seems like it would be the most likely issue if `SourceMgr` was null.


================
Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.h:59
+  /// Initializes the IncludeInserter using the IncludeStyle \p Style.
+  /// In most cases the \p Style will be retrived from the ClangTidyOptios using
+  /// \code
----------------
retrived -> retrieved
ClangTidyOptios -> ClangTidyOptions 


================
Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.h:70
+  /// Creates a \p Header inclusion directive fixit in the File \p FileID.
+  /// Returns ``llvm::None`` on error or if inclusion directive already exists.
+  llvm::Optional<FixItHint>
----------------
or if inclusion -> or if the inclusion


================
Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.h:75
+  /// Creates a \p Header inclusion directive fixit in the main file.
+  /// Returns``llvm::None`` on error or if inclusion directive already exists.
   llvm::Optional<FixItHint>
----------------
or if inclusion -> or if the inclusion


================
Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.h:89
+  llvm::DenseMap<FileID, llvm::StringSet<>> InsertedHeaders;
+  const SourceManager *SourceMgr;
   const IncludeSorter::IncludeStyle Style;
----------------
In-class initialize this to nullptr instead of leaving it indeterminate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83680