Method invocation XXX may produce NPE
This inspection usually works pretty but now I got some wrong results.
Ascii art follows :)
@Nullable private RfxStructureNode parent;
public boolean isLast()
{
if(parent==null) return true;
return parent.indexOf(this)+1==parent.getChildCount();
^^^^^^^^^^^^^^^^^^^^^^
}
public RfxNode getPreviousSibling()
{
if(parent==null) return null;
int pos=parent.indexOf(this);
return pos>0 ? parent.getChildren().get(pos-1) : null;
^^^^^^^^^^^^^^^^^^^^
}
public RfxNode getNextSibling()
{
if(parent==null) return null;
int pos=parent.indexOf(this);
return pos+1<parent.getChildCount() ?
^^^^^^^^^^^^^^^^^^^^^^
parent.getChildren().get(pos+1) : null;
^^^^^^^^^^^^^^^^^^^^
}
Occurs in build 3419.
Please sign in to leave a comment.
These cases come about essentially because JB are taking into account
the fact that your app might be multi-threaded, and so the field parent
can be legally set to null at any time.
See, e.g. http://www.jetbrains.net/jira/browse/IDEA-2068
R
I can reproduce this in build 3425 and I think it a bug. Have you
submitted a JIRA issue?
Bas
Sven Steiniger wrote:
Not only multi-threaded but with current annotation system we can't be sure
calling parent.indexOf(...) will not set parent to null via some chained
calls.
-
Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"
So workaround would be introducing local variable for the parent.
-
Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"
Whoops, I guess I was too tired to see that obviousness there.
Bas
Maxim Shafirov (JetBrains) wrote:
>> the fact that your app might be multi-threaded
!?!! I can accept the reason that parent.indexOf() may change the parent
field. That's ugly, but correct (at least if you don't analyse the
called method :)
But multi-threaded access should not considered at all. In this case any
test would fail. eq. "if(parent!=null) parent.doSomething()" can fail in
multi-thread access.
Maxim Shafirov (JetBrains) schrieb:
>> the fact that your app might be multi-threaded
I disagree about ugliness of workaround. I personally create local
variables for any field which I reference more than once in a method,
and I know others who suggest such style.
Max, maybe in this case the inspection could suggest creating a local
variable for the field. People have posted about this several times.
Maxim Shafirov (JetBrains) wrote:
Sven Steiniger wrote:
That's true, and you should not use that construct, and IDEA should warn
about it, I think.
That's why 'synchronized' has been invented... :)
Keith Lea wrote:
>> !?!! I can accept the reason that parent.indexOf() may change the parent
>> field. That's ugly, but correct (at least if you don't analyse the
>> called method :)
>>
>> But multi-threaded access should not considered at all. In this case
>> any test would fail. eq. "if(parent!=null) parent.doSomething()" can
>> fail in
>> multi-thread access.
--
Martin Fuhrer
Fuhrer Engineering AG
http://www.fuhrer.com
Sadly, that would result in too many false positives. There's plenty of valid use cases for such constructs. For instance, you may know that all instances of a class are used locally to one thread, and thus no synchronization is necessary.
I'm pretty sure that the data-flow inspections don't take multi-threading into account.
--Dave Griffith
Even without multithreading, I think reading that field over and over is
going to be slower than reading it into a local variable once, then
reading the variable over and over.
Dave Griffith wrote:
One of the problems is that adding @Nullable to a field just produces a
huge swathe of yellow in your code. I produced a small hunk of test code
and discovered at least one bug (in the ordinary, unannotated case) - I
would be eager to hear the comments of this thread's participants.
http://www.jetbrains.net/jira/browse/IDEA-4126
R
Quite true, at least for non-final fields, and I agree that multiple reads should usually be replaced by cached local variables. There's actually an inspection for multiple accesses of a field in-product already, under the new "J2ME inspections" category.
--Dave Griffith
Dave Griffith wrote:
Is it really that slow? I was under the impression that the reason the
volatile keyword exists is because the JVM may locally cache the value
of a field, thus the volatile keyword would force the JVM to fetch the
value every time the field was referenced.
Ciao,
Gordon
--
Gordon Tyler (Software Developer)
Quest Software <http://www.quest.com/>
260 King Street East, Toronto, Ontario M5A 4L5, Canada
Voice: (416) 933-5046 | Fax: (416) 933-5001
Gordon Tyler wrote:
>> Quite true, at least for non-final fields, and I agree that multiple
>> reads should usually be replaced by cached local variables. There's
>> actually an inspection for multiple accesses of a field in-product
>> already, under the new "J2ME inspections" category.
I think performance is one reason not to do it. I don't know if it will
cache it. There are absolutely no semantics for it - it might cache, it
might not. That scares me, so I always use a local variable.
You'd have to profile your apps to be sure if it's worthwhile. The core java classes (particularly in the collections API) copy multiply-accessed fields to locals quite often, so at very least someone at Sun thinks it's worth doing.
In J2ME land, there's also the issue that field accesses result in larger class-size footprints than local-variable accesses. If you're shaving every byte off of a download, that sort of thing can add up.
--Dave Griffith
Dave Griffith wrote:
This may sound strange, but I wouldn't hold the JDK library up as a
paragon of code perfection. I've seen some stupid stuff going on in there.
Anyway, I'm not really disagreeing with you, it just seems unnecessary
to me, except perhaps for J2ME like you say. If I were looking to
increase the performance of my application, this would be one of the
last things I would do; algorithm optimizations being the first.
Ciao,
Gordon
--
Gordon Tyler (Software Developer)
Quest Software <http://www.quest.com/>
260 King Street East, Toronto, Ontario M5A 4L5, Canada
Voice: (416) 933-5046 | Fax: (416) 933-5001
Agreed, on all counts. That's why I moved this inspection from "Performance issues" to the considerably more specific "J2ME issues".
--Dave Griffith
I agree that it may be slower and produces smaller classfiles, but
that's not the point. As Gordon already said, optimizing usually starts
with optimizing algorithms. This peephole stuff is only done for very
performance ciritical parts.
I think the main problem is that it's not obvious. I just annotated some
code using @Nullable/@NotNull and IDEA shows some yellow spots which I
simply can't understand. Why? Because I am the coder and know that
indexOf() has no side-effects! I sat there and though what will be wrong
here and didn't get the point. That's why I started this thread.
Under the assumption that any method call may have side-effects (which
are not analysed by IDEA yet), the warnings are ok. But IDEA has to tell
me why! Otherwise many users may just assume it's an false positive.
Sven.
Keith Lea schrieb:
>> Sadly, that would result in too many false positives. There's plenty
>> of valid use cases for such constructs. For instance, you may know
>> that all instances of a class are used locally to one thread, and
>> thus no synchronization is necessary.
>>
>> I'm pretty sure that the data-flow inspections don't take
>> multi-threading into account.
>>
>> --Dave Griffith