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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a comment.

LGTM! Had some nits that can be fixed without review.

Can't see anything else major that needs change. As said I'm fairly new here so probably a more experienced reviewer can find some more improvements/optimizations, especially on the `Loc` aspects. On the other hand I think after 6 years it's about time to get this in, it can always be improved afterwards :)



================
Comment at: clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp:84
+  if (llvm::find(Files.back(), FileName) != Files.back().end()) {
+    SourceLocation Start =
+        advanceBeyondCurrentLine(SM, HashLoc, -1).getLocWithOffset(-1);
----------------
Nit: as a n00b I would have appreciated a quick comment about why we can't directly use the `HashLoc` and `FilenameRange` as `Start` and `End` locations. Took me a while to understand this is to cover these cases:

`           #include "foo.h"`                     -> needs updated `Start`
`#include "bar.h"   // Bar is needed!`  -> needs updated `End`




================
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
----------------
Nit: `#ifndef` 


================
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
----------------
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.


================
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;$}}
----------------
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.


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

https://reviews.llvm.org/D7982