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

Clean up int literal hint #2313

Merged
merged 3 commits into from Apr 11, 2019
Merged

Clean up int literal hint #2313

merged 3 commits into from Apr 11, 2019

Conversation

@Julow
Copy link
Contributor

@Julow Julow commented Mar 12, 2019

As @gasche pointed out on my previous PR, my two previous PR need a small cleanup.

I changed Location.report to allow sub messages without location and print the int literal hint as a sub message.

@Armael
Copy link
Member

@Armael Armael commented Mar 12, 2019

You don't need to change Location.report to have sub-messages without a (displayed) location: you can do that by setting the sub-message location to Location.none.

@Julow
Copy link
Contributor Author

@Julow Julow commented Mar 12, 2019

@Armael I reverted and use Location.none. This added 2 spaces at the beginning of the line, what do you think of my last commit ?

@Julow Julow force-pushed the Julow:hint_handling branch from 417c769 to fdbe6f4 Mar 13, 2019
@Julow
Copy link
Contributor Author

@Julow Julow commented Mar 13, 2019

@Armael I finally reverted all my changes to location.ml.

Copy link
Member

@Armael Armael left a comment

Sorry for the delay on reviewing this. The patch looks good, I agree it's an improvement.
I left a couple comments, but I'm ready to merge after they are addressed.

typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.ml Outdated Show resolved Hide resolved
@Julow
Copy link
Contributor Author

@Julow Julow commented Apr 10, 2019

Thanks for the review !

#2319 may not be merged because my PRs are adding code that should not be in the type checker. Do you think moving error-reporting code to another module is feasible or is a good solution ?

typing/typecore.ml Outdated Show resolved Hide resolved
@Armael
Copy link
Member

@Armael Armael commented Apr 10, 2019

Let's not mix this PR and #2319 . And note that in the current situation, creating new modules in the compiler is a bit delicate, since they pollute the global namespace for anyone linking against compiler-libs...

@Armael Armael merged commit e728886 into ocaml:trunk Apr 11, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants