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

Move Location.absname to Clflags.absname #1886

Merged
merged 2 commits into from Jul 6, 2018

Conversation

Projects
None yet
3 participants
@Armael
Copy link
Member

commented Jul 5, 2018

A comment in location.ml indicates that the reference for the absname option should be defined in Clflags, but is defined in Location because otherwise it would complicate Camlp4's bootstrap.

Since Camlp4 is now packaged separately, maybe this is not the case anymore? If so, this PR moves the absname reference to Clflags where it apparently belongs.

@Armael Armael force-pushed the Armael:location-absname branch from bbf07ca to e3f2c54 Jul 5, 2018

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

Ping @diml .

@diml

diml approved these changes Jul 6, 2018

@diml

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

There are a few more instances to fix (see CI failures) but otherwise this change looks fine to me.

grep -r absname . in camlp4 yielded no result, so no worries on this side.

@Armael Armael force-pushed the Armael:location-absname branch from e3f2c54 to 4b8c135 Jul 6, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

Oops, hopefully the remaining occurrences are now fixed.

@diml

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

Thanks, could you also add a changelog entry in the section Internal/compiler-libs changes?

@Armael

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

Done, thanks for the review.

@diml

diml approved these changes Jul 6, 2018

@diml

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

No problem, thanks for the PR. We'll merge as soon as CI finishes

@diml diml merged commit 7c9c210 into ocaml:trunk Jul 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.