-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Stricter C99 warnings, strict prototypes for primitives list #11861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Many thanks for your work here, @MisterDA. I really like the style of
the PR, how everything is nicely explained. Thanks!
Regarding your first commit, my main concern is that the list of files
to scan for primitives is now duplicated and I am wondering whether this
duplication could be aovided. On the one hand, thelist of files to scan
is in `runtime/gen_primitives.sh` to make Dune's life easier but on the
other hand, from the persepctive of the main build system, it would be
better to have the list of files only in the root Makefile and then
passed to the script as arguemnts on its command-line. That way, the
script could be called from anywhere, which is not possible at the
moment.
Also, another problem with duplicating the list is that the two versions
will have to be kept in sync, which be fragile and error-prone.
Would it make sense to scan `runtime/*.c` rather than being explicit
about the list?
If that's the case, could this please be done in the Makefile, something
like
```
runtime_PRIMITIVES = $(wildcard runtime/*.c)
```
and then in `runtime/gen_primitives.sh` you would have a for loop that
iterates over `$@` or `$*`, I never know which is the right one. I am
assuming that Dune would also be able to expand `runtime/*.c`, wouldn't
it?
Re: the rule that generates `runtime/primitives`, I'd like to suggest
that you put all the prerequisites on one line and change their order:
```
runtime/primitives.new: runtime/gen_primitives.sh $(runtime_PRIMITIVES)
```
Then, if the names of C sources to scan are passed to the script the
command could be written as `$^ > $@` and if, for some reason we have to
call the script without argumemnts, then the command would be `$< > $@`
so it's simpler no matter how you handle the list of source files to
parse.
Regarding your second commit, perhaps you could use the `SED` build
variable to call the sed that has been detected by configure. In
addition to that, you could add the `LC_ALL=C` bi to the definition of
the `SED` variable in `Makefile.build_config.in`. Concretely speaking,
the line you would add there would be:
```
SED=LC_ALL=C @Sed@
```
In a similar way, you could define
```
SED=LC_ALL=C tr
```
In theory this definition should be added to `Makefile.common` because
it's not yet a configured value, but as it may be the case that we want
`tr` to be found by `configure`, it would seem more future-proof to me
to define `TR` in `Makefile.build_config.in` close to `SED`.
Then in the `Makefile you'll just have to use `$(SED)` and `$(TR)`
rather than `sed` and `tr` to ensure that `LC_ALL` is properly defined
when they are called.
And then you may want to make these variables available to the shell
script but I don't know if it's worth the trouble, except perhaps for
sed because it may be a problem that the script does not use the sed
that has been found by configure.
Nothing to say about the third commit.
And finally, regarding the last commit, here are a few notes about the
commit message itself: I would write `interpreter` (with an e before the
final r) rather than interpretor, but you may wnat to double check. It
may also make the text more readable to mention the origin of the quoted
text before the quoted text rather than after it. For instance: as
stated by ... in .. and then you could even include the URL of the
comment. Also, I was not completely sure whether the explanations should
be in the commit or in the code itself but whatever you decide is fine,
I jsut wanted to suggest this as food for thinking. And finally for the
commit message: `Some reference` should be `Some references`, plural.
Now that you know the PR number I think you can update `Changes`.
And I think that's it for this round of review. :-) And thanks a gain
for your cool work.
|
One other possible way to deal with the
(borrowed from the Makefile of the Linux kernel so I guess it's safe to use) |
Could you please remove those changes? Three reasons: Without those Makefile changes I'll be happy to approve and merge this PR. |
Xavier Leroy (2023/01/11 08:40 -0800):
3- there's too much churn in Makefiles right now, so let's not change
something that is not broken, merely hard to read.
Isn't this a bit contradictory with #11844? ;-)
|
Fedora is considering moving all its packages to C99 and enabling stricter C99 warnings. OCaml has already moved to a compiler requiring C11 features, let's enable these warnings on GCC-compatible compilers. Some of the warnings on the following article are already covered by -Wall and -Werror. Reference: [Modernizing Fedora's C code][1]. [1]: https://lwn.net/Articles/913505/
It is always possible to cast a function pointer to another function pointer type, and cast again to the original type before calling the pointed function. I pick the base typedef value (*c_primitive)(void); to store the functions in the array, and cast them back in the interpreter according to the arity described by the opcode. Some references: - [Function pointers in C][1] - [Function pointer casts][2] [1]: https://frama-c.com/2013/08/24/Function-pointers-in-C.html [2]: https://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf The primitives function pointers can be stored as simple void pointers: (Comment by Xavier Leroy: ocaml#11763 (comment)) > Note that ISO C forbids casting a function pointer to type void *, > but we've been doing this for a very long time, and all C compilers > that I know of are happy with this. This is documented by C11 Standard J.5.7, Function pointer casts. The prototypes in `prims.c` and the headers differ, but: (Comment by Xavier Leroy: ocaml#11763 (comment)) > On the one hand, it's not legal C11 to declare value f(void) a > function f that is defined in another compilation unit as value > f(value), say; while I think it is correct to declare it value f(), > like we currently do. On the other hand, this is no worse than the > many other "not legal C11" things we already do.
484021b
to
8db1817
Compare
Thank you @shindere for the nice review, I've added your suggestions for the commit message. I like to keep longer comments and references in the commit message as not to add too much info in the code. Following Xavier's message I've removed the commits touching the Makefile from this PR. Someone might want to revisit the idea later. This might be good to go now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in a merging mood right now so I decided to interpret the following words of Xavier
Without those Makefile changes I'll be happy to approve and merge this PR.
as saying that I can approve on his behalf now that the changes he requested have been performed.
…void Stricter C99 warnings, strict prototypes for primitives list (cherry picked from commit 0509300)
…void Stricter C99 warnings, strict prototypes for primitives list (cherry picked from commit 0509300)
…void Stricter C99 warnings, strict prototypes for primitives list (cherry picked from commit 0509300)
…void Stricter C99 warnings, strict prototypes for primitives list (cherry picked from commit 0509300)
This PR is a simplified version of #11763, where we don't use the actual types of the primitives when generating the list of primitives, because we end up casting the primitives to a default type anyway (as noted by David Allsopp).
I've retained some of the changes I made to the Makefile, because I think they're easier to understand than the current version.
cc @dra27 @xavierleroy, reviewers of the previous PR.