Skip to content

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
@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

4 participants