This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] aliases for safety module from section 8 to 12 done
ClosedPublic

Authored by JonasToth on Feb 26 2017, 4:42 AM.

Details

Summary

This commit will add aliases into the safety module from other modules.
Since the original purpose of the effort was to implement the High Integrity Coding Standard,
Jonathan (jbcoe) and me had the idea to start out by figuring out what is already done
in clang-tidy.
Splitting the whole standard into parts, this work checked the section 8 to 12.

I made a todolist for me, which can be seen here: https://github.com/JonasToth/HighIntegrityTooling

When we are done aliasing, we create real todos and would like to migrate the list to issues
in the llvm issue tracker.

Please give your opinion on that approach.

Diff Detail

Repository
rL LLVM

Event Timeline

JonasToth created this revision.Feb 26 2017, 4:42 AM
JonasToth updated this revision to Diff 89813.Feb 26 2017, 4:50 AM

[Fix] typos in the module file - actually builds now :D

alexfh added inline comments.Feb 28 2017, 3:21 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

Should we have a separate module for the High Integrity C++ Coding Standard as we have for CERT? It seems natural to be able to check the code for HIC++ conformance instead of checking it for conformance with some "safety" checks. Isn't it?

docs/clang-tidy/checks/safety-explicit-conversions.rst
5 ↗(On Diff #89813)

How about adding links to the specific HIC++ rule this check enforces? Same for other checks.

alexfh requested changes to this revision.Feb 28 2017, 3:22 AM
This revision now requires changes to proceed.Feb 28 2017, 3:22 AM
JonasToth added inline comments.Feb 28 2017, 8:57 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

Yes, AFAIK was the safety module the starting point of that standard. But the naming is not explicit about it. Maybe we could discuss about a rebranding of the safety module. @jbcoe what do you think?

Furthermore, is it possible to configure aliased checks specially? It makes sense to set the default values/configuration so that it truly confirms.
Most aliases are just "we can do it with this one". Overview pages for different standards would be interesting as well, but out of scope for now, i would say.

docs/clang-tidy/checks/safety-explicit-conversions.rst
5 ↗(On Diff #89813)

Yup. I will update when the naming issue from above is clearer.

aaron.ballman added inline comments.Feb 28 2017, 12:41 PM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I think it's a good idea -- but, as with CERT, we should make sure we have the permission to use that specific name in our documentation and naming of the module. See LICENSE.TXT and clang-tidy/cert/LICENSE.TXT.

jbcoe added inline comments.Feb 28 2017, 2:17 PM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I contacted Programming Research, the authors of HIC++ and have not received a reply.

I would like to name this clang-tidy module after HIC++ but am not happy to do so without permission as it suggests endorsement.

As Aaron suggests, without permission we are probably better off rescinding references to HIC++ entirely and leaving a generic (and still very useful) safety module in place.

aaron.ballman added inline comments.Feb 28 2017, 3:30 PM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

That wasn't what I was suggesting -- I think having the module would be natural and consistent. The way I read the Conditions of Use is that it cannot imply *PRQA* endorses this use, which I don't think we're in danger of here.

I think we're fine so long as we properly attribute in the license (because we may be using names from the coding standard). Of course, IANAL, etc. ;-)

JonasToth added inline comments.Feb 28 2017, 11:30 PM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

Programming Research LTD business is to make static code analysis tools.
so i think, we cut their business interest implementing their HIC++ -standard.

probably better to be on the safe site and say the safety module is inspired by HIC++ and others. CERT and MISRA have stuff to say about c++ as well right?
i think we should be sure that we are allowed to use their name and implement their standard, because if we are not, it makes sense for them to sue. at least my thoughts.

maybe we can implement in the old categories + safety and provide example configuration files that would enforce one standard.

@jbcoe did you ask a clang/llvm or urself? Maybe their could be an official request to the company, they probably react to that one.

aaron.ballman added inline comments.Mar 1 2017, 10:09 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

Programming Research LTD business is to make static code analysis tools. so i think, we cut their business interest implementing their HIC++ -standard.
probably better to be on the safe site and say the safety module is inspired by HIC++ and others. CERT and MISRA have stuff to say about c++ as well right?

Correct, hence why we've updated the license for CERT. As for HIC++ , their Conditions of Use are pretty easy to read and interpret. Personally, I don't think we have much to worry about in this regard, so long as we perform the proper attribution they ask for.

i think we should be sure that we are allowed to use their name and implement their standard, because if we are not, it makes sense for them to sue. at least my thoughts.

We have that permission from CERT. We have no such permission from MISRA.

JonasToth added inline comments.Mar 1 2017, 10:10 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

i dont mind these issues tbh :D
just dont want to be the root of later problems :)

iam fine with it, if you are as well.

jbcoe added inline comments.Mar 1 2017, 10:46 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I asked as myself.

Maybe a senior clang-community member with some CERT background (Aaron?) could ask on behalf of LLVM/Clang.

aaron.ballman added inline comments.Mar 1 2017, 11:48 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

It just so happens there is a PRQA person here at the WG21 meetings. I spoke with him and he said that he will poke the appropriate people to ensure you get a response in a timely manner.

jbcoe added inline comments.Mar 3 2017, 3:30 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

Can we merge this subject to probable revision when PRQA get back to us?
I'm happy to make the code changes needed to rename things when required.

alexfh added inline comments.Mar 3 2017, 5:24 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

If it doesn't take more than a couple of weeks, I'd better wait until we have an answer from PRQA or otherwise reach clarity on how we can refer to the HIC++ standard.

This patch is not going to bitrot soon, and I'd like to avoid unnecessary churn, if possible.

aaron.ballman added inline comments.Mar 3 2017, 3:53 PM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I agree. PRQA will hopefully respond soon, or @dberlin will weigh in. Either way, I think the module can wait for a short while to avoid churn.

jbcoe edited edge metadata.EditedMar 14 2017, 2:35 AM

@aaron.ballman any news for PRQA on naming this module? Seems a shame to block good work.

aaron.ballman edited edge metadata.Mar 14 2017, 5:27 AM

@aaron.ballman any news for PRQA on naming this module? Seems a shame to block good work.

I was hoping you had heard from them. Have you received any communications from PRQA (and if so, from who)? I'll see if the person I know from PRQA can get the ball rolling again.

In the meantime, @dberlin, can you comment on whether you think we should be fine to use the name "High Integrity C++ Coding Standard" in our documentation and as a clang-tidy module name (provided we update our license similar to how we did for the CERT module)?

aaron.ballman added inline comments.Mar 19 2017, 6:43 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I've heard back from PRQA, and they are happy for us to proceed.

"We are perfectly happy for Clang community or any individuals to add in-built or plug-in support for parts of HICPP. We do want an attribution alongside any rule enforcement or rule reference."

I've clarified the attribution with them, and we should proceed in the same way we did with the CERT rules. Basically, we add a LICENSE.TXT to include what's in the Conditions of Use, link to it from the clang-tools-extra LICENSE.TXT, and the documentation for the HIC++ checks should link back to the HIC++ coding standard directly.

I recommend we put this in a module named "hicpp" or "hic++" rather than "safety" to make it clear that the modules relate to the coding standard directly.

JonasToth added inline comments.Mar 19 2017, 8:44 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

That sounds really nice :)
I will proceed with adding the reference to the checks.
I don't know about the License Docs, could someone else do this, or at least point me in the right direction?

Renaming the whole module is ok with me.

aaron.ballman added inline comments.Mar 19 2017, 8:58 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I think this should be done in two steps (in separate patches). I'll go ahead and take care of the initial patch, and once that's in, you can rebase this patch on top of it. The new patch will:

  • Rename the safety directory to hicpp
  • Rename the SafetyTidyModule.cpp file to HICPPTidyModule.cpp
  • Rename the safety checks to hicpp checks
  • Update extra/LICENSE.TXT
  • Add extra/clang-tidy/hicpp/LICENSE.TXT

Hopefully this doesn't cause merge issues for you.

aaron.ballman added inline comments.Mar 19 2017, 10:36 AM
docs/ReleaseNotes.rst
70–74 ↗(On Diff #89813)

I've commit this change in r298229, so you should be safe to rebase now.

JonasToth updated this revision to Diff 92290.Mar 19 2017, 12:07 PM
JonasToth edited edge metadata.

Update the aliases to reflect the new name and link to the related rules in High Integrity C++

  • [Merge] Transition to hicpp wording
  • [Rename] rename safety files
  • Substitute safety with hicpp
  • [Doc] references to the section in hicpp added
JonasToth updated this revision to Diff 92291.Mar 19 2017, 12:14 PM
  • [Doc] add rule to every mention of the rule number
JonasToth marked 17 inline comments as done.Mar 19 2017, 12:14 PM

I wonder if it's worth adding a module-level test so that people could easily see that various violations of HICPP are diagnosed?

clang-tools-extra/tests/clang-tidy/test-hicpp.cpp

I'm a bit nervous about code going in without tests and more nervous about the aliases being ok now but wrong later if one of the aliased checks changes its meaning.

JonasToth added a comment.EditedMar 19 2017, 12:19 PM

@jbcoe Yeah that is actually a good point. Can the aliases be configured independent with default arguments? That would be a good improvement as well. For example function-size could take different default parameters.

But i think not every check needs to be tested. Lets say hicpp-use-equals-delete is really simplistic? and the test on the check itself should be sufficient.
Maybe an example configuration file would be nice as well.

jbcoe added inline comments.Mar 19 2017, 12:26 PM
docs/clang-tidy/checks/hicpp-identifier-naming.rst
7 ↗(On Diff #92291)

I'm not sure this is the right check.

I think you want readability-named-parameter.
https://clang.llvm.org/extra/clang-tidy/checks/readability-named-parameter.html

readability-identifier-naming takes masses of config and uses it to enforce naming conventions like snake case and trailing _ for member variables.
https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html

docs/clang-tidy/checks/hicpp-undelegated-constructor.rst
8 ↗(On Diff #92291)

typo codeduplication

JonasToth updated this revision to Diff 92292.Mar 19 2017, 12:47 PM
  • [Fix] wrong alias mentioned by jonathan
JonasToth marked an inline comment as done.Mar 19 2017, 12:47 PM
JonasToth added inline comments.
docs/clang-tidy/checks/hicpp-identifier-naming.rst
7 ↗(On Diff #92291)

jup. adjusted.

docs/clang-tidy/checks/hicpp-undelegated-constructor.rst
8 ↗(On Diff #92291)

what would be the correct spelling? xD
code duplication?

JonasToth updated this revision to Diff 92294.Mar 19 2017, 12:48 PM
JonasToth marked an inline comment as done.
  • [Fix] typo for code duplication
JonasToth marked 2 inline comments as done.Mar 19 2017, 12:48 PM
aaron.ballman added inline comments.Mar 20 2017, 2:30 PM
clang-tidy/hicpp/HICPPTidyModule.cpp
35–36 ↗(On Diff #92294)

This should be removed -- it's handled below.

docs/ReleaseNotes.rst
81–82 ↗(On Diff #92294)

This should link to the HIC++ standard online.

84 ↗(On Diff #92294)

I think we can go ahead and remove this, because *everything* in the hicpp module will be new for this release.

docs/clang-tidy/checks/hicpp-explicit-conversions.rst
7 ↗(On Diff #92294)

If it doesn't enforce some part of a rule, I think we should call that out explicitly and probably provide wording and an example of what does and does not comply with the rule. Users are going to want to know what level of enforcement there is for a safety-related standard.

Same comments apply to all the docs that claim partial enforcement.

docs/clang-tidy/checks/hicpp-function-size.rst
7 ↗(On Diff #92294)

Typo: useful

docs/clang-tidy/checks/hicpp-named-parameter.rst
8 ↗(On Diff #92294)

Drop "the"

docs/clang-tidy/checks/hicpp-new-delete-operators.rst
8 ↗(On Diff #92294)

Implements <blah> to ensure the new and delete operators have the correct signature.

docs/clang-tidy/checks/hicpp-special-member-functions.rst
7 ↗(On Diff #92294)

function -> functions

docs/clang-tidy/checks/hicpp-undelegated-constructor.rst
8 ↗(On Diff #92294)

This last sentence is ungrammatical, but will likely be improved when you add the extra information about what the check does and does not get right versus the rule.

Also, we probably shouldn't say the check "tries" to implement something. It implements partial support. ;-)

docs/clang-tidy/checks/hicpp-use-equals-default.rst
7 ↗(On Diff #92294)

Implement -> Implements
to default -> to explicitly default

docs/clang-tidy/checks/hicpp-use-equals-delete.rst
8 ↗(On Diff #92294)

to explicitly default or delete.

docs/clang-tidy/checks/hicpp-use-override.rst
8 ↗(On Diff #92294)

to declare a virtual function

JonasToth updated this revision to Diff 93036.Mar 25 2017, 3:48 AM
JonasToth marked 10 inline comments as done.
  • [Doc] fix the bad grammar and typos
JonasToth planned changes to this revision.Mar 25 2017, 3:48 AM

fixed the easier issues.

JonasToth updated this revision to Diff 93037.Mar 25 2017, 3:59 AM
JonasToth marked 2 inline comments as done.
  • [Doc] grammar and content fixed

nontypo fixes.

JonasToth updated this revision to Diff 93038.Mar 25 2017, 4:01 AM
  • minor fix, commas not needed in enumeration
aaron.ballman added inline comments.Mar 26 2017, 7:18 AM
docs/clang-tidy/checks/hicpp-explicit-conversions.rst
9 ↗(On Diff #93038)

castingchecks -> casting checks

docs/clang-tidy/checks/hicpp-named-parameter.rst
4 ↗(On Diff #93038)

This underline is too long.

docs/clang-tidy/checks/hicpp-undelegated-constructor.rst
10 ↗(On Diff #93038)

Rather than saying it's not easily done automatically, it would be more useful to the user to provide examples of what does and does not match the specific coding rule.

JonasToth marked 3 inline comments as done.Mar 28 2017, 1:20 AM
JonasToth added inline comments.
docs/clang-tidy/checks/hicpp-undelegated-constructor.rst
10 ↗(On Diff #93038)

I used the examples from the testcase, i hope its ok this way.

JonasToth updated this revision to Diff 93220.Mar 28 2017, 1:20 AM
JonasToth marked an inline comment as done.
  • [Docs] Typos fixed
  • [Doc] improve documentation on what is done in the delegation constructor check

@JonasToth If you don't have commit access let me know and I can merge this patch for you.

@jbcoe I dont have them, Thank you for commiting :)

This revision was automatically updated to reflect the committed changes.

The docs for all these aliases are missing http redirects e.g.

.. meta::
   :http-equiv=refresh: 5;URL=modernize-use-equals-delete.html

Without these redirects, the automatically generated list of checks (list.rst) does not say that the checks redirect.