[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h:1
+#if !defined(READABILITY_DUPLICATE_INCLUDE_H)
+#define READABILITY_DUPLICATE_INCLUDE_H
----------------
carlosgalvezp wrote:
> Nit: `#ifndef` 
It's a style thing.  I don't prefer `#ifndef` because it only differs from `#ifdef` by a single letter.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h:1
+// empty file used by readability-duplicate-include.cpp
----------------
carlosgalvezp wrote:
> Nit: write "This file is intentionally empty" for consistency with the other files you added? It's already clear from the folder structure that this file is an input for the `readability-duplicate-include` test.
Oops, yeah, meant to do that with all of them and missed this one.
When I first authored this check 7 years ago, they didn't have the Inputs
folder.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp:19
+int f;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include
+// CHECK-FIXES:      {{^int d;$}}
----------------
carlosgalvezp wrote:
> Nit: it would be good to keep `[readability-duplicate-include]` part of the warning consistent - on Line 8 you have it but not on the other lines. So either keep it for all warnings or remove it from line 8.
`CHECK-MESSAGES` will match whatever substring you provide.  To make
the test file less noisy, I do a full match on the entire diagnostic for the
first warning and then just the necessary bits for the remaining checks.


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

https://reviews.llvm.org/D7982