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

I18628 #18841

Merged
merged 4 commits into from
Nov 9, 2023
Merged

I18628 #18841

merged 4 commits into from
Nov 9, 2023

Conversation

EnzeXing
Copy link
Contributor

@EnzeXing EnzeXing commented Nov 3, 2023

This PR fixes the minimized test of #18628 that previously runs infinitely on the global initialization checker.

@sjrd sjrd requested a review from olhotak November 3, 2023 16:31
@sjrd sjrd requested a review from liufengyun November 3, 2023 16:31
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot.

Please add the long test as a positive test (no warnings).

@EnzeXing
Copy link
Contributor Author

EnzeXing commented Nov 3, 2023

Please add the long test as a positive test (no warnings).

@liufengyun With this fix I'm afraid the long test cannot pass the checker since the environment of repMany will eventually widen the by-name parameters to Cold

@liufengyun
Copy link
Contributor

Please add the long test as a positive test (no warnings).

@liufengyun With this fix I'm afraid the long test cannot pass the checker since the environment of repMany will eventually widen the by-name parameters to Cold

It should work --- I checked on my machine, it works as expected. The case is OK, because the recursive call is by-name.

Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

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

I confirm that the long test also works for me. (It gives a warning but does not run out of memory). Please add it, but otherwise LGTM.

@EnzeXing
Copy link
Contributor Author

EnzeXing commented Nov 3, 2023

I confirm that the long test also works for me. (It gives a warning but does not run out of memory). Please add it, but otherwise LGTM.

It is the same for me, but @liufengyun do you expect the test to pass without warning?

@liufengyun
Copy link
Contributor

I confirm that the long test also works for me. (It gives a warning but does not run out of memory). Please add it, but otherwise LGTM.

It is the same for me, but @liufengyun do you expect the test to pass without warning?

Sorry, I made a mistake in my test. Yes, it produce warnings. But it shouldn't --- It seems we have a problem handling the local lazy vals.

Just change the following method in the test from

    def ~ [U](q: => Parser[U]): Parser[~[T, U]] = { lazy val p = q // lazy argument
      (for(a <- this; b <- p) yield new ~(a,b))
    }

to

    def ~ [U](q: => Parser[U]): Parser[~[T, U]] = {
      (for(a <- this; b <- q) yield new ~(a,b))
    }

will suppress the warning.

@liufengyun liufengyun merged commit 1c3a1ae into scala:main Nov 9, 2023
18 checks passed
@liufengyun liufengyun deleted the i18628 branch November 9, 2023 06:40
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants