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

Avoid repeatedly evaluating the source if it's a complex expression #115

Merged
merged 3 commits into from
Sep 11, 2022

Conversation

adamw
Copy link
Member

@adamw adamw commented Sep 11, 2022

Closes #114

@adamw
Copy link
Member Author

adamw commented Sep 11, 2022

@KacperFKorban maybe you could take a look? :)

Copy link
Collaborator

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

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

Ooo that's a really interesting bug. Though I'm glad I wasn't the one reading the generated code snippets 😅

I think that there's a more elegant solution for this. I'm pretty sure that simply removing inline here should fix the problem:

@adamw
Copy link
Member Author

adamw commented Sep 11, 2022

@KacperFKorban Indeed! Nice :) I'm still stuck with Scala2-macro thinking ;) Seems to work - take another look then

Copy link
Contributor

@OndrejSpanel OndrejSpanel left a comment

Choose a reason for hiding this comment

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

Why are those tests in scala-3 specific folder? Would they do any harm in shared? Esp. the functionality tests seems applicable for Scala 2 as well.

Copy link
Collaborator

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

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

LGTM

// #114
it should "not compile for too long in case of chained modify invocations" in {
val start = System.currentTimeMillis()
assertDoesNotCompile("""
Copy link
Contributor

Choose a reason for hiding this comment

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

While the code currently does not compile, would it really be a bug if it did? It does compile in Scala 2. I think assertDoesNotCompile should not be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's in Scala3 sources as assertDoesNotCompile is a Scala3-only thing as far as I know (in ScalaTest).

As for this not compiling... I though that was the point showing the compiler timing issue (which is fixed here), but now I see there are two problems

Copy link
Member Author

Choose a reason for hiding this comment

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

I've extracted this as a separate issue, #116

Copy link
Member Author

Choose a reason for hiding this comment

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

And changed the type here to an unrelated one, which definitely shouldn't work :)

@adamw adamw merged commit 032b503 into master Sep 11, 2022
@mergify mergify bot deleted the cache-obj branch September 11, 2022 18:40
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.

Slow compilation with multiple .modify calls when using Scala3
3 participants