|
Apache OpenOffice (AOO) Bugzilla – Full Text Issue Listing |
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
(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! (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. 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 Created attachment 82479 [details]
use O*String isEmpty() in modules starting with a
Created attachment 82480 [details]
use O*String isEmpty() in modules starting with b
"hdu" committed SVN revision 1564228 into trunk: #i123862# use O*String's isEmpty() method to check for emptiness in modules s... "hdu" committed SVN revision 1564230 into trunk: #i123862# use O*String's isEmpty() method to check for emptiness in modules s... Created attachment 82535 [details]
use O*String isEmpty() in module chart2
Created attachment 82536 [details]
use O*String isEmpty() in module codemaker
Created attachment 82537 [details]
use O*String isEmpty() in module comphelper
Created attachment 82538 [details]
use O*String isEmpty() in module configmgr
Created attachment 82539 [details]
use O*String isEmpty() in module connectivity
Created attachment 82540 [details]
use O*String isEmpty() in module cppu and cppuhelper
Created attachment 82541 [details]
use O*String isEmpty() in module cui
Created attachment 82542 [details]
use O*String isEmpty() in small changes in modules stating with b and c
Created attachment 82553 [details]
use O*String isEmpty() in module dbaccess
"hdu" committed SVN revision 1566533 into trunk: #i123862# use O*String's isEmpty() method to check for emptiness in chart2 mo... "hdu" committed SVN revision 1567593 into trunk: #i123862# use O*String's isEmpty() method to check for emptiness in the codem... "hdu" committed SVN revision 1567594 into trunk: #i123862# use O*String's isEmpty() method to check for emptiness in the comph... @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. (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. > 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... |
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.