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

[Balázs Benics via Phabricator via cfe-commits]

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 excluded because they can
+// be called with NULL argument and in
+// this case the check is not applicable:
+// mblen, mbrlen, mbrtowc, mbtowc, wctomb,
+// wctomb_s
----------------
whisperity wrote:
> aaron.ballman wrote:
> > Pretty sure this comment can be re-flowed to 80 columns. Also needs trailing punctuation.
> 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 `StringRef`s or even `llvm::StringLiteral`s 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`.
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.


================
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;
----------------
carlosgalvezp wrote:
> balazske wrote:
> > carlosgalvezp wrote:
> > > Same here
> > This is correct: 'str34' before 'err33'.
> No, `e` goes before `s`.  The existing checks are ordered: `dcl`, `oop`, `str`. So this check should go after `dcl16`.
+1 for fixing the order


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112409/new/

https://reviews.llvm.org/D112409