3

I am using a string as a lock and so want to ensure the object is a new instance. FindBugs complains because it's generally more efficient to define the string directly (with double quotes). My code looks like:

/** A lock for the list of inputs. */
@edu.umd.cs.findbugs.annotations.SuppressWarnings("DM_STRING_CTOR")
//We want a new String object here as this is a lock.
private final Object inputListLock = new String("inputListLock");

Am I doing something wrong here? The Eclipse FindBugs plugin is still reporting this as a problem:

Pattern id: DM_STRING_CTOR, type: Dm, category: PERFORMANCE

Using the java.lang.String(String) constructor wastes memory because the object so constructed will be functionally indistinguishable from the String passed as a parameter.  Just use the argument String directly.
tttppp
  • 7,723
  • 8
  • 32
  • 38
  • 2
    Why not just solve the problem? – musiKk Oct 20 '10 at 09:33
  • I was under the impression that replacing the new String("inputListLock") with "inputListLock" could potentially cause problems if "inputListLock" is used elsewhere. By defining the string as a new object I think it avoids this problem as there will then be two objects with the value "inputListLock". Maybe this is wrong? – tttppp Oct 20 '10 at 09:40
  • Java internalizes strings and shares pointers from the string pool, this is why strings are immutable. You may not necessarily have the second object that you had hoped for ... good thing you didn't suppress the findbugs warning. – crowne Oct 20 '10 at 10:04
  • 1
    What is the question about? Why does the Eclipse FindBugs plugin still complains despite the use of FindBugs `@SuppressWarnings`? Or what is the right way to do this? Or both? – Pascal Thivent Oct 20 '10 at 10:14
  • @Pascal The question is meant to be why does the Eclipse FindBugs plugin still complain. However as a side issue I am also interested in comments about the code itself. – tttppp Oct 20 '10 at 10:31
  • 2
    Ok, that was my initial understanding (and I think the current answers are not answering the question, or at least only partially). – Pascal Thivent Oct 20 '10 at 10:40
  • @PascalThivent Agreed, no-one so far has provided a satisfactory solution to the actual question ("why can't I suppress the warning"), and even the accepted answer has a serious flaw... – Andres F. Jul 26 '16 at 14:50

3 Answers3

5

Why not just declare the lock object as a new Object? You don't need to make it a String, since you don't do anything that requires the String-ness of the lock, and presumable you don't use it for anything other than locking.

Without seeing the rest of your code I can hazard a guess that you're locking on access to a list of some kind. You could use the list itself as the lock object. If it's private then there is no chance that someone else will cause a deadlock.

Cameron Skinner
  • 51,692
  • 2
  • 65
  • 86
2

The normal idiom is to do this:

private final Object inputListLock = new Object();

which saves space (relative to new String("someLock")) and gets rid of the pesky PMD warning. But if you really want the lock to be a String, there are other ways to create a copy of a String that PMD is unlikely to object to; e.g. 

private final Object inputListLock = "some".concat("Lock");

(Note that "someLock".concat("") doesn't actually create a new String!)

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
1

Ok, so although both the other answers were interesting and useful (+1 for both), I didn't end up changing the code and I'm going to accept my own answer. To satisfy FindBugs I moved the annotation from the member variable to the surrounding class.

I've looked for some time but I haven't found any information suggesting that the SuppressWarnings may only be applied to classes and methods. Neither have I found any examples of it being applied to member variables. So though this solution works I don't know that it's the 'right' solution (maybe there's still something wrong with my FindBugs/Eclipse setup for example).

tttppp
  • 7,723
  • 8
  • 32
  • 38
  • What you wrote is NOT the right solution, since suppressing at the class level will suppress every occurrence of the bug, not only the one you're currently interested in. This also includes future occurrences of the bug! Consider this scenario: FindBugs detects a false positive, which you can prove is false, and therefore you suppress it by annotating the class. Then some time later you inadvertently introduce a new, legitimate bug of the same type. Because of the annotation, FindBugs will never warn you about it, which is definitely not what you want. – Andres F. Jul 26 '16 at 14:46
  • PS: for what it's worth, this problem also happens with [FindSecBugs](http://find-sec-bugs.github.io/) (a security plugin for FindBugs) and the new annotation `@SuppressFBWarnings`. – Andres F. Jul 26 '16 at 14:47