129
edits
Thunderbird:CodeCleanup: Difference between revisions
m
→Strings: end the sentence:)
m (→Strings: end the sentence:)) |
|||
| (13 intermediate revisions by 4 users not shown) | |||
| Line 16: | Line 16: | ||
== IDL Changes == | == IDL Changes == | ||
Many of the mailnews interfaces | Many of the mailnews interfaces use string and wstring for string arguments. In many cases, we can convert these to AString and ACString. This change allows C++ callers to more efficiently manage the strings using nsString/nsCStrings. However it doesn't always make sense to do so. For instance in nsIMsgIdentity.idl, I found some string arguments that were passed directly to the pref service (which requires a char *), and all the call sites were passing in string literals. Changing that argument to ACString wouldn't have made sense. But in general, most things can be converted. | ||
<span class="highlightred">- attribute wstring key;</span> | <span class="highlightred">- attribute wstring key;</span> | ||
| Line 33: | Line 33: | ||
<span class="highlightblue">+ StringBeginsWith(someString, NS_LITERAL_CSTRING("imap://");</span> | <span class="highlightblue">+ StringBeginsWith(someString, NS_LITERAL_CSTRING("imap://");</span> | ||
The following applies to both nsXPIDLString and nsXPIDLCString. This class is going away when we use frozen linkage so we want to replace it with If you are changing an interface such that somemethod now takes a nsAString / nsACString then you don't need the getter_Copies either: | The following applies to both nsXPIDLString and nsXPIDLCString. This class is going away when we use frozen linkage so we want to replace it with nsString and nsCString. If you are changing an interface such that somemethod now takes a nsAString / nsACString then you don't need the getter_Copies either: | ||
<span class="highlightred">- nsXPIDLString strValue; | <span class="highlightred">- nsXPIDLString strValue; | ||
| Line 43: | Line 43: | ||
nsCString charValue; | nsCString charValue; | ||
<span class="highlightred">- uniValue.Assign(NS_ConvertASCIItoUTF16(charValue));</span> | <span class="highlightred">- uniValue.Assign(NS_ConvertASCIItoUTF16(charValue));</span> | ||
<span class="highlightblue">+ CopyASCIItoUTF16( | <span class="highlightblue">+ CopyASCIItoUTF16(charValue, uniValue);</span> | ||
nsCString charValue; | nsCString charValue; | ||
| Line 49: | Line 49: | ||
<span class="highlightred">- charValue.AssignWithConversion(uniValue);</span> | <span class="highlightred">- charValue.AssignWithConversion(uniValue);</span> | ||
<span class="highlightblue">+ LossyCopyUTF16toASCII(uniValue, charValue);</span> | <span class="highlightblue">+ LossyCopyUTF16toASCII(uniValue, charValue);</span> | ||
The append methods like AppendUTF8toUTF16 are not available in the frozen linkage world: | |||
nsCString charValue; | |||
nsString uniValue; | |||
<span class="highlightred">- AppendUTF8toUTF16(charVAlue, uniValue);</span> | |||
<span class="highlightblue">+ uniValue.Append(NS_ConvertUTF8toUTF16(charValue));</span> | |||
nsCString charStr; | nsCString charStr; | ||
| Line 61: | Line 68: | ||
<span class="highlightred">- nsCRT::strcmp(charStr.get(), charStr2.get()) == 0</span> | <span class="highlightred">- nsCRT::strcmp(charStr.get(), charStr2.get()) == 0</span> | ||
<span class="highlightblue">+ charStr.Equals(charStr2)</span> | <span class="highlightblue">+ charStr.Equals(charStr2)</span> | ||
nsCString charStr; | |||
<span class="highlightred">- if (nsCRT::strncmp("imap:", charStr, 5))</span> | |||
<span class="highlightblue">+ if (!StringBeginsWith(charStr, NS_LITERAL_CSTRING("imap:")))</span> | |||
Replacement of case-insensitive checking code: | |||
<span class="highlightred">- char *strA; | |||
- char *strB; | |||
- if (PL_strncasaecmp(strA, strB)) { | |||
... | |||
- }</span> | |||
<span class="highlightblue">+ nsACString strA; | |||
+ nsACString strB; | |||
+ if (strA.Equals(strB, nsCaseInsensitiveCStringComparator())) { | |||
... | |||
+ } | |||
+ | |||
+ nsString strA; | |||
+ nsString strB; | |||
+ if (strA.Equals(strB, nsCaseInsensitiveStringComparator())) { | |||
... | |||
+ }</span> | |||
== nsCOMPtr == | == nsCOMPtr == | ||
| Line 75: | Line 106: | ||
<span class="highlightblue">+ NS_IF_ADDREF(*aIdentity = m_identity);</span> | <span class="highlightblue">+ NS_IF_ADDREF(*aIdentity = m_identity);</span> | ||
== Validating Input Arguments == | == Validating Input Arguments & Return Values == | ||
Use NS_ENSURE_ARG_POINTER to validate argument pointers instead of explicitly validating the argument: | Use NS_ENSURE_ARG_POINTER to validate argument pointers instead of explicitly validating the argument: | ||
| Line 91: | Line 122: | ||
<span class="highlightred">- if (!m_identities) return NS_ERROR_FAILURE;</span> | <span class="highlightred">- if (!m_identities) return NS_ERROR_FAILURE;</span> | ||
<span class="highlightblue">+ NS_ENSURE_TRUE(m_identities, NS_ERROR_FAILURE);</span> | <span class="highlightblue">+ NS_ENSURE_TRUE(m_identities, NS_ERROR_FAILURE);</span> | ||
I see a surprising amount of return value code that looks like: | |||
<span class="highlightred">- if (!*_retval) return NS_ERROR_FAILURE; | |||
- return NS_OK;</span> | |||
<span class="highlightblue">+ return (*retval) ? NS_OK : NS_ERROR_FAILURE</span> | |||
There's no need to check for a failure code and a null value: | |||
nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aNntpServer, &rv); | |||
<span class="highlightred">- if (NS_FAILED(rv)) return rv; | |||
- if (!server) return NS_ERROR_FAILURE;</span> | |||
<span class="highlightblue">+ NS_ENSURE_SUCCESS(rv, rv);</span> | |||
== nsISupportsArray == | == nsISupportsArray == | ||
| Line 113: | Line 156: | ||
* Some of the methods in our IDL files start with an upper case letter. When cleaning up an interface, change these to lower case. (Don't forget to search through http://mxr.mozilla.org and fix any JS callers of the API to also be lower case!) | * Some of the methods in our IDL files start with an upper case letter. When cleaning up an interface, change these to lower case. (Don't forget to search through http://mxr.mozilla.org and fix any JS callers of the API to also be lower case!) | ||
* The more nsCRT/PL_strcmp routines that get replaced with nsString equivalents as part of the string cleanup, the easier it will be to make a later pass through the code to prepare for using frozen linkage where we won't get nsCRT routines anymore. | * The more nsCRT/PL_strcmp routines that get replaced with nsString equivalents as part of the string cleanup, the easier it will be to make a later pass through the code to prepare for using frozen linkage where we won't get nsCRT routines anymore. | ||
* Remove braces around single statement clauses: | |||
if (test) | |||
<span class="highlightred">-{</span> | |||
doSomething(); | |||
<span class="highlightred">-}</span> | |||
* Local variables that begin with the letter 'a'. Variables that begin with the letter 'a' are reserved for function arguments and should not be used at the start of a local variable: | |||
<span class="highlightred">- nsCOMPtr<nsIWebProgressListener> aProgressListener;</span> | |||
<span class="highlightblue">+ nsCOMPtr<nsIWebProgressListener> progressListener;</span> | |||