Page MenuHomePhabricator

[clang-tidy] Add check 'cert-err33-c'.
ClosedPublic

Authored by balazske on Oct 25 2021, 12:14 AM.

Details

Summary

The CERT rule ERR33-C can be modeled partially by the existing check
'bugprone-unused-return-value'. The existing check is reused with
a fixed set of checked functions.

Diff Detail

Event Timeline

balazske created this revision.Oct 25 2021, 12:14 AM
balazske requested review of this revision.Oct 25 2021, 12:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 12:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks, this looks good to me! However, please wait for clang-tidy owners' approval.

This check is likely to generate many warnings on a "random" open-source project because many of these functions are usually not checked. But the rule contains this list and if a project wants to conform this check can be useful.

Eugene.Zelenko added inline comments.Oct 25 2021, 9:26 AM
clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst
8

Usually such statement is located at the end.

14

Please enclose NULL into double back-ticks.

aaron.ballman added inline comments.Oct 25 2021, 9:35 AM
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
45–50

Pretty sure this comment can be re-flowed to 80 columns. Also needs trailing punctuation.

52

Was this list generated automatically or manually? (Just wondering how hard to match it to the CERT rule -- spot checking hasn't found issues so far.)

clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst
51

Can you add a newline back?

carlosgalvezp added a comment.EditedOct 25 2021, 10:30 AM

Also:

  • Please add a unit test. You can probably re-use the corresponding bugprone test and tell it to add the cert-err33-c check as well. If they are too different I suppose it's fine to create it's own standalone test?
  • Mention this new check in the clang-tidy release notes.
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
303–304

Keep alphabetical order (err33 before err34)

327

Same here

clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst
49

an alias

50

Add link?

clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst
2

I believe we usually mention that this is an alias to another check, and then redirect the user to that original check.

clang-tools-extra/docs/clang-tidy/checks/list.rst
117

Missing referring to the alias check.

balazske updated this revision to Diff 382312.Oct 26 2021, 7:37 AM

Improved the documentation.
Small code layout changes.

balazske marked 8 inline comments as done.Oct 26 2021, 7:44 AM

I was not sure how the aliasing is to be handled if the check is not exactly the same as the original. (This was a topic on the mailing list.) I like it more if only those checks are aliases that are exactly the same. But for now it will remain as it was before, otherwise this has to be changed for all aliases.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
52

I am not sure, I have the list for longer time, but it is almost sure that it was generated by using text editor macros. The number of functions is correct (with the excluded ones).

327

This is correct: 'str34' before 'err33'.

balazske added inline comments.Oct 26 2021, 7:46 AM
clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst
2

Automatic redirect does not work here because there is something to read in the page. This check has its own description.

balazske updated this revision to Diff 382323.Oct 26 2021, 7:59 AM

Added to release notes, added new links to documentation.

carlosgalvezp added inline comments.Oct 26 2021, 8:00 AM
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
327

No, e goes before s. The existing checks are ordered: dcl, oop, str. So this check should go after dcl16.

I was not sure how the aliasing is to be handled if the check is not exactly the same as the original.

I agree that the alias situation is a bit of a mess. I'll leave it to people with stronger opinion/experience.

balazske added inline comments.Oct 26 2021, 8:13 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst
31

Change this to a list?

I was not sure how the aliasing is to be handled if the check is not exactly the same as the original.

I agree that the alias situation is a bit of a mess. I'll leave it to people with stronger opinion/experience.

Aliases are not expected to be exact copies; more often they differ from the base check via config options. Typically, we let the alias redirect to the original and we mention those differences in the documentation for the original. However, in this case, the documentation difference is a huge list of function names. I don't think it's a good idea to duplicate these two lists in one page because that's a huge wall of text that won't be easy to spot the differences in, and I don't think users are going to read those lists in the first place (more often, I think they'll search the list rather than read it).

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
52

Thanks! I did more spot checking, didn't see anything wrong.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst
31

I'm on the fence. I think a list is easier to read, but I don't think anyone reads this amount of text to begin with and making it a list then pushes information the user may want to read to much later in the page.

whisperity added inline comments.Oct 27 2021, 2:53 AM
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
45–50

Shouldn't we reuse utils::options::serializeStringList here instead of hardcoding the separator character into a giant literal? I know executing that operation has an associated run-time cost, and as it is not constexpr it needs to be done somewhere else, and std::string could throw so we can't do it at "static initialisation" time... But having those extra chars there just seems way too fragile at a later modification. We've had cases where people missed separating ,s even -- and those are syntactically highlighted differently due to being outside the string literal.

Suggestion:

  • An array of StringRefs or even llvm::StringLiterals containing the individual function names. Array separated with ,, the separator outside the literal. Aligned to column and probably one per line.
  • When using this variable later (🌟), instead of passing the stringref, pass the result of serializeStringList.
327

(🌟)

clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst
12

mblen, mbrlen, etc. are with backticks later. But this list isn't. Was this intended?

It looks good to me. But I'm leaving the approval up to the tidy folks.
BTW shouldn't we use backticks in the giant list?

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
45–50

I think it looks good enough. We will certainly recognize if we change this behavior and it would be easy to port these anyway. So I'm not too concerned about this.

327

+1 for fixing the order

balazske updated this revision to Diff 383047.Oct 28 2021, 8:40 AM

Updated order of code lines, added a test file.

balazske marked 2 inline comments as done.Oct 28 2021, 8:47 AM

Not sure if it is good to have such a test, the first and last function is enough?

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
45–50

It is not simple to change to array, the serializeStringList must be changed too because it accepts only array of std::string. Other small things must be added too to make this change work.

327

Should be correct now, and another ordering problem was fixed.

clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst
12

This is a "other" type of list, the function names are not inside other text.

balazske updated this revision to Diff 383050.Oct 28 2021, 8:49 AM
balazske marked an inline comment as done.

Removed remaining non-alias check line from list.rst.

Not sure if it is good to have such a test, the first and last function is enough?

Yeah, that's testing an awful lot. We don't usually aim for exhaustive tests with these configurable lists, so long as one or two entries are covered, we usually assume the rest will also be fine. Might be worth cutting the test down a bit.

balazske updated this revision to Diff 383245.Oct 29 2021, 12:16 AM

Using a much smaller test file.

This revision is now accepted and ready to land.Oct 29 2021, 4:15 AM
This revision was automatically updated to reflect the committed changes.