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