njames93 marked 6 inline comments as done.
njames93 added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:217
bool AllowEnablingAnalyzerAlphaCheckers;
+ bool SelfContainedDiags;
};
----------------
steveire wrote:
> The order of members here is starting to look suspicious, size-wise. Not
> something for this change though.
Not something I'm prepared to worry about in this patch.
================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp:94
+ : IncludeInserterCheckBase(CheckName, Context,
+ utils::IncludeSorter::IS_Google, true) {}
+
----------------
steveire wrote:
> I think the `true` here points to this being a boolean trap. Consider using
> an enum for the parameter instead.
I don't see a point in defining an enum for this when its only a test case,
though a parameter comment should be added.
================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp:285
+#include "path/to/header2.h"
+#include "path/to/header.h"
+
----------------
steveire wrote:
> 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"?
The name of the test is `InsertMultipleIncludesNoDeduplicate`, Is that not
sufficient?
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
