Re: [Rd] Making headers self-contained for static analysis
Hi, There are a limited number of MIME file types that are accepted through the list server, with plain text being one. Even though a patch file should be plain text, it is possible that your mail client may not have set the correct MIME type for your patch file attachment. If so, that would explain why it was filtered from the list. For future reference, if you change the file extension to ".txt" and then attach it, that should get picked up as plain text and get through the list server filters. Regards, Marc Schwartz R-Devel Co-Admin On March 16, 2023 at 2:32:39 PM, Lionel Henry via R-devel (r-devel@r-project.org (mailto:r-devel@r-project.org)) wrote: > 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 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 > > > > __ > R-devel@r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Making headers self-contained for static analysis
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 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 > __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Making headers self-contained for static analysis
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 __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
[Rd] 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 __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
