Page MenuHomePhabricator

[clang-itdy] Simplify virtual near-miss check
ClosedPublic

Authored by steveire on Feb 7 2021, 11:20 AM.

Details

Summary

Diagnose the problem in templates in the context of the template
declaration instead of in the context of all of the (possibly very many)
template instantiations.

Diff Detail

Event Timeline

steveire created this revision.Feb 7 2021, 11:20 AM
steveire requested review of this revision.Feb 7 2021, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2021, 11:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

What happens when TBase is an explicit specialization. In that situation a warning for the explicit specialization may be useful.

template <> struct TBase<float> {
  virtual void tfunt(T t);
};
clang-tools-extra/test/clang-tidy/checkers/bugprone-virtual-near-miss.cpp
49

Can this be left in to show no fix was applied.

What happens when TBase is an explicit specialization.

It's not changed before or after this patch. This check is documented as changing code based on the derived classes based on typos of methods overridden from the base.

Your suggestion makes sense to me, but it is orthogonal to this patch.

clang-tools-extra/test/clang-tidy/checkers/bugprone-virtual-near-miss.cpp
49

After this patch it actually does apply the fixit. I can see why the fixit would have been skipped when it was based on the template instantiation (because a specialization could do unexpected things), but I don't see why it should be excluded when matching based on the template declaration.

njames93 accepted this revision.Feb 20 2021, 1:07 PM

Fair enough, LGTM.

This revision is now accepted and ready to land.Feb 20 2021, 1:07 PM
This revision was automatically updated to reflect the committed changes.

Can you take a look at https://llvm.org/PR49330, I'm guessing this patch introduced this regression.

Can you take a look at https://llvm.org/PR49330, I'm guessing this patch introduced this regression.

+1 We've had to disable the check in the meantime.

Thanks for the heads-up. I've reverted it now.