273726 - Small cleanup for nsInstallVersion.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) (KHTML, like Gecko)
Build Identifier: 

this is my first patch/change which is really minor, more to get started. 
 
- It removes an unnecessary malloc()/free() implied by PR_sprintf_append(). 
- Doing this, the only two references to PR_ functions within the source are  
eliminated. 
- These includes are not longer needed: 
#include "prprf.h" 
#include "prmem.h" 
- due to my editor's settings, all trailing whitespaces where removed 
 
 

Reproducible: Always
Steps to Reproduce:
1.
2.
3.




diff -u -8 -p -d -r1.20 nsInstallVersion.cpp 
--- nsInstallVersion.cpp        17 Oct 2004 00:32:17 -0000      1.20 
+++ nsInstallVersion.cpp        8 Dec 2004 13:44:00 -0000 
@@ -41,35 +41,32 @@ 
 #include "nsInstallVersion.h" 
 #include "nsIDOMInstallVersion.h" 
 
 #include "nscore.h" 
 #include "nsIFactory.h" 
 #include "nsISupports.h" 
 #include "nsIScriptGlobalObject.h" 
 
-#include "prprf.h" 
-#include "prmem.h" 
- 
 static NS_DEFINE_IID(kISupportsIID, NS_ISUPPORTS_IID); 
 static NS_DEFINE_IID(kIScriptObjectOwnerIID, NS_ISCRIPTOBJECTOWNER_IID); 
 
 static NS_DEFINE_IID(kIInstallVersion_IID, NS_IDOMINSTALLVERSION_IID); 
 
 
 nsInstallVersion::nsInstallVersion() 
 { 
     mScriptObject   = nsnull; 
 } 
 
 nsInstallVersion::~nsInstallVersion() 
 { 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::QueryInterface(REFNSIID aIID,void** aInstancePtr) 
 { 
     if (aInstancePtr == NULL) 
     { 
         return NS_ERROR_NULL_POINTER; 
     } 
 
     // Always NULL result, in case of failure 
@@ -98,164 +95,164 @@ nsInstallVersion::QueryInterface(REFNSII 
 } 
 
 
 NS_IMPL_ADDREF(nsInstallVersion) 
 NS_IMPL_RELEASE(nsInstallVersion) 
 
 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::GetScriptObject(nsIScriptContext *aContext, void** 
aScriptObject) 
 { 
     NS_PRECONDITION(nsnull != aScriptObject, "null arg"); 
     nsresult res = NS_OK; 
- 
-    if (nsnull == mScriptObject) 
+ 
+    if (nsnull == mScriptObject) 
     { 
-        res = NS_NewScriptInstallVersion(aContext, 
-                                         (nsISupports *)
(nsIDOMInstallVersion*)this, 
-                                         nsnull, 
+        res = NS_NewScriptInstallVersion(aContext, 
+                                         (nsISupports *)
(nsIDOMInstallVersion*)this, 
+                                         nsnull, 
                                          &mScriptObject); 
     } 
- 
+ 
 
     *aScriptObject = mScriptObject; 
     return res; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::SetScriptObject(void *aScriptObject) 
 { 
   mScriptObject = aScriptObject; 
   return NS_OK; 
 } 
 
 //  this will go away when our constructors can have parameters. 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::Init(PRInt32 aMajor, PRInt32 aMinor, PRInt32 aRelease, 
PRInt32 aBuild) 
 { 
     major   = aMajor; 
     minor   = aMinor; 
     release = aRelease; 
     build   = aBuild; 
 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::Init(const nsString& version) 
 { 
     PRInt32 errorCode; 
     PRInt32 aMajor, aMinor, aRelease, aBuild; 
 
     major = minor = release = build = 0; 
- 
-    errorCode = nsInstallVersion::StringToVersionNumbers(version, &aMajor, 
&aMinor, &aRelease, &aBuild); 
-    if (NS_SUCCEEDED(errorCode)) 
+ 
+    errorCode = nsInstallVersion::StringToVersionNumbers(version, &aMajor, 
&aMinor, &aRelease, &aBuild); 
+    if (NS_SUCCEEDED(errorCode)) 
     { 
         Init(aMajor, aMinor, aRelease, aBuild); 
     } 
- 
+ 
     return NS_OK; 
 } 
 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::GetMajor(PRInt32* aMajor) 
 { 
     *aMajor = major; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::SetMajor(PRInt32 aMajor) 
 { 
     major = aMajor; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::GetMinor(PRInt32* aMinor) 
 { 
     *aMinor = minor; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::SetMinor(PRInt32 aMinor) 
 { 
     minor = aMinor; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::GetRelease(PRInt32* aRelease) 
 { 
     *aRelease = release; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::SetRelease(PRInt32 aRelease) 
 { 
     release = aRelease; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::GetBuild(PRInt32* aBuild) 
 { 
     *aBuild = build; 
     return NS_OK; 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::SetBuild(PRInt32 aBuild) 
 { 
     build = aBuild; 
     return NS_OK; 
 } 
 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::CompareTo(nsIDOMInstallVersion* aVersion, PRInt32* aReturn) 
 { 
     PRInt32 aMajor, aMinor, aRelease, aBuild; 
- 
+ 
     aVersion->GetMajor(&aMajor); 
     aVersion->GetMinor(&aMinor); 
     aVersion->GetRelease(&aRelease); 
     aVersion->GetBuild(&aBuild); 
 
     CompareTo(aMajor, aMinor, aRelease, aBuild, aReturn); 
- 
+ 
     return NS_OK; 
 } 
 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::CompareTo(const nsString& aString, PRInt32* aReturn) 
 { 
     nsInstallVersion inVersion; 
     inVersion.Init(aString); 
 
     return CompareTo(&inVersion, aReturn); 
 } 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::CompareTo(PRInt32 aMajor, PRInt32 aMinor, PRInt32 aRelease, 
PRInt32 aBuild, PRInt32* aReturn) 
 { 
     int diff; 
- 
-    if ( major == aMajor ) 
+ 
+    if ( major == aMajor ) 
     { 
-        if ( minor == aMinor ) 
+        if ( minor == aMinor ) 
         { 
-            if ( release == aRelease ) 
+            if ( release == aRelease ) 
             { 
                 if ( build == aBuild ) 
                     diff = EQUAL; 
                 else if ( build > aBuild ) 
                     diff = BLD_DIFF; 
                 else 
                     diff = BLD_DIFF_MINUS; 
             } 
@@ -275,82 +272,80 @@ nsInstallVersion::CompareTo(PRInt32 aMaj 
         diff = MAJOR_DIFF_MINUS; 
 
     *aReturn = diff; 
 
     return NS_OK; 
 } 
 
 
-NS_IMETHODIMP 
+NS_IMETHODIMP 
 nsInstallVersion::ToString(nsString& aReturn) 
 { 
-    char *result=NULL; 
-    result = PR_sprintf_append(result, "%d.%d.%d.%d", major, minor, release, 
build); 
- 
-    aReturn.AssignWithConversion(result); 
+    char buf[64]; 
 
-    PR_FREEIF(result); 
+    snprintf(buf, sizeof(buf), "%d.%d.%d.%d", major, minor, release, build); 
+    aReturn.AssignWithConversion(buf); 
 
     return NS_OK; 
 } 
 
 
 nsresult 
-nsInstallVersion::StringToVersionNumbers(const nsString& version, PRInt32 
*aMajor, PRInt32 *aMinor, PRInt32 *aRelease, PRInt32 *aBuild) 
+nsInstallVersion::StringToVersionNumbers(const nsString& version, PRInt32 
*aMajor, PRInt32 *aMinor, PRInt32 *aRelease, PRInt32 *aBuild) 
 { 
     PRInt32 errorCode = nsInstall::UNEXPECTED_ERROR; 
 
     if (!aMajor || !aMinor || !aRelease || !aBuild) 
         return nsInstall::INVALID_ARGUMENTS; 
 
     *aMajor = *aMinor = *aRelease = *aBuild = 0; 
 
     int dot = version.FindChar('.', 0); 
 
-    if ( dot == -1 ) 
+    if ( dot == -1 ) 
     { 
         *aMajor = version.ToInteger(&errorCode); 
     } 
-    else 
+    else 
     { 
         nsString majorStr; 
         version.Mid(majorStr, 0, dot); 
         *aMajor  = majorStr.ToInteger(&errorCode); 
 
         int prev = dot+1; 
         dot = version.FindChar('.',prev); 
-        if ( dot == -1 ) 
+        if ( dot == -1 ) 
         { 
             nsString minorStr; 
             version.Mid(minorStr, prev, version.Length() - prev); 
             *aMinor = minorStr.ToInteger(&errorCode); 
         } 
-        else 
+        else 
         { 
             nsString minorStr; 
             version.Mid(minorStr, prev, dot - prev); 
             *aMinor = minorStr.ToInteger(&errorCode); 
 
             prev = dot+1; 
             dot = version.FindChar('.',prev); 
-            if ( dot == -1 ) 
+            if ( dot == -1 ) 
             { 
                 nsString releaseStr; 
                 version.Mid(releaseStr, prev, version.Length() - prev); 
                 *aRelease = releaseStr.ToInteger(&errorCode); 
             } 
-            else 
+            else 
             { 
                 nsString releaseStr; 
                 version.Mid(releaseStr, prev, dot - prev); 
                 *aRelease = releaseStr.ToInteger(&errorCode); 
- 
+ 
                 prev = dot+1; 
-                if ( (int)version.Length() > dot ) 
+                if ( (int)version.Length() > dot ) 
                 { 
                     nsString buildStr; 
                     version.Mid(buildStr, prev, version.Length() - prev); 
                     *aBuild = buildStr.ToInteger(&errorCode); 
                } 
             } 
         } 
     }

Comment 1

[Tim Ruehsen]

Attachment: cvs diff of nsInstallVersion.cpp

Comment 2

[Benjamin Smedberg]

Comment on attachment 168214 [details] [diff] [review]
cvs diff of nsInstallVersion.cpp

Since you're a new moz hacker, I'm going to be ultra-nitpicky; don't take this
the wrong way, I just want to get you used to the coding style:

>Index: nsInstallVersion.cpp

> nsInstallVersion::GetScriptObject(nsIScriptContext *aContext, void** aScriptObject)

>-    
>-    if (nsnull == mScriptObject) 
>+
>+    if (nsnull == mScriptObject)

If you're going to be touching these lines, please go ahead and fix the check
also:

if (!mScriptObject)

> nsInstallVersion::ToString(nsString& aReturn)
> {
>-    char *result=NULL;
>-    result = PR_sprintf_append(result, "%d.%d.%d.%d", major, minor, release, build);
>-    
>-    aReturn.AssignWithConversion(result);
>+    char buf[64];
> 
>-    PR_FREEIF(result);
>+    snprintf(buf, sizeof(buf), "%d.%d.%d.%d", major, minor, release, build);
>+    aReturn.AssignWithConversion(buf);

Use PR_snprintf instead of the stdlib version. We do this because the NSPR
version matches "%d" with PRInt32, while the stdlib version uses the compiler
"int" which might not be 32 bits.

Thank you for fixing up the whitespace. Beware though, mozilla code is "you
bought it, you own it": you are going to own CVS blame for these lines ;)

Comment 3

[Tim Ruehsen]

In the source we have pointer comparisons with NULL and with nsnull. Which is 
the recommended one? NULL would be better readable (since 99% of the developers  
are used to it) and more standards compliant. Tell me how the community handles 
this (or what is the general moz practice in this case). 
 

Comment 4

[Benjamin Smedberg]

If you're null-checking a pointer, just use "if (pointer)", don't use NULL or
nsnull. Otherwise, use NULL in NSPR and some parts of the stub installer code,
and nsnull in general mozilla code.

Comment 5

[Tim Ruehsen]

Attachment: cvs diff

- use PR_snprintf instead of snprintf
- changed expression 'nsnull==mScriptObject' to '!ScriptObject'
- removed function StringToVersionNumbers(), completely replaced it by
PR_sscanf() and put it into the Init() function
- goes together with the second patch for nsInstallVersion.h

Comment 6

[Tim Ruehsen]

Attachment: cvs diff

- goes together with nsInstallVersion.cpp patch

Comment 7

[Benjamin Smedberg]

Comment on attachment 168305 [details] [diff] [review]
cvs diff

In the future, you can (and should) make a single patch with all the changed
files:

cvs diff -u8pdN file1 file2 directory3

Comment 8

[Boris Zbarsky [:bzbarsky]]

So is this ready to check in?  If so, which exact attachments need to be checked in?

bsmedberg, does this code need sr?

Comment 9

[Tim Ruehsen]

both 'active' attachments should be checked in, that is 
the second (2004-12-09 07:04 PST, 7.96 KB) and the third (2004-12-09 07:05 PST, 
1.09 KB). 

Comment 10

[Benjamin Smedberg]

This code does need sr.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 168305 [details] [diff] [review]
cvs diff

+    aReturn.AssignWithConversion(buf);

this is deprecated, use AssignASCII.

Comment 12

[Tim Ruehsen]

Attachment: contains cvs diff for nsInstallVersion.cpp and nsInstallVersion.h

- Using AssignASCII()
- one patchfile for .cpp and .h (2 patches seemed to confuse people)
- nsInstallVersion: removed an unused variable 'errorCode' at line 147

Comment 13

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 171144 [details] [diff] [review]
contains cvs diff for nsInstallVersion.cpp and nsInstallVersion.h

> nsInstallVersion::Init(const nsString& version)
> {
>-    PRInt32 errorCode;
>     PRInt32 aMajor, aMinor, aRelease, aBuild;
> 
>     major = minor = release = build = 0;
>-    
>-    errorCode = nsInstallVersion::StringToVersionNumbers(version, &aMajor, &aMinor, &aRelease, &aBuild);    
>-    if (NS_SUCCEEDED(errorCode)) 
>-    {
>-        Init(aMajor, aMinor, aRelease, aBuild);
>-    }
>-    
>-    return NS_OK;
>+
>+    // nsString is a flat string
>+    if (PR_sscanf(NS_ConvertUTF16toUTF8(version).get(),"%d.%d.%d.%d",&aMajor,&aMinor,&aRelease,&aBuild) < 1)
>+        return NS_ERROR_UNEXPECTED;
>+
>+    return Init(aMajor, aMinor, aRelease, aBuild);

If you get a short version ("1.3") this will call Init() with uninitialized
values, overwriting the release and build values initialized at the top of the
routine. StringToVersionNumbers() made sure unset values were 0. (It's
confusing nomenclature: major, minor etc should have been mMajor,mMinor or
m_major,m_minor etc. -- want to fix that up too? ;-))

If you switch to sscanf you need to initialize the returned values.

Is there any reason it wouldn't be safe to PR_sscanf() directly into the member
values? Then you could dispense with the call to the overloaded form of Init().

StringToVersionNumbers() also supported versions like 1.2a.3.4 (ignoring the
internal text), which will now be interpreted as plain 1.2.  It wasn't an
advertised feature, so long as things like "1.2a" with only trailing text
decoration work it's probably OK.

Comment 14

[Tim Ruehsen]

Attachment: updated patch for nsInstallVersion.h and nsInstallVersion.cpp

- fixed the uninitialized values (really bad one from me, argh).
- moved major,minor etc. to mMajor, mMinor etc.

Version numbering:
The ToString()/CompareTo() subroutines never worked with non-digits. Whenever
we initialize nsInstallVersion with e.g. 1.8a6, the ToString routine comes back
with '1.8'.
Due to this I would not encourage to use the kind of version numbering.
If we want to use it consistently, we have to change mMajor etc. from Integers
to Strings. And we have to agree upon a definition of comparing these version
strings (e.g. maybe just use strcmp or strcasecmp?).

Other projects (e.g. I think KDE) would use 1.7.99.x to mark the 1.8
alpha/beta.

Comment 15

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 171983 [details] [diff] [review]
updated patch for nsInstallVersion.h and nsInstallVersion.cpp

sr=dveditz
Are you going to need someone to check this in?

Comment 16

[Tim Ruehsen]

Yes, please. I don't have write access. 
 

Comment 17

[Boris Zbarsky [:bzbarsky]]

Patch checked in.