[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check
Carlos Galvez via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 20 10:32:08 PST 2022
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
More information about the cfe-commits
mailing list