You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pivot.apache.org by Sandro Martini <sa...@gmail.com> on 2009/08/03 11:13:41 UTC
patch for TabPane
Hi to all,
another small fix, but in this case we must decide what value has to
be set in the case of null:
is it safer a true, or false (then remove my comment) ?
Index: src/org/apache/pivot/wtk/TabPane.java
===================================================================
--- src/org/apache/pivot/wtk/TabPane.java (revision 800243)
+++ src/org/apache/pivot/wtk/TabPane.java (working copy)
@@ -380,7 +380,7 @@
public static boolean isCloseable(Component component) {
Attributes attributes = (Attributes)component.getAttributes();
- return (attributes == null) ? null : attributes.closeable;
+ return (attributes == null) ? true : attributes.closeable;
// verify what value to set in case of null ...
}
public static void setCloseable(Component component, boolean closeable) {
And another thing, already discussed with Greg some time ago, but I'd
like to have some infos also from others (and this is the last time i
write on this, don't worry):
across sources, i have many bugs warnings from Findbugs on "Comparison
of String parameter using == or !=" instead of equals(), like this in
TabPane.java
String previousLabel = attributes.label;
if (previousLabel != label) {
attributes.label = label;
Ok, it's not a problem to keep this, but it seems to be not always
predictable, as from the Bug description:
This code compares a java.lang.String parameter for reference equality
using the == or != operators. Requiring callers to pass only String
constants or interned strings to a method is unnecessarily fragile,
and rarely leads to measurable performance gains. Consider using the
equals(Object) method instead.
Greg said me that this is Ok because we don't need a full String
comparison with equals() , but so why not remove the statements
if (previousLabel != label)
and always assign the new value ?
What do you think ?
Bye,
Sandro
Re: patch for TabPane
Posted by Todd Volkert <tv...@gmail.com>.
"safer" is a misnomer here, because there's no harm in running the setter.
If we define the change event to mean that the reference changed, then this
is the right check. We don't use logical equals() in our comparison of
other component model objects (like list data), so a reference equality
check is more consistent with checks like "if previousListData != listData".
-T
On Mon, Aug 3, 2009 at 6:57 AM, Sandro Martini <sa...@gmail.com>wrote:
> HI Niclas,
> good point :-) i agree with you, I'm for safer tests, or to avoid them
> and assign always the new value ...
>
> And what other say ??
>
>
> Than, what default value should we set in the patch for the
> isCloseable of TabPane (and there are others like this in sources, but
> i can get more details on others) ?
>
> Thanks,
> Sandro
>
Re: patch for TabPane
Posted by Sandro Martini <sa...@gmail.com>.
HI Niclas,
good point :-) i agree with you, I'm for safer tests, or to avoid them
and assign always the new value ...
And what other say ??
Than, what default value should we set in the patch for the
isCloseable of TabPane (and there are others like this in sources, but
i can get more details on others) ?
Thanks,
Sandro
Re: patch for TabPane
Posted by Sandro Martini <sa...@gmail.com>.
Hi Christopher,
thanks for your help ...
Ok so for String comparison, I'll disable that Findbugs filter :-) , no problem.
For the patch to TabPane, I've just seen the default value in the
inner class is this:
private static class Attributes {
public boolean closeable = false;
and so I've just changed the patch with a value of false in case of null.
If this is ok i could apply the new patch.
Tell me what to do.
Bye,
Sandro
Re: patch for TabPane
Posted by Christopher Brind <br...@brindy.org.uk>.
Hi,
This is the simplest way I can think of to describe scenario:
We're doing an object reference comparison. It just happens the objects are
> Strings.
>
... so that means it is a false positive. :)
Cheers,
Chris
2009/8/3 Sandro Martini <sa...@gmail.com>
> Hi, sorry for my English ...
>
> "safer" for me in this context means "right" in the sense of right
> full-string comparison :-) to ensure that the string has changed ...
>
> So to archive this question, how should we do:
> filter these bugs as false positive, or make other tests ?
>
>
> And finally, for the patch on TabPane have i to do the Commit, but
> keeping the true value for the isCloseable() in case of null ?
>
> Thanks,
> Sandro
>
Re: patch for TabPane
Posted by Sandro Martini <sa...@gmail.com>.
Hi, sorry for my English ...
"safer" for me in this context means "right" in the sense of right
full-string comparison :-) to ensure that the string has changed ...
So to archive this question, how should we do:
filter these bugs as false positive, or make other tests ?
And finally, for the patch on TabPane have i to do the Commit, but
keeping the true value for the isCloseable() in case of null ?
Thanks,
Sandro
Re: patch for TabPane
Posted by Todd Volkert <tv...@gmail.com>.
> String abc = "Abc";
> String def = "A" + "b" + "c";
> String rst = create( "A" ) + create( "b" ) + create( "c" );
> String xyz = new StringBuffer().append( 'A' ).append( 'b' ).append(
> 'c' ).toString();
>
> boolean r1 = abc == def;
> boolean r2 = abc == rst;
> boolean r3 = abc == xyz;
>
> private String create( String in )
> {
> return new String( in );
> }
>
>
> Can you tell me which one of those r1-r4 that are true?? (I can't, and
> I suspect it is compiler/jre dependent, but I think the spec requires
> only abc==def to be true, and the others are (jit-)compiler smartness
> only).
The answer depends on which ones are interned, which like you say is
guaranteed to be at least abc and def. I strongly suspect the others will
*not* get interned and will reference difference objects, but perhaps that
is JVM-dependent.
We only employ this "shortcut" when deciding whether or not to run certain
setters in components. Running the setter isn't harmful if two
logically-equal strings appear to be different because of a reference
equality check -- it's just unnecessary overhead. So basically, it's a
judgement call as to which overhead we want to incur: the string comparison
or the body of the setter. The thought was that the vast majority of the
time a caller calls that setter, they'll be setting a distinct value, so
chances are we'll run the body of the setter anyway, so we save the string
comparison. That being said, I do think it'd be incorrect assume that
that's true 100% of the time and to not have any check - otherwise, the
event becomes semantically incorrect.
-T
Re: patch for TabPane
Posted by Niclas Hedhman <ni...@hedhman.org>.
On Mon, Aug 3, 2009 at 6:22 PM, Todd Volkert<tv...@gmail.com> wrote:
> We use reference equality comparison
> because we don't want to incur the string comparison overhead, but we do
> need to perform some comparison.
You do realize the instance comparison difference?
String abc = "Abc";
String def = "A" + "b" + "c";
String rst = create( "A" ) + create( "b" ) + create( "c" );
String xyz = new StringBuffer().append( 'A' ).append( 'b' ).append(
'c' ).toString();
boolean r1 = abc == def;
boolean r2 = abc == rst;
boolean r3 = abc == xyz;
private String create( String in )
{
return new String( in );
}
Can you tell me which one of those r1-r4 that are true?? (I can't, and
I suspect it is compiler/jre dependent, but I think the spec requires
only abc==def to be true, and the others are (jit-)compiler smartness
only).
I hope you don't pull too many stunts/assumptions and do "programming
by coincidence"(tm) ;-).
Cheers
--
Niclas Hedhman, Software Developer
http://www.qi4j.org - New Energy for Java
I live here; http://tinyurl.com/2qq9er
I work here; http://tinyurl.com/2ymelc
I relax here; http://tinyurl.com/2cgsug
Re: patch for TabPane
Posted by Todd Volkert <tv...@gmail.com>.
> Greg said me that this is Ok because we don't need a full String
> comparison with equals() , but so why not remove the statements
> if (previousLabel != label)
> and always assign the new value ?
Because we fire events when the value changes, which would be incorrect if
the value did in fact not change. We use reference equality comparison
because we don't want to incur the string comparison overhead, but we do
need to perform some comparison.
-T
Re: patch for TabPane
Posted by Sandro Martini <sa...@gmail.com>.
Ok, I'll make immediately the patch, before to forget ;-) ...
Thanks again.
Re: patch for TabPane
Posted by Greg Brown <gk...@mac.com>.
False is fine.
On Aug 3, 2009, at 8:57 AM, Sandro Martini wrote:
> Hi Greg,
> sorry, I've just replaced the file from That in trunk but TabPane.java
> is without the patch, can i try to do it now (but this time using
> false as a default value in case of null) ?
>
> Tell me.
>
> Thanks,
> Sandro
Re: patch for TabPane
Posted by Sandro Martini <sa...@gmail.com>.
Hi Greg,
sorry, I've just replaced the file from That in trunk but TabPane.java
is without the patch, can i try to do it now (but this time using
false as a default value in case of null) ?
Tell me.
Thanks,
Sandro
Re: patch for TabPane
Posted by Greg Brown <gk...@mac.com>.
OK, I see that this has already been checked in - thanks.
On Aug 3, 2009, at 8:30 AM, Greg Brown wrote:
>> Index: src/org/apache/pivot/wtk/TabPane.java
>> ===================================================================
>> --- src/org/apache/pivot/wtk/TabPane.java (revision 800243)
>> +++ src/org/apache/pivot/wtk/TabPane.java (working copy)
>> @@ -380,7 +380,7 @@
>>
>> public static boolean isCloseable(Component component) {
>> Attributes attributes = (Attributes)component.getAttributes();
>> - return (attributes == null) ? null : attributes.closeable;
>> + return (attributes == null) ? true : attributes.closeable;
>> // verify what value to set in case of null ...
>> }
>
> I have applied this patch. But how did the version that returns null
> even compile? I assume it has something to do with auto-boxing and
> would fail at runtime, but that's weird.
>
>
Re: patch for TabPane
Posted by Greg Brown <gk...@mac.com>.
> Index: src/org/apache/pivot/wtk/TabPane.java
> ===================================================================
> --- src/org/apache/pivot/wtk/TabPane.java (revision 800243)
> +++ src/org/apache/pivot/wtk/TabPane.java (working copy)
> @@ -380,7 +380,7 @@
>
> public static boolean isCloseable(Component component) {
> Attributes attributes = (Attributes)component.getAttributes();
> - return (attributes == null) ? null : attributes.closeable;
> + return (attributes == null) ? true : attributes.closeable;
> // verify what value to set in case of null ...
> }
I have applied this patch. But how did the version that returns null
even compile? I assume it has something to do with auto-boxing and
would fail at runtime, but that's weird.