aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM



================
Comment at: 
clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp:43-44
+
+  if (!LocAtFault.isValid())
+    return;
+
----------------
EricWF wrote:
> 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.
> Newly added test cases were triggering the failure.

Thanks for the clarification!


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

https://reviews.llvm.org/D72484



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to