GROOVY-11309 Optimise bytecode for empty list expressions

[PaintNinja]

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (359ee64) 68.5308% compared to head (749e225) 68.5283%.

Additional details and impacted files

@@                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	Coverage Δ
...rg/codehaus/groovy/classgen/AsmClassGenerator.java	84.1482% <100.0000%> (+0.0826%)	:arrow_up:

... and 2 files with indirect coverage changes

[paulk-asert]

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.

[PaintNinja]

Thanks. In that case, would you like me to add a code comment to ScriptBytecodeAdapter#createList, referencing this change?

[paulk-asert]

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.

[daniellansun]

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.

[paulk-asert]

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.