aaron.ballman added a comment. In D111041#3039637 <https://reviews.llvm.org/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! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111041/new/ https://reviews.llvm.org/D111041 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
