Skip to content
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

Minor cleanup around printing in the compiler (warnings, errors, etc) #1906

Merged
merged 5 commits into from Jul 17, 2018

Conversation

Projects
None yet
5 participants
@sliquister
Copy link
Contributor

commented Jul 15, 2018

If you look at this pull request, look commit by commit.

I was trying to understand/change what gets printed in these ppf formatters that get passed around in various places in the compiler. While doing this, I noticed that it's almost only the output of -dlambda, -dcmm etc, but a couple places also print warnings and fatals errors in there, whereas the rest of the compiler uses respectively Location.formatter_for_warnings and stderr for that.
So I bring these places in line with the rest of the compiler in the 1st, 3rd and 5th commit.

While working on the 3rd commit, I fixed missing warnings documentation (2nd commit) and stumbled once more on this Clflags.fast variable, whose name made sense 20 years ago when -unsafe was called -fast but not anymore, so I fixed that too (4th commit) (and while doing that renaming, I noticed that -unsafe removes the check for division and modulo by zero, which is not the documented behavior, that 0/0 is "optimized" from sigfpe to ocaml exception with -unsafe, and that this optimization happens through Sys.opaque_identity, but I didn't change these).

sliquister added some commits Jul 15, 2018

Print warnings on the warnings formatter in the couple of places that…
… don't

Except the OCAMLPARAM stuff, as that runs before the command line
warning settings are even parsed, so while they are reported using the
normal warnings code, they don't look like normal warnings.
Create warning 64, for uses of -unsafe with a -pp that returns a mars…
…halled ast

Instead of the current print to stderr. This way it's treated the same
as other warnings: it has a position, colors, can be made an error,
disabled, goes in the expected formatter, is documented.
rename Clflags.fast as Clflags.unsafe
The variable and the flag got out in sync in
2977939.
liveness: print fatal errors fully on stderr
instead of half on stderr and half the ppf argument
@gasche

This comment has been minimized.

Copy link
Member

commented Jul 15, 2018

All commits look like nice simplifications to me, thanks!

I think that the most controversial one is 3d0299a, which adds a new warning for something that is arguably fairly inconsequential. It's not clear to me that printing a warning here is a good idea; could you maybe trace when it was added? maybe @alainfrisch has opinions as well.

Overall I like the simplifications of the code and you have clearly been very thoughful when working on this, so my vote would go to trust you and merge, even if I'm not fully convinced by the new warning. I'll let others a change to give an opinion as well.

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2018

Thanks for the review.
Unsurprisingly, the warning was added by de Rauglaudre in 2002, so people using camlp4 were confused by -unsafe not working. If you use -pp and -unsafe it does seem like a useful warning. Granted, -pp is rarer nowadays, but you use -ppx you're never going to see it anyway, so it's not like there's much downside.

@gasche

gasche approved these changes Jul 16, 2018

Copy link
Member

left a comment

I approve the PR, but will still give a day or two for others to comment if they wish to.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

I'm also ok with all changes. No strong opinion on 3d0299a. It feels both a bit useless and harmless to introduce a warning for such use case. (I would have said the same thing about just dropping the previous pseudo-warning; except that it would have simplified the code instead of "complexifying" it.)

@gasche gasche merged commit dca053d into ocaml:trunk Jul 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sliquister sliquister deleted the sliquister:doutput-prepare branch Jul 17, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

FTR, commit 3d0299a resolves one item of MPR#6080.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I just noticed this now, but isn't the new warning 64 not actually needed? Once upon a time the -unsafe option was implemented in the parser and so skipping the parser with a marshalled AST would mean it was ignored. However, it has long since been implemented in the back-end. This means that using a marshalled AST doesn't skip the handling of unsafe and so the warning seems completely unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.