[PATCH] D111041: [clang-tidy] Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

Carlos Galvez via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 4 04:55:04 PDT 2021


carlosgalvezp added a comment.

Thanks for the input!

> release note mentions

Sure thing!

> documentation changes

As we discussed in the separate mail thread, there is no documentation at all as to what default values an alias/main check has. Where should I put that? Or do you mean that now that all 3 checks (primary + 2 aliases) have identical defaults, I should document that in the main check documentation?

> test coverage

I was surprised to not find any unit test for this check, probably because it's already done by the unit test of the "main check"? Should add a new unit test, pretty much copy-pasting the test for the main check?

> evidence that this option is not being used by users

Not sure how to go about this one :) I guess we can't have a 100% guarantee that this won't break things for users, there will always be an edge case. Perhaps we should document it very clearly in the Release Notes as well as explaining how to revert to the old behavior if needed?

Since we are talking about a "guideline check" (cppcoreguidelines-explicit-virtual-functions) and a "good practice check" (modernize-use-override), I don't think it's unreasonable that users have enabled both checks. If so, the "modernize" check should already have this behavior as default and prompted users to make changes - the cppcoreguidelines was less strict, and now it would be as strict as the "modernize-use-override" check. In that scenario, this patch shouldn't break things for users. The scenario that would break things for users is when they only enable the cppcoreguidelines check alone.

The guidelines changed in 2019, I think after 2 years users wouldn't mind the tooling catching up :)

Since I'm new here and missed quite a few things, do you know if there's a checklist somewhere where I can read about what else I should include in the patch?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111041



More information about the cfe-commits mailing list