Skip to content

GROOVY-9946: Add @Pure to mark constant result of method#1492

Closed
daniellansun wants to merge 1 commit intomasterfrom
GROOVY-9946
Closed

GROOVY-9946: Add @Pure to mark constant result of method#1492
daniellansun wants to merge 1 commit intomasterfrom
GROOVY-9946

Conversation

@daniellansun
Copy link
Copy Markdown
Contributor

@daniellansun daniellansun commented Feb 16, 2021

@eric-milles
Copy link
Copy Markdown
Member

If this annotation is a marker and not a transform, is there a better package for it? I am not understanding the value proposition here. The ticket does not describe why this would be useful. Is the recreation of GString value a performance problem in some context?

@daniellansun
Copy link
Copy Markdown
Contributor Author

If this annotation is a marker and not a transform, is there a better package for it?

Any suggestion is welcome. Currently I put it with @KnownImmutable in the same package. /cc @paulk-asert

The ticket does not describe why this would be useful.

Currently we just cache literal of GString when its all values are immutable, but for more common cases, if the result of value's toString is constant, we could cache too.

@paulk-asert
Copy link
Copy Markdown
Contributor

I am thinking this could be generalised. If instead of having a very specific class annotation, we had a @Pure or @Constant annotation and place it on the toString method itself, then the annotation could be used in more places. @Memoized could annotate the methods it generates (giving the caching optimization for free on memoized toString methods) and future type checkers could ensure that the return value of "pure" methods only comes from expressions involving constants or other pure methods - kind of what Frege does.

@daniellansun
Copy link
Copy Markdown
Contributor Author

OK. Let me try to implement @Pure instead of @ConstantString.

@daniellansun daniellansun changed the title GROOVY-9946: Add @ConstantString to mark constant result of toString GROOVY-9946: Add @Pure to mark constant result of method Feb 17, 2021
@paulk-asert
Copy link
Copy Markdown
Contributor

LGTM. The one thing which I think we should do is move the @Pure marker interface into a groovy.annotations package in its own groovy-annotations mandatory module. However, I can do that move later if you like. That would be a good place for other such marker interfaces to also go. Then folks writing Java libraries could make them more Groovy friendly in some places and only need worry about having that one module available as a compile-time only dependency for their builds.

@daniellansun
Copy link
Copy Markdown
Contributor Author

@paulk-asert

LGTM. The one thing which I think we should do is move the @pure marker interface into a groovy.annotations package in its own groovy-annotations mandatory module. However, I can do that move later if you like.

Thanks for your reviewing and help ;-)

for (Object value : values) {
if (null == value) continue;
if (!(ImmutablePropertyUtils.builtinOrMarkedImmutableClass(value.getClass())
if (!(toStringMarkedPure(value.getClass())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can probably swap the order of this clause and the next since this one is probably less common than next line.

@asfgit asfgit closed this in f4323a3 Feb 18, 2021
@paulk-asert
Copy link
Copy Markdown
Contributor

Merged, thanks!

@eric-milles eric-milles deleted the GROOVY-9946 branch April 26, 2021 12:21
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.

3 participants