mkindahl wrote:
> First off, by usage of "—" and comments in code it seems to be partially/all
> written by AI. Without notice about AI, it's violation of
> https://llvm.org/docs/AIToolPolicy.html: "Contributors are expected to be
> transparent and label contributions that contain substantial amounts of
> tool-generated content"
Yes, I am using Claude Code as part of development. I wasn't aware that you
needed to add a trailer. Do you want it in the commit, the PR, or both?
> This check must have false positives when buffer is used safely (correct me
> if I'm wrong please)
>
> ```c++
> void good_setvbuf(void) {
> char buf[BUFSIZ];
> setvbuf(stdout, buf, _IOFBF, BUFSIZ);
>
> // ...
>
> fflush(stdout);
> setvbuf(stdout, nullptr, _IONBF, 0);
> }
> ```
>
> So we should check for "safe usages" as well before emitting diagnostic.
This is a very rare case and if the developer really needs this they can use
`NOLINT` to silence it. Most uses either heap-allocate a buffer or use a static
buffer. I think it is more important to capture the idiomatic faulty cases.
Also, if you require the capture of `fflush()`, what about other functions that
call `fflush()`? For example, shall we capture `_fcloseall()`?
Looked for issue regarding `setvbuf` that actually sets a buffer. Most use
NULL, but [this one](https://github.com/neovim/neovim/pull/26790/changes) uses
a `malloc()`-allocated buffer (strictly speaking, `xmalloc`).
I ran a quick test on a few repos I had locally, with the following result:
| Codebase | setvbuf/setbuf calls | Warnings |
|--------------|----------------------|----------|
| PostgreSQL | 14 | 0 |
| Apache httpd | 1 | 0 |
| CPython | 15 | 0 |
Incidentally, neither use buffers (they just use NULL). It seems that is not a
very common case.
> This check will currently also run for C++, when we can do:
>
> ```c++
> struct StdoutBuffer {
> char buf[BUFSIZ];
>
> StdoutBuffer() { setbuf(stdout, buf); }
> ~StdoutBuffer() { fflush(stdout); setbuf(stdout, nullptr); }
> };
> ```
It is an interesting case, but adding a `NOLINT` for these also makes sense
since it is a very rare case.
https://github.com/llvm/llvm-project/pull/187637
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
