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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Looks good! I need to familiarize myself better with the `Loc` manipulation code to be able to review the free-standing function but otherwise looks reasonable.



================
Comment at: clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp:22
+
+SourceLocation advanceBeyondCurrentLine(const SourceManager &SM,
+                                        SourceLocation Start, int Offset) {
----------------
Move out of anonymous namespace and use `static`, as per LLVM guidelines.


================
Comment at: clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp:64
+    SrcMgr::CharacteristicKind FileType) {
+  if (!SM.isInMainFile(HashLoc)) {
+    return;
----------------
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?


================
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"
+
----------------
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
```


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

https://reviews.llvm.org/D7982