[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+  // TODO(hugoeg): refactor matcher to be configurable or just match on any internal access from outside the enclosing namespace.
+  
----------------
JonasToth wrote:
> Nit: This comment is very long, pls break the line
Also, we don't add developer names to comments.


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:39-41
+       "depends upon internal implementation details, which violates the "
+       "Abseil compatibilty guidelines. See "
+       "https://abseil.io/about/compatibility");
----------------
Diagnostic text is not supposed to be grammatically correct with capitalization and punctuation. Please do not put links into diagnostics. It's more appropriate for this to be in the documentation. I think this might be improved wording: `do not reference the 'internal' namespace; those implementation details are reserved to Abeil` or something along those lines.


================
Comment at: clang-tidy/abseil/NoInternalDepsCheck.h:19-21
+/// Finds instances where the user depends on internal details and warns them
+/// against doing so.
+/// This check should not be run on internal Abseil files or Abseil source code.
----------------
Can you re-flow these comments?


================
Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:6
+
+Gives a warning if code using Abseil depends on internal details. If something is in a namespace or filename/path that includes the word “internal”, code is not allowed to depend upon it beaucse it’s an implementation detail. They cannot friend it, include it, you mention it or refer to it in any way. Doing so violtaes Abseil's compatibility guidelines and may result in breakage.
+
----------------
JonasToth wrote:
> s/violtaes/violates/
Please wrap lines to 80 cols.

Nothing in this check looks at filenames and paths, but this suggests the check will find those. Is that intended work that's missed in this patch, or are the docs incorrect?


https://reviews.llvm.org/D50542