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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
