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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
| 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? | |
Comment Actions
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. | |
Comment Actions
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'. | |
| 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. | |
| 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. | |
Comment Actions
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.
| clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst | ||
|---|---|---|
| 31 | Change this to a list? | |
Comment Actions
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. | |
| 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:
| |
| 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? | |
Comment Actions
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 | |
Comment Actions
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. | |
Comment Actions
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.
Pretty sure this comment can be re-flowed to 80 columns. Also needs trailing punctuation.