Skip to content

[java] Update rule AvoidArrayLoops#3408

Merged
oowekyala merged 3 commits into
pmd:pmd/7.0.xfrom
adangel:pmd7-update-AvoidArrayLoops
Aug 7, 2021
Merged

[java] Update rule AvoidArrayLoops#3408
oowekyala merged 3 commits into
pmd:pmd/7.0.xfrom
adangel:pmd7-update-AvoidArrayLoops

Conversation

@adangel

@adangel adangel commented Jul 15, 2021

Copy link
Copy Markdown
Member

@adangel adangel added this to the 7.0.0 milestone Jul 15, 2021
This was referenced Jul 15, 2021
Closed
Merged
@ghost

ghost commented Jul 16, 2021 •
edited by ghost
Loading

Copy link
Copy Markdown
2 Messages
📖 Compared to pmd/7.0.x:
This changeset changes 0 violations,
introduces 25 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
📖 Compared to master:
This changeset changes 30762 violations,
introduces 32724 new violations, 3 new errors and 0 new configuration errors,
removes 141564 violations, 8 errors and 3 configuration errors.
Full report
✅ Compared to pmd/7.0.x:
This changeset changes 0 violations,
introduces 134 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
✅ Compared to master:
This changeset changes 30227 violations,
introduces 32803 new violations, 3 new errors and 0 new configuration errors,
removes 143848 violations, 8 errors and 3 configuration errors.
Full report

Generated by 🚫 Danger

@adangel

adangel commented Jul 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author
  1. false positive:
table[i] = labels[offset + readInt(u)];

(https://github.com/spring-projects/spring-framework/blob/v5.0.6.RELEASE/spring-core/src/main/java/org/springframework/asm/ClassReader.java#L1595)

That's not simply done by calling System.arraycopy. The array index on the rhs is calculated via readInt... and it is not based on i (so, no fixed offset)

  1. false positive
S[i] = S[j];
S[j] = Si;

(https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/com/sun/crypto/provider/ARCFOURCipher.java#L78)

Not so simple either. It's the same array - so the content is shifted. Not a use case for Arrays.copyOf...

  1. valid, correct case
                for (int i=0; i<time.length; i++) {
                    blob[8+time.length-i-1] = time[i];
                }

(https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/com/sun/security/ntlm/Client.java#L168)

I think, that's actually correct. This could indeed be rewritten as: System.arraycopy(time, 0, blob, 8+time.length-1, time.length).

  1. false positive
        eachArg:
            for (int j = 0; j < arguments.length; j++) {
                if (arguments[j] instanceof Name) {
                    Name n = (Name) arguments[j];
                    int check = n.index;
                    // harmless check to see if the thing is already in newNames:
                    if (check >= 0 && check < newNames.length && n == newNames[check])
                        continue eachArg;
                    // n might not have the correct index: n != oldNames[n.index].
                    for (int i = start; i < end; i++) {
                        if (n == oldNames[i]) {
                            if (n == newNames[i])
                                continue eachArg;
                            if (!replaced) {
                                replaced = true;
                                arguments = arguments.clone();
                            }
                            arguments[j] = newNames[i];
                            continue eachArg;
                        }
                    }
                }
            }

(https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/lang/invoke/LambdaForm.java#L1453)

There are two for-loops. I guess, we detected the inner loop, but for some reason, we reported the outer loop. The copy might be arguments[j] = newNames[i];, but a continue statement is following, which makes this not a straightforward copy...

  1. false positive
        for (int i = 0; i < uVlen; i++) {
            int nextp = a[headp];
            a[headp] = uValues[i];
            headp = nextp;
        }

(https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/com/sun/java/util/jar/pack/PopulationCoding.java#L299)

for the array assignment on the lhs the loop index is not used.

  1. false positive
        for (int i = srcBegin + (1 >> LO_BYTE_SHIFT); i < srcEnd; i += 2) {
            dst[dstBegin++] = value[i];
        }

(https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/lang/StringUTF16.java#L263)

Update: These cases are now fixed ✔️

@oowekyala oowekyala self-requested a review July 16, 2021 12:40
@oowekyala oowekyala self-assigned this Aug 7, 2021
@oowekyala oowekyala merged commit 607faa1 into pmd:pmd/7.0.x Aug 7, 2021
@adangel adangel deleted the pmd7-update-AvoidArrayLoops branch August 19, 2021 08:28
@adangel adangel mentioned this pull request Jan 23, 2023
Closed
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oowekyala oowekyala oowekyala approved these changes

Assignees

@oowekyala oowekyala

Labels

None yet

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@adangel @oowekyala