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

Tue Oct 26 08:44:06 PDT 2021

aaron.ballman added a reviewer: alexfh.
aaron.ballman added a comment.

In D112409#3087517 <https://reviews.llvm.org/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 situation is a bit of a mess. I'll leave it to people with stronger opinion/experience.

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).

================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:52
+// FIXME: The check can be improved to handle such cases.
+const llvm::StringRef CertErr33CCheckedFunctions = "::aligned_alloc;"
+                                                   "::asctime_s;"
----------------
balazske wrote:
> aaron.ballman wrote:
> > 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.)
> 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).
Thanks! I did more spot checking, didn't see anything wrong.

================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-return-value.rst:31
    semget, setjmp, shm_open, shmget, sigismember, strcasecmp, strsignal,
    ttyname``

----------------
balazske wrote:
> Change this to a list?
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112409