[PATCH] D117409: [clang-tidy] Move IncludeInserter into ClangTidyContext

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:220-238
+llvm::Optional<FixItHint>
+ClangTidyCheck::createMainFileIncludeInsertion(StringRef Include) {
+  if (!Context->hasIncludeInserter()) {
+    // Only crash on debug builds
+    assert(false && "No IncludeInserter registered");
+    return llvm::None;
+  }
----------------
njames93 wrote:
> LegalizeAdulthood wrote:
> > Law of Demeter.  Move this to `ClangTidyContext` and let it handle the details.
> Are you suggesting I create methods for createIncludeInsertion in the ClangTidyContext?
Yes, exactly.  Delegate everything to the Context object, instead of
grabbing a piece of data (the include inserter) our of the Context
object and manipulating it.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:128-131
+  llvm::Optional<FixItHint> createMainFileIncludeInsertion(StringRef Include);
+  llvm::Optional<FixItHint> createIncludeInsertion(FileID File,
+                                                   StringRef Include);
+
----------------
Add doxygen comments, please


================
Comment at: clang-tools-extra/clang-tidy/IncludeInserter.h:20
 namespace tidy {
-namespace utils {
 
----------------
njames93 wrote:
> LegalizeAdulthood wrote:
> > What's the guidance on what belongs in `clang::tidy`
> > and what belongs in `clang::tidy::utils`?
> Well as the file moved out of utils, its no longer in the utils namespace.
Yes, if you move the file out of utils then it shouldn't be in the
utils namespace.  But why move the file in the first place?

IOW, what is the guideline for what files belong under `utils`
and what files belong under `clang-tidy`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117409