You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openoffice.apache.org by bu...@apache.org on 2013/12/16 13:40:42 UTC

[Bug 123862] New: O*String's isEmpty() should be used instead of its older alternatives

https://issues.apache.org/ooo/show_bug.cgi?id=123862

            Bug ID: 123862
        Issue Type: ENHANCEMENT
           Summary: O*String's isEmpty() should be used instead of its
                    older alternatives
           Product: General
           Version: 4.1.0-dev
          Hardware: All
                OS: All
            Status: CONFIRMED
          Severity: normal
          Priority: P4
         Component: code
          Assignee: issues@openoffice.apache.org
          Reporter: hdu@apache.org
                CC: issues@openoffice.apache.org

In our current codebase there are all kinds of variants for checking whether a
string is empty:
    aString == OUString()
    aString.equalsAscii("")
    aString.equals( OUString())
    aString.compareTo( OUString(""))
    aString.compareToAscii( "")
    aString.getLength() == 0
    aString.getLength() < 1
and the related checks to check whether it is not empty:
    aString.getLength() > 0
    aString.getLength() >= 1

The O*String method isEmpty() allows cleaner code and is as fast or faster than
all its alternatives, so this method should be used.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

hdu@apache.org <hd...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82480|review?(hdu@apache.org)     |review+
              Flags|                            |

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

j.nitschke@ok.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82479|                            |review?(hdu@apache.org)
              Flags|                            |
                 CC|                            |hdu@apache.org

--- Comment #5 from j.nitschke@ok.de ---
Created attachment 82479
  --> https://issues.apache.org/ooo/attachment.cgi?id=82479&action=edit
use O*String isEmpty() in modules starting with a

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #23 from hdu@apache.org <hd...@apache.org> ---
> btw. is there a standard encoding for the codebase?

I personally prefer UTF-8, but as you saw many files are in iso8859-1. And some
people don't use UTF-8 enabled dev environments. It s***s but that's how it is.
Both encodings overlap in the ASCII space, so if in doubt please transcribe to
ASCII.

> I read the patches anyway so it's not a big deal to remove these changes but
> if you got a solution for this it's welcome.

Speaking of patches and their review: I suggest to provide the script that you
developed to let it do the bulk of the work. And then we'll do fixups where
needed as patches. This makes the review much easier...

Just for the laughs: a colleague just found a new variant of an isEmpty()
predecessor: main/svx/source/sidebar/possize/PosSizePropertyPanel.cxx line
1203...

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #14 from j.nitschke@ok.de ---
Created attachment 82540
  --> https://issues.apache.org/ooo/attachment.cgi?id=82540&action=edit
use O*String isEmpty() in module cppu and cppuhelper

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #4 from j.nitschke@ok.de ---
almost missed the very common implicit casts
in conditions:
if (a.getLength())
a.getLength() ? b : c
while (a.getLength())

and via logical operators !,|| and &&:
a.getLength() && b
a.getLength() || b
!a.getLength()

explicit casts:
(bool)a.getLength()
(sal_Bool)...
(??_Bool)...

it might be easier to generate an expression which excludes all legitimate
usages of getLength(), instead of including all variants

but priority is to build expressions which can't break the logic

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #22 from j.nitschke@ok.de ---
(In reply to hdu@apache.org from comment #21)
> @j.nitschke: All patches are good so far, its a wonderful cleanup. Thanks
> for working on it!
Welcome

> But with AOO 4.1 beta coming soon I'd like to wait with further integrations
> until the release branch for AOO 4.1 has been split off.
Yes, that's only reasonable. I try to work with great caution, but with the
number of changes, errors get more likely.

> Looking at these masses of emptiness-checks I thought that having another
> method isNotEmpty() would improve the readability even more.
My current regexp finds about 7500 potential replace candidates. I expect 1/5
false positives, leaves 6000 replacements.
If half of these are checks for not empty, an other method for 3000 calls is a
good idea.


btw. is there a standard encoding for the codebase? I use eclipse with standard
setting UTF-8 and sometimes the german comments get changed. It seems at least
some files use ISO-8859-x encoding. I did a standard checkout and diff with svn
command line tool on linux.
I read the patches anyway so it's not a big deal to remove these changes but if
you got a solution for this it's welcome.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #20 from SVN Robot <sv...@dev.null.org> ---
"hdu" committed SVN revision 1567594 into trunk:
#i123862# use O*String's isEmpty() method to check for emptiness in the
comph...

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

hdu@apache.org <hd...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82479|review?(hdu@apache.org)     |review+
              Flags|                            |

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #2 from hdu@apache.org <hd...@apache.org> ---
(In reply to j.nitschke from comment #1)
> Hi, I would like to work on this simple task to get an overview of the
> various modules.

Thanks for working on this! Your approach and e.g. the regular expression looks
fine. You know what you're doing ;-)

With the many locations and the variations to check for emptiness before the
obviously missing method became available an almost automatic conversion with
many manual checks is the only reasonable way to go.

> How would you like negations formated?
> a) !a.isEmpty()
> b) ! a.isEmpty()
> c) ( !a.isEmpty() )
> d) ?

I personnaly prefer a.

> I planned to make one patch file per module, maybe bundle small modules in
> one

Sounds good. Have fun!

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #10 from j.nitschke@ok.de ---
Created attachment 82536
  --> https://issues.apache.org/ooo/attachment.cgi?id=82536&action=edit
use O*String isEmpty() in module codemaker

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

review requested: [Bug 123862] O*String's isEmpty() should be used instead of its older alternatives : [Attachment 82480] use O*String isEmpty() in modules starting with b

Posted by bu...@apache.org.
j.nitschke@ok.de has asked hdu@apache.org <hd...@apache.org> for review:
Bug 123862: O*String's isEmpty() should be used instead of its older
alternatives
https://issues.apache.org/ooo/show_bug.cgi?id=123862

Attachment 82480: use O*String isEmpty() in modules starting with b
https://issues.apache.org/ooo/attachment.cgi?id=82480&action=edit

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #13 from j.nitschke@ok.de ---
Created attachment 82539
  --> https://issues.apache.org/ooo/attachment.cgi?id=82539&action=edit
use O*String isEmpty() in module connectivity

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #9 from j.nitschke@ok.de ---
Created attachment 82535
  --> https://issues.apache.org/ooo/attachment.cgi?id=82535&action=edit
use O*String isEmpty() in module chart2

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

review granted: [Bug 123862] O*String's isEmpty() should be used instead of its older alternatives : [Attachment 82480] use O*String isEmpty() in modules starting with b

Posted by bu...@apache.org.
hdu@apache.org <hd...@apache.org> has granted j.nitschke@ok.de's request for
review:
Bug 123862: O*String's isEmpty() should be used instead of its older
alternatives
https://issues.apache.org/ooo/show_bug.cgi?id=123862

Attachment 82480: use O*String isEmpty() in modules starting with b
https://issues.apache.org/ooo/attachment.cgi?id=82480&action=edit

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

Oliver-Rainer Wittmann <or...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |orw@apache.org
   Target Milestone|---                         |4.1.0

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #18 from SVN Robot <sv...@dev.null.org> ---
"hdu" committed SVN revision 1566533 into trunk:
#i123862# use O*String's isEmpty() method to check for emptiness in chart2
mo...

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #19 from SVN Robot <sv...@dev.null.org> ---
"hdu" committed SVN revision 1567593 into trunk:
#i123862# use O*String's isEmpty() method to check for emptiness in the
codem...

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #11 from j.nitschke@ok.de ---
Created attachment 82537
  --> https://issues.apache.org/ooo/attachment.cgi?id=82537&action=edit
use O*String isEmpty() in module comphelper

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #16 from j.nitschke@ok.de ---
Created attachment 82542
  --> https://issues.apache.org/ooo/attachment.cgi?id=82542&action=edit
use O*String isEmpty() in small changes in modules stating with b and c

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #12 from j.nitschke@ok.de ---
Created attachment 82538
  --> https://issues.apache.org/ooo/attachment.cgi?id=82538&action=edit
use O*String isEmpty() in module configmgr

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #8 from SVN Robot <sv...@dev.null.org> ---
"hdu" committed SVN revision 1564230 into trunk:
#i123862# use O*String's isEmpty() method to check for emptiness in modules
s...

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

review requested: [Bug 123862] O*String's isEmpty() should be used instead of its older alternatives : [Attachment 82479] use O*String isEmpty() in modules starting with a

Posted by bu...@apache.org.
j.nitschke@ok.de has asked hdu@apache.org <hd...@apache.org> for review:
Bug 123862: O*String's isEmpty() should be used instead of its older
alternatives
https://issues.apache.org/ooo/show_bug.cgi?id=123862

Attachment 82479: use O*String isEmpty() in modules starting with a
https://issues.apache.org/ooo/attachment.cgi?id=82479&action=edit

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

j.nitschke@ok.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82480|                            |review?(hdu@apache.org)
              Flags|                            |

--- Comment #6 from j.nitschke@ok.de ---
Created attachment 82480
  --> https://issues.apache.org/ooo/attachment.cgi?id=82480&action=edit
use O*String isEmpty() in modules starting with b

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

j.nitschke@ok.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |j.nitschke@ok.de

--- Comment #1 from j.nitschke@ok.de ---
Hi, I would like to work on this simple task to get an overview of the various
modules.

I plan to use eclipse cdt
search the variants with regexp
check the type in the ide and replace with regexp groups.

first analysis:
regexp for first 5 variants
"(\s*(!|=)=\s*OU?String\s*\(\s*\)|\.(equals(Ascii)?|compareTo(Ascii)?)\s*\(\s*(""|OU?String\s*\((\s*""\s*)?\))?\s*\))"
72 occurrences

last 4 variants:
"(\s*|\.)getLength\s*\(\s*\)\s*(>\s*0|>=\s*1|==\s*0|!=\s*0|<\s*1)"
>0, >=1, !=0
<1, <=0, ==0
3432 occurrences
* findings include build generated files

common types with getLenght() method
most are O*String
Sequence <T> ( has similar method hasElements() )
O*StringBuffer


in most cases it would be a simple replacement
some cases need fix for brackets
if ( a && (b.getLenght() == 0) ) -> if ( a && b.isEmpty() )
few cases where neatly indentations need fixes

How would you like negations formated?
a) !a.isEmpty()
b) ! a.isEmpty()
c) ( !a.isEmpty() )
d) ?

I planned to make one patch file per module, maybe bundle small modules in one

please comment

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #3 from hdu@apache.org <hd...@apache.org> ---
(In reply to hdu@apache.org from comment #2)
> (In reply to j.nitschke from comment #1)
> > How would you like negations formated?
> > a) !a.isEmpty()
> > b) ! a.isEmpty()
> > c) ( !a.isEmpty() )
> > d) ?
> 
> I personnaly prefer a.

To add to this: if the 'a' means something lengthy like
   getFoo()->getBar()->getMore()->getDetails()->getName()
then I actually prefer something like
   a.isEmpty() == false
to keep the logic close together so it is easier to read. Not using the
super-long and hardly debugable stuff would be even better, but for this
half-automatic fix we shouldn't change such innards.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

hdu@apache.org <hd...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CONFIRMED                   |ACCEPTED
   Target Milestone|4.1.0                       |AOO Later

--- Comment #21 from hdu@apache.org <hd...@apache.org> ---
@j.nitschke: All patches are good so far, its a wonderful cleanup. Thanks for
working on it!

But with AOO 4.1 beta coming soon I'd like to wait with further integrations
until the release branch for AOO 4.1 has been split off.

Looking at these masses of emptiness-checks I thought that having another
method isNotEmpty() would improve the readability even more.

In the commits so far I added Patch-by attributions as good as possible with
SVN. If you want your full name for the Patch-by please provide it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

hdu@apache.org <hd...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Latest|---                         |4.0.1
    Confirmation on|                            |
          Developer|---                         |simple
         Difficulty|                            |

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #17 from j.nitschke@ok.de ---
Created attachment 82553
  --> https://issues.apache.org/ooo/attachment.cgi?id=82553&action=edit
use O*String isEmpty() in module dbaccess

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

review granted: [Bug 123862] O*String's isEmpty() should be used instead of its older alternatives : [Attachment 82479] use O*String isEmpty() in modules starting with a

Posted by bu...@apache.org.
hdu@apache.org <hd...@apache.org> has granted j.nitschke@ok.de's request for
review:
Bug 123862: O*String's isEmpty() should be used instead of its older
alternatives
https://issues.apache.org/ooo/show_bug.cgi?id=123862

Attachment 82479: use O*String isEmpty() in modules starting with a
https://issues.apache.org/ooo/attachment.cgi?id=82479&action=edit

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #7 from SVN Robot <sv...@dev.null.org> ---
"hdu" committed SVN revision 1564228 into trunk:
#i123862# use O*String's isEmpty() method to check for emptiness in modules
s...

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.

[Bug 123862] O*String's isEmpty() should be used instead of its older alternatives

Posted by bu...@apache.org.
https://issues.apache.org/ooo/show_bug.cgi?id=123862

--- Comment #15 from j.nitschke@ok.de ---
Created attachment 82541
  --> https://issues.apache.org/ooo/attachment.cgi?id=82541&action=edit
use O*String isEmpty() in module cui

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.
You are watching all bug changes.