[PATCH] D152589: [clang-tidy] Add readability test for not allowing relative includes

[Piotr Zegar via Phabricator via cfe-commits]

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