Thunderbird:CodeCleanup: Difference between revisions

← Older edit

Thunderbird:CodeCleanup (view source)

Revision as of 17:51, 25 July 2007

4,955 bytes added , 25 July 2007

m

→‎Strings: end the sentence:)

Mkmelin

129

edits

@@ Line 1: / Line 1: @@
 In preparation for Thunderbird and Seamonkey moving to frozen linkage/libxul/XULRunner, we need to remove our use of nsXPIDLStrings from mailnews (along with other changes). While we are doing this, we're taking the opportunity to modernize some of our core interfaces to use AString and ACString instead of wstring and string.
-In starting to make these changes, we've noticed inefficient code patterns in our C++ code in mailnews. A lot of of these patterns can be improved as part of the AString API changes. By documenting these patterns and the associated fixes, it will hopefully be easier for more folks to help out in the great mailnews code cleanup of 2007!
+In starting to make these changes, we've noticed inefficient code patterns in our C++ code in mailnews. A lot of of these patterns can be improved as part of the AString changes we are making to the interfaces. By documenting these patterns and the associated code improvements, it will hopefully be easier for more folks to help out in the great mailnews code cleanup of 2007!
+== How To Help ==
+* Read the rest of this document.
+* Look at some of the patches we are working on in [https://bugzilla.mozilla.org/show_bug.cgi?id=379070 Bug 379070] to get a feel for what we are changing and cleaning up.
+*I recommend keeping the focus of the changes as narrow as possible to avoid too much fan out.
+*Pick a single interface like something in mailnews\base\public\search. Ask in the bug or e-mail mscott@mozilla.org for help in picking an interface to cleanup. Audit the interface making AString/ACString changes where they are appropriate.
+*Change the interface ID.
+*Fix the implementation for that interface, adjusting the methods to account for the new signatures.
+*As you audit the implementation of the interface, examine the code method by method, looking for and correcting the patterns described in this document.
+*After that's done, start fixing up the C++ callers of the APIs that you've changed. If a caller is using a nsXPIDLString, be sure to change that to nsString.
+*Post a patch in [https://bugzilla.mozilla.org/show_bug.cgi?id=379070 Bug 379070] for the interface & implementation you've cleaned up.
+*If you have any questions, just ask us in the bug or in the newsgroup!
+== IDL Changes ==
+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="highlightblue">+ attribute AString key;</span>
+ <span class="highlightred">- string getChromePackageName(in string aExtensionName);</span>
+ <span class="highlightblue">+ ACString getChromePackageName(in ACString aExtensionName);</span>
 == Strings ==
@@ Line 12: / Line 33: @@
   <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;
@@ Line 22: / Line 43: @@
   nsCString charValue;
   <span class="highlightred">- uniValue.Assign(NS_ConvertASCIItoUTF16(charValue));</span>
-  <span class="highlightblue">+ CopyASCIItoUTF16(realName, fullName);</span>
+  <span class="highlightblue">+ CopyASCIItoUTF16(charValue, uniValue);</span>
   nsCString charValue;
@@ Line 28: / Line 49: @@
   <span class="highlightred">- charValue.AssignWithConversion(uniValue);</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;
@@ Line 40: / Line 68: @@
   <span class="highlightred">- nsCRT::strcmp(charStr.get(), charStr2.get()) == 0</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 ==
@@ Line 54: / Line 106: @@
   <span class="highlightblue">+ NS_IF_ADDREF(*aIdentity = m_identity);</span>
-== Validating Input Arguments ==
+== Validating Input Arguments & Return Values ==
-NS_ENSURE_ARG_POINTER
-NS_ENSURE_SUCCESS
+Use NS_ENSURE_ARG_POINTER to validate argument pointers instead of explicitly validating the argument:
+  NS_IMETHODIMP nsMsgAccount::GetIdentities(nsISupportsArray **_retval)
+ <span class="highlightred">- if (!_retval) return NS_ERROR_NULL_POINTER;</span>
+ <span class="highlightblue">+ NS_ENSURE_ARG_POINTER(_retval);</span>
+In many cases, you can use NS_ENSURE_SUCCESS instead of explicitly checking for a failure and returning:
+ <span class="highlightred">- if (NS_FAILED(rv)) return rv;</span>
+ <span class="highlightblue">+ NS_ENSURE_SUCCESS(rv, rv);</span>
+You can use NS_ENSURE_TRUE for things like:
+ <span class="highlightred">- if (!m_identities) return 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 ==
@@ Line 79: / 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!)
 * 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>