[PATCH] D72484: [clang-tidy] Fix check for Abseil internal namespace access

Fri Jan 17 10:42:06 PST 2020

EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

LGTM.

The weird template test case might is derived from an old failure that I believe was addressed in Clang, but regression tests are fine by me.
The macro expansion fix is correct and correctly tested.

================
Comment at: clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp:43-44
+
+  if (!LocAtFault.isValid())
+    return;
+
----------------
rogeeff wrote:
> aaron.ballman wrote:
> > rogeeff wrote:
> > > lebedev.ri wrote:
> > > > Is there test coverage for this? When does/can this happen?
> > > At the time when I wrote this internally the test cases in abseil-no-internal-dependencies.cpp were reproducing this failure. I'm not sure this is still the case. It is possible compiler was fixed since then.
> > I am not certain I'm following along (sorry if I'm just being dense). Are you saying that the existing test coverage in abseil-no-internal-dependencies.cpp was failing for you internally, and that's the reason for this fix? Or are you saying that the newly-added test cases in this patch were triggering this failure?
> Newly added test cases were triggering the failure.
@lebedev.ri This also happened when the source location was inside a macro expansion. Which is solved by getting the spelling loc, and there is a test case that covers it.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72484/new/

https://reviews.llvm.org/D72484