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

[carlosgalvezp]

Summary

This requirement was introduced in the C++ Core guidelines in 2016:

https://github.com/isocpp/CppCoreGuidelines/commit/1894380d0abf4d7d49a983005647e0d41ecbf214

Then clang-tidy got updated to comply with the rule.

However in 2019 this decision was reverted:

https://github.com/isocpp/CppCoreGuidelines/commit/5fdfb20b760c5307bf86873798a146fcd7e912e6

Therefore we need to apply the correct configuration to
clang-tidy again.

This also makes this cppcoreguidelines check consistent
with the other 2 alias checks: hicpp-use-override and
modernize-use-override.

Additionally, add another RUN line to the unit test,
to make sure cppcoreguidelines-explicit-virtual-functions
is tested.

[carlosgalvezp]

I wonder why I'm not getting automated pre-merge builds?

[aaron.ballman]

In D111041#3039590, @carlosgalvezp wrote:

I wonder why I'm not getting automated pre-merge builds?

Huh, that is strange. Usually you get them automatically when you set the Repository properly (which it looks like you have).

This change is missing documentation changes, release note mentions, test coverage, etc, so it needs a bit of work in those areas. However, more importantly, do you have evidence that this option is not being used by users? I understand that the core guidelines changed their position on this feature and so we may want to change the default value, but removing the option entirely feels user-hostile without some evidence that this option is unused in practice.

[carlosgalvezp]

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!

[aaron.ballman]

In D111041#3039637, @carlosgalvezp wrote:

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?

Ah, I see now -- we failed to mention in modernize-use-override.rst that the default value for this is true in cppcoreguidelines-explicit-virtual-functions. That's where I'd expect to see that documentation. I suppose now that documentation would no longer be necessary, so we're good here.

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?

Erf, the lack of initial test coverage also makes me sad -- I would have expected modernize-use-override-no-destructor.cpp to have an additional RUN line to test the behavior of this check as well (which would have then failed with the changes). I'd say let's go ahead and add that as a test case; add another RUN line to run cppcoreguidelines-explicit-virtual-functions and give it a specific FileCheck prefix so that we can test we don't get the warning from modernize but we do get the warning from cppcoreguidelines. For an example of the FileCheck prefix I'm talking about, see the RUN lines in cert-static-object-exceptions.cpp.

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?

I think that'll be the best we can do.

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.

Agreed; I'm worried about the people who enabled only the core guideline check getting new diagnostics they weren't expecting. However, that's what the core guidelines have decided they want, so I think it's reasonable to make the change.

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?

I don't know if we have a checklist like that anywhere, but what reviewers are generally going to look for is: functionality of the patch itself, test coverage for the changes (unless the change is marked NFC, aka No Functional Change), documentation for the changes (if needed).

but removing the option entirely feels user-hostile without some evidence that this option is unused in practice.

Quoting myself here, but derp, this isn't removing the option entirely, just removing the fact that the default value is different, which is fine to do here. Sorry for the confusion on that!

[carlosgalvezp]

Added documentation

[carlosgalvezp]

Regarding test coverage, I'm unsure if what you propose is intended. I have looked at similar aliases and I can't find any test at all for them either. For example modernize-avoid-c-arrays is aliased by "cppcoreguidelines" and "hicpp" and I cannot find tests for them. So perhaps there's a reason for this?

I had a look at your example with check_prefix but I'm not sure it fits well here. In the case you mention, it's about running *the same check* with different flags. Here we are talking about running *different checks* (though aliased) in the same .cpp test file. If we want to treat aliases as "first class checks", I think it only makes sense to have a "first class test" also for the aliases. But that leads to code duplication, of course. Feels like this should be discussed in cfe-dev, but perhaps it's outside the scope of this patch? Right now there's really nothing new to test - the check is now a perfect alias, and the test strategy is consistent with other aliases.

[carlosgalvezp]

Oh, I found misc-non-private-member-in-classes that does what you proposed. I'll add another RUN line to run the cppcoreguidelines check.

[carlosgalvezp]

Added an extra RUN line to run the checks also for the alias, cppcoreguidelines-explicit-virtual-functions.

[carlosgalvezp]

@aaron.ballman Let me know if this addresses your comments!

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/modernize-use-override.cpp

56 ↗

(On Diff #378514)

This isn't quite what I was after; now it looks like we expect to always get the diagnostic (in fact, I'm a bit worried that this test is passing). I'd rather see:

// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
// CHECK-CPPCOREGUIDELINES-NOT: :[[@LINE-2]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'

So that we check explicitly we see the diagnostic for modernize and explicitly that we don't see the diagnostic for C++ Core Guidelines.

You'll need to change the second RUN line to not use check_clang_tidy but instead execute clang-tidy manually so you can pass -check-prefix=CHECK-CPPCOREGUIDELINES to it (as done in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp#L4).

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/modernize-use-override.cpp

56 ↗

(On Diff #378514)

Not sure I follow. What you propose is something that would make sense _before_ this patch, when the cppcoreguidelines check was _different_ than modernize. But now they are not, they are identical, so we expect to get identical diagnostics from them. Or am I missing something obvious?

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/checkers/modernize-use-override.cpp

56 ↗

(On Diff #378514)

Or am I missing something obvious?

No, you just caught me having a major think-o. Again. Because this is the second time I've gotten the changes exactly backwards in my mind. :-D Sorry for the confusion!

[carlosgalvezp]

I realized I can speed up the test by removing the extra RUN line and simply add the new check to the list of checks, let me fix that before merge :)

clang-tools-extra/test/clang-tidy/checkers/modernize-use-override.cpp

56 ↗

(On Diff #378514)

No worries! :)

[carlosgalvezp]

Removed RUN line and put the check in the existing one, to speed up the test.

[aaron.ballman]

Thank you for the fix! I've commit on your behalf in 40546cb38189e438a81faa6400c103d159600f9e.

[carlosgalvezp]

Thanks a lot for the review and the help!

By the way, should we keep the [clang-tidy] header for the next commits? Glancing at the commit history, I think it's much easier to see what the commit is updating, and probably can setup email filters to only receive notifications e.g. when [clang-tidy] is in the commit message. It's also consistent with the rest of commits. For example this one:

https://github.com/llvm/llvm-project/commit/a76cfc2e840ff373b80e3a5f84fc48c5f1f90d8a

[aaron.ballman]

In D111041#3059054, @carlosgalvezp wrote:

Thanks a lot for the review and the help!

By the way, should we keep the [clang-tidy] header for the next commits? Glancing at the commit history, I think it's much easier to see what the commit is updating, and probably can setup email filters to only receive notifications e.g. when [clang-tidy] is in the commit message. It's also consistent with the rest of commits. For example this one:

https://github.com/llvm/llvm-project/commit/a76cfc2e840ff373b80e3a5f84fc48c5f1f90d8a

Eh, I personally don't care all that much one way or the other (so sure, I can do that!), but I recall there being a push to keep the "title" line under something very small (50 chars?) because of the way tools sometimes display this information to users, so I've always stripped the bit within [] to ensure we keep a sufficiently short title.

That said, I'm wondering if you're planning to stick around in the clang-tidy community? If so, given that you've got a few good patches accepted already, it might be time to consider getting you commit privileges of your own. https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access has more details on what that entails.

[carlosgalvezp]

Eh, I personally don't care all that much one way or the other (so sure, I can do that!), but I recall there being a push to keep the "title" line under something very small (50 chars?) because of the way tools sometimes display this information to users, so I've always stripped the bit within [] to ensure we keep a sufficiently short title.

Yeah I understand, it makes sense. I just find that in general that's not respected so I wonder if people choose consistency in using [] over short commit messages. I can also find this in the Docs:

When the changes are restricted to a specific part of the code (e.g. a back-end or optimization pass), it is customary to add a tag to the beginning of the line in square brackets. For example, “[SCEV] …” or “[OpenMP] …”. This helps email filters and searches for post-commit reviews.

Anyhow, minor detail :)

That said, I'm wondering if you're planning to stick around in the clang-tidy community? If so, given that you've got a few good patches accepted already, it might be time to consider getting you commit privileges of your own. https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access has more details on what that entails.

That'd be great, thanks for the support! Since I'm mostly working on this on my spare time I can't promise much involvement, but rather occasional bug fixes and minor improvements. Still would be good to lift the commit burden from you haha. I really enjoy the repro structure, build system and processes, it's a very nice codebase to work with. If time allows I could potentially look into larger pieces of work, like adding new clang-tidy modules (e.g. Misra/Autosar checks). I find some local forks here and there that have done it but never pushed upstream, which I find a bit sad.

[aaron.ballman]

In D111041#3059245, @carlosgalvezp wrote:

Eh, I personally don't care all that much one way or the other (so sure, I can do that!), but I recall there being a push to keep the "title" line under something very small (50 chars?) because of the way tools sometimes display this information to users, so I've always stripped the bit within [] to ensure we keep a sufficiently short title.

Yeah I understand, it makes sense. I just find that in general that's not respected so I wonder if people choose consistency in using [] over short commit messages. I can also find this in the Docs:

When the changes are restricted to a specific part of the code (e.g. a back-end or optimization pass), it is customary to add a tag to the beginning of the line in square brackets. For example, “[SCEV] …” or “[OpenMP] …”. This helps email filters and searches for post-commit reviews.

Anyhow, minor detail :)

Yeah, we're pretty inconsistent about this because it depends on the committer and what process they use to land commits. I commit manually (I don't use arc), so I can leave the tags in easily enough.

That said, I'm wondering if you're planning to stick around in the clang-tidy community? If so, given that you've got a few good patches accepted already, it might be time to consider getting you commit privileges of your own. https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access has more details on what that entails.

That'd be great, thanks for the support! Since I'm mostly working on this on my spare time I can't promise much involvement, but rather occasional bug fixes and minor improvements. Still would be good to lift the commit burden from you haha. I really enjoy the repro structure, build system and processes, it's a very nice codebase to work with. If time allows I could potentially look into larger pieces of work, like adding new clang-tidy modules (e.g. Misra/Autosar checks). I find some local forks here and there that have done it but never pushed upstream, which I find a bit sad.

It's totally up to you if you'd like to request commit access or not (I'll happily support your request), but it's not a burden for me to commit things. I just have to be around to babysit the bots in case a fix or revert is needed, basically. My recommendation is: if you think you'll do patches here and there as time allows, you might as well request commit access; if you think your contributions are likely to not continue in the future, then no reason to do that dance.

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

public:
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
 
     Opts["cppcoreguidelines-non-private-member-variables-in-classes."
          "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "true";
 
-    Opts["cppcoreguidelines-explicit-virtual-functions."
-         "IgnoreDestructors"] = "true";
-
     return Options;
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
 
 } // namespace cppcoreguidelines
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CppCoreGuidelinesModule.
 volatile int CppCoreGuidelinesModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-flp37-c
   <clang-tidy/checks/cert-flp37-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Removed default setting `cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"`,
+  to match the current state of the C++ Core Guidelines.
+
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...