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

Fix #mod_use in toplevel #9457

Merged
merged 5 commits into from Apr 20, 2020
Merged

Fix #mod_use in toplevel #9457

merged 5 commits into from Apr 20, 2020

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Apr 17, 2020

Regression from #9283. Fixes #9455.

This is an absolute show-stopper for 4.11!

@Octachron
Copy link
Member

It sounds like it would be a good idea to have test?

@dra27
Copy link
Member Author

dra27 commented Apr 17, 2020

I'm just about to push it!

@dra27
Copy link
Member Author

dra27 commented Apr 17, 2020

That test definitely fails on trunk

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Some minor questions/remarks:

  • there is another use of false in use_output, is that one correct?
  • could we maybe use a labelled argument? (non-labelled boolean are a code smell as their call-sites are unreadable)
  • I believe that wrap_mod was the right spelling, not wrap_mode, as I think "mod" refers to "module" here.

@gasche
Copy link
Member

gasche commented Apr 17, 2020

(But maybe wrap_mod could be renamed into wrap_in_module to avoid the confusion completely.)

@dra27
Copy link
Member Author

dra27 commented Apr 17, 2020

Actually, the typo was the other way around - @diml mistyped wrap_mod which is the actual argument name. It reads to me as though Wojciech intended "wrap module" in 2012 when he added #mod_use.

I agree that a labelled argument or renaming would be better, but that code was fine for 8 years prior to being poked for a new feature - and the toplevel is getting a lot of attention elsewhere at the moment, anyway.

The use of false in #use_output is correct - otherwise it'll create an illegal module called (stdin), I think.

@gasche
Copy link
Member

gasche commented Apr 17, 2020

Yes, our comments on wrap_mod agree. (Although I see why it's confusing that I'm commenting on the diff from the other PR). Personally I would prefer to improve the code now that there was a bug, instead of waiting for the next bug. I will push a labeling commit.

@avsm
Copy link
Member

avsm commented Apr 17, 2020

I can confirm that a30e621 fixes the various topfind compilation issues I saw.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the PR. The code looks sensible, the regression is confirmed to be fixed.

@gasche
Copy link
Member

gasche commented Apr 17, 2020

@dra27 I will let you decide if/when to merge given that I was the last to push a commit.

@ghost
Copy link

ghost commented Apr 20, 2020

PR looks good to me too!

@dra27
Copy link
Member Author

dra27 commented Apr 20, 2020

Thanks @diml - let's do it!

@dra27 dra27 merged commit f4dc300 into ocaml:trunk Apr 20, 2020
@dra27 dra27 deleted the fix-mod_use branch April 20, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.11.0~dev breaks use of topfind
4 participants