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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
