R devel - Mar 2023 - Making headers self-contained for static analysis

Hello,

I started using clangd to get better static analysis and code
refactoring tooling with the R sources (using eglot-mode in Emacs, it
just works once you've generated a `compile_commands.json` file with
`bear make all`). I noticed that the static analyser can't understand
several header files because these are not self-contained. So I went
through all .h files and inserted the missing includes, cf the
attached patch.

Making the headers self-contained has consequences for the .c or .cpp
files that include them. In the case of C files, the only downside I
see is that it might cause users to accidentally rely on indirect
inclusion of standard headers, instead of directly including the
header to make the dependency explicit as would be good practice.
This doesn't seem like a big deal compared to the benefits of enabling
static analysis.

However in the case of C++ that's more problematic. We don't want to
include the C headers because that would pollute the global namespace
and users might prefer to import the missing symbols (`size_t` and
`FILE`) selectively. Also that wouldn't help static analysis within
the header files since the analysers use the C path. So I have guarded
inclusion of standard C headers behing a `__cplusplus` check.

If that makes sense, would R core consider applying the attached patch
to the R sources?

Best,
Lionel

Hello Lionel,

Just letting you know off-list that the patch didn't make it through.
Unfortunately, I don't remember the exact rules regarding attachments
(does text/plain work?), but an external link is always an option,
especially for large patches.

-- 
Best regards,
Ivan

People have let me know that the attachment didn't make it through.
Do patches get filtered out?

Please find it there:
https://github.com/lionel-/r-svn/commit/e3de56798b1321a3fa8688a42bbb73d763b78024.patch

I'm also happy to post it on the bugzilla if that makes sense.

Best,
Lionel

On 3/16/23, Lionel Henry <lionel at posit.co>
wrote:
> Hello,
>
> I started using clangd to get better static analysis and code
> refactoring tooling with the R sources (using eglot-mode in Emacs, it
> just works once you've generated a `compile_commands.json` file with
> `bear make all`). I noticed that the static analyser can't understand
> several header files because these are not self-contained. So I went
> through all .h files and inserted the missing includes, cf the
> attached patch.
>
> Making the headers self-contained has consequences for the .c or .cpp
> files that include them. In the case of C files, the only downside I
> see is that it might cause users to accidentally rely on indirect
> inclusion of standard headers, instead of directly including the
> header to make the dependency explicit as would be good practice.
> This doesn't seem like a big deal compared to the benefits of enabling
> static analysis.
>
> However in the case of C++ that's more problematic. We don't want
to
> include the C headers because that would pollute the global namespace
> and users might prefer to import the missing symbols (`size_t` and
> `FILE`) selectively. Also that wouldn't help static analysis within
> the header files since the analysers use the C path. So I have guarded
> inclusion of standard C headers behing a `__cplusplus` check.
>
> If that makes sense, would R core consider applying the attached patch
> to the R sources?
>
> Best,
> Lionel
>