Page MenuHomePhabricator

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

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

Diff Detail

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

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

clang-tidy/modernize/UseOverrideCheck.h
23

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

27

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

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

Please prefer early exit:

if (!getLangOpts().CPlusPlus11)
  return;
<everything else>
docs/ReleaseNotes.rst
71
  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

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

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

Will be good idea to remove duplicated empty line.

13

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