Page MenuHomePhabricator

[clang-tidy] added cppcoreguidelines-explicit-virtual-functions
ClosedPublic

Authored by lewmpk on Feb 27 2019, 11:46 AM.

Event Timeline

lewmpk created this revision.Feb 27 2019, 11:46 AM

Welcome to the LLVM community and thank you for the patch lewmpk!

Please add unit tests for the changes you made to the check. They live in clang-tools-extra/test/clang-tidy/modernize-.....
It is common to add a additional test-file to ensure the configuration options do work as expected.
You can get inspiration from other checks that provide configuration capability (e.g. https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html#options)
for reference.

If you have any questions don't hesitate to ask!

P.S: Usually we upload the patches with full context (https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch) for the reviewer to see the change in context.

clang-tidy/modernize/UseOverrideCheck.cpp
23 ↗(On Diff #188642)

Please ellide the braces for the single-statements (this if and the else branch)

clang-tidy/modernize/UseOverrideCheck.h
23 ↗(On Diff #188642)

That requires additional methods for the function for the configuration to function properly. Please see other checks and implement that the same way.

27 ↗(On Diff #188642)

please use private instead

JonasToth edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 12:00 PM

I think this change is worth mentioning in the release notes as well (cte/docs/ReleaseNotes.rst)

Please resubmit this patch with full context

git diff --cached -U999999 > patch_to_submitt.diff

You need to add some documention describing the new option into the check in

docs/clang-tidy/checks/modernize.... .rst

lewmpk updated this revision to Diff 188622.Feb 27 2019, 2:17 PM
lewmpk marked an inline comment as done.
  • addressed comments
  • provided tests
  • updated documentation
  • updated release notes
lewmpk marked 2 inline comments as done and an inline comment as not done.Feb 27 2019, 2:18 PM
lewmpk added inline comments.
clang-tidy/modernize/UseOverrideCheck.h
23 ↗(On Diff #188642)

Could you elaborate? I looked at other checks and they seem to implement it the same way.

/llvm/tools/clang/tools/extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp:74

CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),

Thank you for the contribution! Please see the comments inline.

clang-tidy/modernize/UseOverrideCheck.cpp
34 ↗(On Diff #188642)

Please prefer early exit:

if (!getLangOpts().CPlusPlus11)
  return;
<everything else>
docs/ReleaseNotes.rst
71 ↗(On Diff #188642)
  1. Please mention that this is an alias for the modernize-use-override check.
  2. Please add a document for the new alias. See docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst for an example.
alexfh added inline comments.Feb 27 2019, 2:25 PM
clang-tidy/modernize/UseOverrideCheck.h
23 ↗(On Diff #188642)

You need to override the storeOptions() method.

alexfh added inline comments.Feb 27 2019, 2:27 PM
test/clang-tidy/modernize-use-override-no-destructors.cpp
7 ↗(On Diff #188642)

Remove the semicolons after methods. Clang-format the test (but ensure clang-format doesn't break comments at the 80 character bound).

lewmpk updated this revision to Diff 188628.Feb 27 2019, 2:48 PM

addressed comments

lewmpk marked 4 inline comments as done.Feb 27 2019, 2:50 PM

thanks for the feedback everyone (and the warm welcome)

alexfh accepted this revision.Feb 27 2019, 3:04 PM

Looks good! Do you need someone to commit the patch for you?

This revision is now accepted and ready to land.Feb 27 2019, 3:04 PM

I'm trying to find a way to run the test. If someone else has already tested it then yes please.

I'm trying to find a way to run the test. If someone else has already tested it then yes please.

If you've set up the build with ninja, ninja check-clang-tools should do the right thing. But I'm running the test anyway.

I'm trying to find a way to run the test. If someone else has already tested it then yes please.

If you've set up the build with ninja, ninja check-clang-tools should do the right thing. But I'm running the test anyway.

The new test fails even after I fixed the YAML config in the RUN lines. Please fix and re-upload the patch.

lewmpk updated this revision to Diff 188638.Feb 27 2019, 3:42 PM
  • fixed tests
  • fixed typo in documentation
Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/modernize-use-override.rst
5 ↗(On Diff #188642)

Will be good idea to remove duplicated empty line.

13 ↗(On Diff #188642)

Please use ` to highlight value.

lewmpk updated this revision to Diff 188642.Feb 27 2019, 3:57 PM

cleaned up documentation

lewmpk marked 2 inline comments as done.Feb 27 2019, 4:00 PM

cleaned up documentation

Are you planning on landing this anytime soon given that it was accepted? I would like to land D57087: [clang-tidy] add OverrideMacro to modernize-use-override check but I suspect either you are I will need to rebase, I'm just trying to be nice and ask if you'd like to land first?

I'm happy to land this ASAP but I don't have commit rights

MyDeveloperDay added a comment.EditedFeb 28 2019, 3:59 AM

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

MyDeveloperDay edited the summary of this revision. (Show Details)Feb 28 2019, 4:20 AM

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

@MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise I can land this patch now.

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

@MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise I can land this patch now.

I'll let you folks do this. But please note that the patch wasn't generated with arcanist, so it doesn't apply with arc patch --nobranch. Copy-pasting the raw diff to patch -p0 works though (as would downloading the diff and feeding it to patch as a file). Make sure to put "Differential Revision: https://reviews.llvm.org/D58731" at the end of the message for Phabricator to close this revision automatically.

I'm happy to land this ASAP but I don't have commit rights

So one of us could land it for you.. (I've not personally done that before as I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues I'd recommend it.. (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)

@MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise I can land this patch now.

I'll let you folks do this. But please note that the patch wasn't generated with arcanist, so it doesn't apply with arc patch --nobranch. Copy-pasting the raw diff to patch -p0 works though (as would downloading the diff and feeding it to patch as a file). Make sure to put "Differential Revision: https://reviews.llvm.org/D58731" at the end of the message for Phabricator to close this revision automatically.

@JonasToth you are more qualified than me please go ahead...

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2019, 6:55 AM

Thank you for the patch!

@JonasToth I left a comment in the commit needed to fix the index.rst, which I don't think your later review fixes, sphinx complained about the rst file being an unreferenced octtree

https://reviews.llvm.org/rG5fbeff797a9dba504f08f14c4fa59b6f1076fe72#inline-2691

@JonasToth I left a comment in the commit needed to fix the index.rst, which I don't think your later review fixes, sphinx complained about the rst file being an unreferenced octtree

https://reviews.llvm.org/rG5fbeff797a9dba504f08f14c4fa59b6f1076fe72#inline-2691

Yup. Thank you for the hint. I was just blind and couldn't see the solution :D