[PATCH] D97121: [clang-tidy] Add a single fix mode to clang-tidy

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp:81
+    if (InsertedHeaders[FileID].contains(Header))
+      return llvm::None;
+  } else if (!InsertedHeaders[FileID].insert(Header).second)
----------------
njames93 wrote:
> steveire wrote:
> > The comment and the code seem to me to be opposites. The code says "if `SingleFixMode` is true and we have already inserted the header,  don't insert it again." The comment says that "`SingleFixMode` does not prevent inserting it again" -- The comment seems to be the opposite of the code?
> That't not what the code is saying, the magic is clear in the else branch. If SingleFixMode is enabled we are only querying the set, If its disabled then we are updating the set. 
> The real shortfall is the map is called InsertedHeaders which is not a good reflection of what it actually contains. It actually contains all the headers included in each file. Which in `Multi` fix mode gets updated with each include insertion.
Ah, the "and we have already inserted the header, don't insert it again" is not true because we never do an `insert` in `SingleFixMode`.

Perhaps update the comment to say that the InsertedHeaders is never added to in `SingleFixMode`, so it only contains the headers that are already there? If something like that is true.



================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp:94
+      : IncludeInserterCheckBase(CheckName, Context,
+                                 utils::IncludeSorter::IS_Google, true) {}
+
----------------
I think the `true` here points to this being a boolean trap. Consider using an enum for the parameter instead.


================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp:285
+#include "path/to/header2.h"
+#include "path/to/header.h"
+
----------------
I still find it really confusing that the "single inserter" mode results in multiple of the same header being added.  Perhaps the names should be along the lines of "duplicating" and "deduplicating" instead of "single" and "multi"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97121