[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

This revision was automatically updated to reflect the committed changes. Closed by commit rG4bcbb3d4d7a8: [clang-tidy] Add check 'cert-err33-c'. (authored by balazske). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.org/D112409

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske updated this revision to Diff 383245. balazske added a comment. Using a much smaller test file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.org/D112409 Files: clang-tools-extra/clang-tidy/cert/CERTTidy

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

aaron.ballman added a comment. In D112409#3093815 , @balazske wrote: > 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 configur

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske updated this revision to Diff 383050. balazske marked an inline comment as done. balazske added a comment. Removed remaining non-alias check line from list.rst. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske marked 2 inline comments as done. balazske added a comment. Not sure if it is good to have such a test, the first and last function is enough? Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:45-50 +// The following functions are +// deliberately exclu

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske updated this revision to Diff 383047. balazske added a comment. Updated order of code lines, added a test file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.org/D112409 Files: clang-tools-extra/clang-ti

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

steakhal added a comment. 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? Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:45-50 +// The following functions are +// deliberately ex

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:45-50 +// The following functions are +// deliberately excluded because they can +// be called with NULL argument and in +// this case the check is not applicable: +// mblen, mbrlen

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

aaron.ballman added a reviewer: alexfh. aaron.ballman added a comment. In D112409#3087517 , @carlosgalvezp wrote: >> 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 sit

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst:31 semget, setjmp, shm_open, shmget, sigismember, strcasecmp, strsignal, ttyname`` Change this to a list? Repository: rG LLVM Gith

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

carlosgalvezp added a comment. > 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. Repository: rG LLVM Github Monorepo CHANG

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:328 Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "false"; +Opts["cert-err33-c.CheckedFunctions"] = CertErr33CCheckedFunctions; return Options; -

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske updated this revision to Diff 382323. balazske added a comment. Added to release notes, added new links to documentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.org/D112409 Files: clang-tools-extr

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst:1 +.. title:: clang-tidy - cert-err33-c + carlosgalvezp wrote: > I believe we usually mention that this is an alias to another check, and then > redirect the us

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske marked 8 inline comments as done. balazske added a comment. 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 n

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske updated this revision to Diff 382312. balazske added a comment. Improved the documentation. Small code layout changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.org/D112409 Files: clang-tools-extra/c

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

carlosgalvezp added a comment. 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-

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:45-50 +// The following functions are +// deliberately excluded because they can +// be called with NULL argument and in +// this case the check is not applicable: +// mblen, mbr

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst:7 +Warns on unused function return values. +This check corresponds to (a part of) CERT C Coding Standard rule `ERR33-C. +Detect and handle standard library errors -

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske added a comment. 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. Repository: rG LLVM Github Monorepo

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

martong added a comment. Thanks, this looks good to me! However, please wait for clang-tidy owners' approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112409/new/ https://reviews.llvm.org/D112409 _

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

balazske created this revision. Herald added subscribers: carlosgalvezp, steakhal, martong, gamesh411, Szelethus, dkrupp, xazax.hun, whisperity. balazske requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. The CERT rule ERR33-C can