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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
