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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp:64
+    SrcMgr::CharacteristicKind FileType) {
+  if (!SM.isInMainFile(HashLoc)) {
+    return;
----------------
carlosgalvezp wrote:
> I'm not familiar with `isInMainFile`, will the check work if the duplicated include is in a header included by the main .cpp file? Should a test be added for that?
Good catch.  I originally wrote this check in 2015 (yes, that's
how long it's been waiting for a review) and recently revived it.
At the time, I didn't understand that checks could apply fixits to
headers and that user headers could be differentiated from system
headers, so this is a mistake.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst:34-47
+  class LangOptionsBase {
+  public:
+    // Define simple language options (with no accessors).
+  #define LANGOPT(Name, Bits, Default, Description) unsigned Name : Bits;
+  #define ENUM_LANGOPT(Name, Type, Bits, Default, Description)
+  #include "clang/Basic/LangOptions.def"
+
----------------
carlosgalvezp wrote:
> As an external user that is not deeply involved in the details of LLVM, I find this example rather complicated. Would it be possible to use a simpler example? What about this one with `cassert`
> 
> ```
> #undef NDEBUG
> #include <cassert>
> 
> // ... code with assertions enabled
> 
> #define NDEBUG
> #include <cassert>
> 
> // ... code with assertions disabled
> ```
Good idea and I like your example better.


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

https://reviews.llvm.org/D7982