You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "PaintNinja (via GitHub)" <gi...@apache.org> on 2024/02/07 21:28:19 UTC
[PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
PaintNinja opened a new pull request, #2054:
URL: https://github.com/apache/groovy/pull/2054
(no comment)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2054:
URL: https://github.com/apache/groovy/pull/2054#issuecomment-1935490334
> Thanks. In that case, would you like me to add a code comment to ScriptBytecodeAdapter#createList, referencing this change?
Probably not needed in this case.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2054:
URL: https://github.com/apache/groovy/pull/2054#issuecomment-1933522752
This looks okay to me. But, by way of background, if we ever wanted to change the default list in Groovy from ArrayList to LinkedList (not that we would), we would now have one more place to make a change. So, if we thought such changes might be on the cards, this would increase the maintenance burden. But I think we are okay here.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "PaintNinja (via GitHub)" <gi...@apache.org>.
PaintNinja commented on PR #2054:
URL: https://github.com/apache/groovy/pull/2054#issuecomment-1933744394
Thanks. In that case, would you like me to add a code comment to ScriptBytecodeAdapter#createList, referencing this change?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert closed pull request #2054: GROOVY-11309 Optimise bytecode for empty list expressions
URL: https://github.com/apache/groovy/pull/2054
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2054:
URL: https://github.com/apache/groovy/pull/2054#issuecomment-1933196729
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2054?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`359ee64`)](https://app.codecov.io/gh/apache/groovy/commit/359ee64eda6bdafde7fa5c53ccab38f5e89df493?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 68.5308% compared to head [(`749e225`)](https://app.codecov.io/gh/apache/groovy/pull/2054?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 68.5283%.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2054?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2054 +/- ##
==================================================
- Coverage 68.5308% 68.5283% -0.0025%
+ Complexity 29178 29177 -1
==================================================
Files 1422 1422
Lines 113495 113502 +7
Branches 19548 19549 +1
==================================================
+ Hits 77779 77781 +2
- Misses 29178 29181 +3
- Partials 6538 6540 +2
```
| [Files](https://app.codecov.io/gh/apache/groovy/pull/2054?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rg/codehaus/groovy/classgen/AsmClassGenerator.java](https://app.codecov.io/gh/apache/groovy/pull/2054?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL0FzbUNsYXNzR2VuZXJhdG9yLmphdmE=) | `84.1482% <100.0000%> (+0.0826%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2054/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2054:
URL: https://github.com/apache/groovy/pull/2054#issuecomment-1936835711
How about creating empty map, empty array?
IMHO, both maintenance and flexibility are important, but the PR reduces them in the same time...
-1 from me.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11309 Optimise bytecode for empty list expressions [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2054:
URL: https://github.com/apache/groovy/pull/2054#issuecomment-1953451647
I'll close this PR since a -1 vote for a code change is an absolute veto:
https://www.apache.org/foundation/voting.html
I have kept it open for a while since the normal process would be to continue debate and see if the -1 vote is rescinded. That didn't happen here (yet), but feel free to create a new PR with some of @daniellansun's points addressed if you wish, or start a thread on the mailing list to avoid further disappointment.
For me personally, I'd like to explore performing a change like what you suggested when the declaring class is marked with POJO, but I'd like to see it handle more than just the empty case. In that case, we explicitly want to avoid calling back through ScriptBytecodeAdapter, so the direction you are heading makes sense. I personally don't mind if empty map, etc. aren't covered up front since we often make such changes iteratively, and we could handle that next.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org