groovy

FindBugs errors for new Integer() vs. Integer.valueOf() like calls

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.5, 1.8-beta-2
  • Fix Version/s: 1.7.5, 1.8-beta-2
  • Component/s: None
  • Labels:
    None
  • Number of attachments :
    2

Description

Groovy compiler seems to be generating calls like {{ new Integer() }} and {{ new Long() }} (also for other number wrapper types), which FindBugs tool calls out as performance issues and suggests that the equivalent calls {{ Integer.valueOf() }} and {{ Long.valueOf() }} be used.

Class generation needs to change a bit to support it to reduce the errors reported by FindBugs for groovy classes.

  1. File
    4412_v18x.diff
    11/Sep/10 1:16 PM
    3 kB
    Roshan Dawrani
  2. File
    v2_4412_v18x.diff
    11/Sep/10 2:47 PM
    4 kB
    Roshan Dawrani

Issue Links

relates to

Improvement - An improvement or enhancement to an existing feature or task. GROOVY-4152 Groovy code using constants seems unsuitable for hotswap

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Roshan Dawrani added a comment -

Patch attached for review.

Show
Roshan Dawrani added a comment - Patch attached for review.
Hide
Permalink
blackdrag blackdrag added a comment -

the patch looks mostly good to me... good but one point... may I suggest to split the number case in such with valueOf and those with constructor to handle the constructor version like in BigDecimal before the remaining mnumber cases? Then we can use for int, long and such also the full signature instead of having to build it there every time.

Show
blackdrag blackdrag added a comment - the patch looks mostly good to me... good but one point... may I suggest to split the number case in such with valueOf and those with constructor to handle the constructor version like in BigDecimal before the remaining mnumber cases? Then we can use for int, long and such also the full signature instead of having to build it there every time.
Hide
Permalink
Roshan Dawrani added a comment -

Silly mistake. It was like trading one performance issue for another due to those string concatenations.

I am attaching the revised patch.

Show
Roshan Dawrani added a comment - Silly mistake. It was like trading one performance issue for another due to those string concatenations. I am attaching the revised patch.
Hide
Permalink
blackdrag blackdrag added a comment - - edited

well that was a performance bug in the compiler instead of the generated class...anyway.. ehm.. I hope you don't fell annoyed yet, but I was actually thinking about checking for the Bigs before the others and then only check for Number before breaking up into the concrete parts for Short Integer and such.

Meaning more or less the reverse order of what you have

Show
blackdrag blackdrag added a comment - - edited well that was a performance bug in the compiler instead of the generated class...anyway.. ehm.. I hope you don't fell annoyed yet, but I was actually thinking about checking for the Bigs before the others and then only check for Number before breaking up into the concrete parts for Short Integer and such. Meaning more or less the reverse order of what you have
Hide
Permalink
Roshan Dawrani added a comment - - edited

No problem. Since that if-else cannot have a 3rd branch, I will reverse the order and check-in.

Show
Roshan Dawrani added a comment - - edited No problem. Since that if-else cannot have a 3rd branch, I will reverse the order and check-in.

People

  • Assignee:
    Roshan Dawrani
    Reporter:
    Roshan Dawrani
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: