[PATCH] D26752: [include-fixer] Refactor include fixer to be usable as a plugin

[Haojian Wu via cfe-commits]

hokein added inline comments.


================
Comment at: include-fixer/IncludeFixer.cpp:64
   getIncludeFixerContext(const clang::SourceManager &SourceManager,
                          clang::HeaderSearch &HeaderSearch) {
+    return SemaSource.getIncludeFixerContext(SourceManager, HeaderSearch);
----------------
also make it const?


================
Comment at: include-fixer/IncludeFixer.cpp:124
+  auto Reps = createIncludeFixerReplacements(Code, Context,
+                                             format::getLLVMStyle(), false);
+  if (!Reps)
----------------
nit: `/*AddQualifiers=*/false`.


================
Comment at: include-fixer/IncludeFixer.cpp:136
+
+    auto Begin = StartOfFile.getLocWithOffset(Placed.getOffset());
+    auto End = Begin.getLocWithOffset(Placed.getLength());
----------------
I have a concern that `Placed` here might be not the replacement for inserting the new header, becuase the `Reps` returned from `createIncludeFixerReplacements` may have some replacements for cleanup.

To make it more correct, maybe we can check whether `Placed.getReplacementText()` is equal to `"#include" + Context.getHeaderInfos().front().Header`?


================
Comment at: include-fixer/IncludeFixer.cpp:283
+  if (GenerateDiagnostics) {
+    FileID FID = CI->getSourceManager().getFileID(Typo.getLoc());
+    StringRef Code = CI->getSourceManager().getBufferData(FID);
----------------
You can use `SM` directly here since you have created a `SM` variable for `SourceManager` above? The same below.


================
Comment at: include-fixer/IncludeFixer.h:119
+  IncludeFixerContext
+  getIncludeFixerContext(const clang::SourceManager &SourceManager,
+                         clang::HeaderSearch &HeaderSearch);
----------------
Make it a `const` member function?


================
Comment at: include-fixer/IncludeFixer.h:124
+  /// Query the database for a given identifier.
+  bool query(StringRef Query, StringRef ScopedQualifiers, tooling::Range Range);
+  CompilerInstance *CI;
----------------
A blank line below.


================
Comment at: include-fixer/plugin/IncludeFixerPlugin.cpp:1
+//===- ClangTidyPlugin.cpp - clang-tidy as a clang plugin -----------------===//
+//
----------------
nit: IncludeFixerPlugin


================
Comment at: include-fixer/plugin/IncludeFixerPlugin.cpp:91
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the clang-tidy plugin.
+volatile int ClangIncludeFixerPluginAnchorSource = 0;
----------------
nit: clang-tidy=>include-fixer


https://reviews.llvm.org/D26752