PiotrZSL requested changes to this revision.
PiotrZSL added a comment.
This revision now requires changes to proceed.
First this looks like "misc" check, not as "readability" one. So please move it
to proper module. There is no "readability" aspect of it.
Second thing this is not an "absolute", absolute would be #include
"/usr/includes/stdio", with this style you can still use relative paths like
#include <../something>
Proper names then would be one of: misc-no-quote-includes or
misc-use-angle-bracket-includes (this one looks to sound better), choose one,
any.
and again naming, those are not relative and absolute, those are quote and
angle-bracket, avoid confusion with path put into include.
================
Comment at:
clang-tools-extra/clang-tidy/readability/AbsoluteIncludesOnlyCheck.cpp:43
+
+void AbsoluteIncludesOnlyPPCallbacks::InclusionDirective(
+ SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
----------------
inline this method in AbsoluteIncludesOnlyPPCallbacks no need to keep it out
of class.
================
Comment at:
clang-tools-extra/clang-tidy/readability/AbsoluteIncludesOnlyCheck.cpp:48
+ SrcMgr::CharacteristicKind FileType) {
+ if (IncludeTok.getIdentifierInfo()->getPPKeywordID() == tok::pp_import)
+ return;
----------------
i think that `Imported!=nullptr` should also do a trick, but this way shoud be
fine. Check if getIdentifierInfo is not null to be sure
================
Comment at:
clang-tools-extra/clang-tidy/readability/AbsoluteIncludesOnlyCheck.cpp:51
+
+ SourceLocation DiagLoc = FilenameRange.getBegin().getLocWithOffset(0);
+ if (!IsAngled) {
----------------
`getLocWithOffset(0)` does nothing, remove
================
Comment at:
clang-tools-extra/clang-tidy/readability/AbsoluteIncludesOnlyCheck.cpp:53-54
+ if (!IsAngled) {
+ Check.diag(DiagLoc, "relative include found, use only absolute includes",
+ DiagnosticIDs::Warning);
+ }
----------------
1. You could provide auto-fix here, but if you do not want to do that its fine.
2. Warning is a default, no need to specify it.
3. put included header name into message, it will make way easier for reader to
find out whats going on
4. consider excluding includes included from system headers on this level,
otherwise check may generate lot of includes that going to be filter out later
in process, but that still may be costly.
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:188
+Finds relative includes in your code and warn about them. for example
+don't use "readability/absolute-includes-only", use
<clang-tidy/checks/readability/absolute-includes-only>
+
----------------
sounds like "do not eat cheese, eat cheese", align later this documentation
with check name
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst:11
+This is relevant for the canonical project structure specified in paper
p1204r0.
+The relevant part is the src-dir where relative includes are discussed:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html#src-dir
+
----------------
format this url properly, look into other checks how they do it in documentation
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst:11
+This is relevant for the canonical project structure specified in paper
p1204r0.
+The relevant part is the src-dir where relative includes are discussed:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html#src-dir
+
----------------
PiotrZSL wrote:
> format this url properly, look into other checks how they do it in
> documentation
80 characters limit
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst:17
+
+ // #include "utility.hpp" // Wrong.
+ // #include <utility.hpp> // Wrong.
----------------
uncomment those includes
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability/absolute-includes-only.rst:18
+ // #include "utility.hpp" // Wrong.
+ // #include <utility.hpp> // Wrong.
+ // #include "../hello/utility.hpp" // Wrong.
----------------
whats wrong here ? check wont raise warning here
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152589/new/
https://reviews.llvm.org/D152589
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
