You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2021/02/16 08:02:30 UTC
[GitHub] [groovy] danielsun1106 opened a new pull request #1492: GROOVY-9946: Add `@ConstantString` to mark constant result of `toString`
danielsun1106 opened a new pull request #1492:
URL: https://github.com/apache/groovy/pull/1492
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1492: GROOVY-9946: Add `@Pure` to mark constant result of method
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#discussion_r578090183
##########
File path: src/main/java/org/codehaus/groovy/runtime/GStringImpl.java
##########
@@ -351,15 +353,26 @@ public String toString() {
return str;
}
- private static boolean checkValuesImmutable(Object[] values) {
+ private static boolean checkValuesStringConstant(Object[] values) {
for (Object value : values) {
if (null == value) continue;
- if (!(ImmutablePropertyUtils.builtinOrMarkedImmutableClass(value.getClass())
+ if (!(toStringMarkedPure(value.getClass())
Review comment:
We can probably swap the order of this clause and the next since this one is probably less common than next line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1492: GROOVY-9946: Add `@Pure` to mark constant result of method
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-780956024
@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 ;-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1492: GROOVY-9946: Add `@ConstantString` to mark constant result of `toString`
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-780211010
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1492: GROOVY-9946: Add `@Pure` to mark constant result of method
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-780954105
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] asfgit closed pull request #1492: GROOVY-9946: Add `@Pure` to mark constant result of method
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1492:
URL: https://github.com/apache/groovy/pull/1492
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1492: GROOVY-9946: Add `@ConstantString` to mark constant result of `toString`
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-780294224
OK. Let me try to implement `@Pure` instead of `@ConstantString`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1492: GROOVY-9946: Add `@Pure` to mark constant result of method
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-781029972
Merged, thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1492: GROOVY-9946: Add `@ConstantString` to mark constant result of `toString`
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-779890204
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1492: GROOVY-9946: Add `@ConstantString` to mark constant result of `toString`
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1492:
URL: https://github.com/apache/groovy/pull/1492#issuecomment-779873310
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org