mikemccand / stargazers-migration-test

Testing Lucene's Jira -> GitHub issues migration
0 stars 0 forks source link

Add wrapper class constructors to forbiddenapis [LUCENE-8345] #345

Closed mikemccand closed 6 years ago

mikemccand commented 6 years ago

Wrapper classes for the Java primitives (Boolean, Byte, Short, Character, Integer, Long, Float, Double) have constructors which will always create new objects. These constructors are officially deprecated as of Java 9 and it is recommended to use the public static methods since these can reuse the same underlying objects. In 99% of cases we should be doing this, so these constructors should be added to forbiddenapis and code corrected to use autoboxing or call the static methods (.valueOf, .parse*) explicitly.


Legacy Jira details

LUCENE-8345 by Michael Braun (@michaelbraun) on Jun 03 2018, resolved Jul 16 2018 Pull requests: https://github.com/apache/lucene-solr/pull/392

mikemccand commented 6 years ago

+1

[Legacy Jira: Uwe Schindler (@uschindler) on Jun 03 2018]

mikemccand commented 6 years ago

@thetaphi does this patch look sufficient?

[Legacy Jira: Michael Braun (@michaelbraun) on Jun 05 2018]

mikemccand commented 6 years ago

Hi, looks fine!

I compared it with the autogenerated Java 9 deprecations already shipped with forbiddenapis:

You also added the Character ctor, cool!

I will take this issue, but I am on travel at the moment, so I will commit that tomorrow!

[Legacy Jira: Uwe Schindler (@uschindler) on Jun 05 2018]

mikemccand commented 6 years ago

Maybe we should also add new String(String), has same issue. There is only a legal use case for doing this if you need something that is not identical to an already interened string. But String interning is no longer a use-case in Lucene (since Lucene 4).

[Legacy Jira: Uwe Schindler (@uschindler) on Jun 05 2018]

mikemccand commented 6 years ago

Thanks for the feedback - will have an update in the next day!

[Legacy Jira: Michael Braun (@michaelbraun) on Jun 05 2018]

mikemccand commented 6 years ago

@thetaphi corrected the random T in the text. Should the String add be included as part of this ticket?

[Legacy Jira: Michael Braun (@michaelbraun) on Jun 05 2018]

mikemccand commented 6 years ago

@thetaphi do you want me to update the patch for the latest trunk? Also should the String constructor be added?

[Legacy Jira: Michael Braun (@michaelbraun) on Jul 13 2018]

mikemccand commented 6 years ago

Oh sorry, I will commit this later. I was a bit busy!

I'd like to add the new String(String) constructor, too. But we can also do this in a separate issue.

[Legacy Jira: Uwe Schindler (@uschindler) on Jul 13 2018]

mikemccand commented 6 years ago

@thetaphi updated the PR, there was an additional change needed for trunk to pass the new forbidden apis check.

[Legacy Jira: Michael Braun (@michaelbraun) on Jul 14 2018]

mikemccand commented 6 years ago

OK, thanks!

[Legacy Jira: Uwe Schindler (@uschindler) on Jul 14 2018]

mikemccand commented 6 years ago

Commit fb6574100e493ed9162265133f2cbe967746c166 in lucene-solr's branch refs/heads/master from Michael Braun https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fb65741

LUCENE-8345 - add wrapper class constructors to forbiddenapis

[Legacy Jira: ASF subversion and git services on Jul 16 2018]

mikemccand commented 6 years ago

Commit c97f27b06c1d7c250e9596a9bc7bf5ca11ef6ad3 in lucene-solr's branch refs/heads/master from @thetaphi https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c97f27b

Merge branch 'remove-constructor-wrapper-classes' of https://github.com/michaelbraun/lucene-solr: LUCENE-8345, GitHub PR #392: Remove instantiation of redundant wrapper classes for primitives; add wrapper class constructors to forbiddenapis. This closes #392

[Legacy Jira: ASF subversion and git services on Jul 16 2018]

mikemccand commented 6 years ago

Hi, I merged your branch into master and closed the PR. I will let jenkins run for a few hours and then try to cherry-pick to branch_7x. If there are major problems (quite many files touches), I will contact you, @mbraun688. No need to take action now.

[Legacy Jira: Uwe Schindler (@uschindler) on Jul 16 2018]

mikemccand commented 6 years ago

The cherry-pick worked without any conflicts. I am now running forbiddenapis to find any additional violations in branch_7x. If there are none, I will push the changes ASAP.

[Legacy Jira: Uwe Schindler (@uschindler) on Jul 16 2018]

mikemccand commented 6 years ago

Commit 8f209f66ff7494567c355900657aa883d653b9a8 in lucene-solr's branch refs/heads/branch_7x from Uwe Schindler https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8f209f6

Merge branch 'remove-constructor-wrapper-classes' of https://github.com/michaelbraun/lucene-solr: LUCENE-8345, GitHub PR #392: Remove instantiation of redundant wrapper classes for primitives; add wrapper class constructors to forbiddenapis. This closes #392

[Legacy Jira: ASF subversion and git services on Jul 16 2018]

mikemccand commented 6 years ago

Hi, forbiddenapis was happy after the cherry-pick. I pushed the changes to 7.x, too. Thanks @michaelbraun!

[Legacy Jira: Uwe Schindler (@uschindler) on Jul 16 2018]