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

[Balázs Kéri via Phabricator via cfe-commits]

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 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
----------------
steakhal wrote:
> 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.
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.


================
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;
----------------
steakhal wrote:
> 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
Should be correct now, and another ordering problem was fixed.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst:11
+
+* aligned_alloc()
+* asctime_s()
----------------
whisperity wrote:
> `mblen`, `mbrlen`, etc. are with backticks later. But this list isn't. Was this intended?
This is a "other" type of list, the function names are not inside other text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112409