730147 - Need style fixes in mailnews/import codes.

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

The codes in mailnews/import has differential code style. 

For instance, 
* a space after the open bracket
* return value in brackets

Whole of those styles should be fixed in one fix.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: fix

$ find mailnews/import -type f -regex  ".*[h|cpp]$" | xargs sed -i -e 's/(\s/(/' -e 's/\s)/)/' -e 's/return(\(.*\))/return \1/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 607028 [details] [diff] [review]
fix

Urghh.

mailnews/import/src/ImportCharset.h has one-liner...

  inline static PRUint8 ToLower( PRUint8 ch) { if ((m_Ascii[ch] & cAlphaChar) == cAlphaChar) { return( cLowerAChar + (m_upperCaseMap[ch] - cUpperAChar)); } else return( ch); }

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

$ find mailnews/import -type f -regex  ".*[h|cpp]$" | xargs sed -i -e 's/(\s/(/g' -e 's/\s)/)/g' -e 's/return\s*(\([^;^&^|^>]*\));/return \1;/g' -e 's/return(/return (/g'

This sed expression does not support return statement like this:

return (PRUint32)(i - 1);

But fortunately there is no such statement in mailnews/import. (I suppose it should be used static_cast in such case.)


Pushed to try server.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=a24c3815131f

Comment 4

[David :Bienvenu]

Comment on attachment 607821 [details] [diff] [review]
Revised patch

thx for doing this. While we're here, we might as well clean up a few of the other obvious things:

don't need braces here (or != nsnull either, for that matter)

+      if ((pEntry = ProcessAlias(pLine, len, errors)) != nsnull) {
+        m_alias.AppendElement(pEntry);
       }

can you break up this long line?

-            ResolveEntries( pEntry->m_name, pEntry->m_list, result, addResolvedEntries, true, numResolved);
+            ResolveEntries(pEntry->m_name, pEntry->m_list, result, addResolvedEntries, true, numResolved);

this line way too long:

-void nsEudoraCompose::GetNthHeader( const char *pData, PRInt32 dataLen, PRInt32 n, nsCString& header, nsCString& val, bool unwrap)
+void nsEudoraCompose::GetNthHeader(const char *pData, PRInt32 dataLen, PRInt32 n, nsCString& header, nsCString& val, bool unwrap)

and this:
-void nsEudoraCompose::GetHeaderValue( const char *pData, PRInt32 dataLen, const char *pHeader, nsCString& val, bool unwrap)
+void nsEudoraCompose::GetHeaderValue(const char *pData, PRInt32 dataLen, const char *pHeader, nsCString& val, bool unwrap)

don't need braces here:

     if (end > start) {
-      val.Append( pData + start, end - start);
+      val.Append(pData + start, end - start);
     }

just remove the commented out line here:
-    // NS_PRECONDITION( false, "Forced Break");
+    // NS_PRECONDITION(false, "Forced Break");
 
these two ifs can be combined into one:

-  if (NS_FAILED( rv)) {
+  if (NS_FAILED(rv)) {
     // in this case, if we did not use the default compose
     // charset, then try that.
-    if (!charSet.Equals( m_defCharset)) {
+    if (!charSet.Equals(m_defCharset)) {
       body.Truncate();
-      rv = nsMsgI18NConvertFromUnicode( NS_LossyConvertUTF16toASCII(charSet).get(),
+      rv = nsMsgI18NConvertFromUnicode(NS_LossyConvertUTF16toASCII(charSet).get(),
                                         uniBody, body);
     }
   }

return should be on its own line:
-      if (!Grow( sz)) return( false);
+      if (!Grow(sz)) return false;

this can be:

return (count < len) ? count : -1;

   if (count < len)
-    return( count);
+    return count;
 
-  return( -1);
+  return -1;

wrap long line:
-nsresult nsEudoraCompose::CopyComposedMessage( nsCString& fromLine, nsIFile *pSrc, nsIOutputStream *pDst, SimpleBufferTonyRCopiedOnce& copy)
+nsresult nsEudoraCompose::CopyComposedMessage(nsCString& fromLine, nsIFile *pSrc, nsIOutputStream *pDst, SimpleBufferTonyRCopiedOnce& copy)

don't need braces here:
-  if (NS_SUCCEEDED( rv)) {
-    rv = WriteHeaders( pDst, m_readHeaders);
+  if (NS_SUCCEEDED(rv)) {
+    rv = WriteHeaders(pDst, m_readHeaders);
   }

returns on their own line:

-    if (NS_FAILED( rv)) return( rv);
-    if (bytesRead != count) return( NS_ERROR_FAILURE);
+    if (NS_FAILED(rv)) return rv;
+    if (bytesRead != count) return NS_ERROR_FAILURE;

nsEudoraCompose::WriteHeaders contains several lines in need of wrapping.

-    if (m_pBuffer) { m_size = sz; return( true); }
-    else { m_size = 0; return( false);}
+    if (m_pBuffer) { m_size = sz; return true; }
+    else { m_size = 0; return false;}
   }

this should be:
if (m_pBuffer) {
  m_size = sz;
  return true;
}
m_size = 0;
return false;

actually, that whole file needs rewriting/reformatting better left to an other bug.

super long line:

-    while ( (startEmbeddedContentLine = m_body.Find(sEudoraEmbeddedContentLines[i], true, startEmbeddedContentLine+1)) != kNotFound )
+    while ((startEmbeddedContentLine = m_body.Find(sEudoraEmbeddedContentLines[i], true, startEmbeddedContentLine+1)) != kNotFound)

this:

       nsCOMPtr<nsIDOMHTMLImageElement>   imageNode;
-      image->QueryInterface( NS_GET_IID(nsIDOMHTMLImageElement), getter_AddRefs(imageNode) );
+      image->QueryInterface(NS_GET_IID(nsIDOMHTMLImageElement), getter_AddRefs(imageNode));

can be simply:
      nsCOMPtr<nsIDOMHTMLImageElement> imageNode(do_QueryInterface(image));

NS_RELEASE nulls out the pointer, so we don't need the braces, or the *ppArray = nsnull:

+  if (NS_FAILED(rv) && *ppArray) {
+    NS_RELEASE(*ppArray);
     *ppArray = nsnull;
   }

bad indentation of return :

+    ImportEudoraMailImpl::ReportError(EUDORAIMPORT_ADDRESS_BADSOURCEFILE, name, &error);
+    ImportEudoraMailImpl::SetLogs(success, error, pErrorLog, pSuccessLog);
+      return NS_ERROR_FAILURE;

don't need braces here:

-  if (NS_SUCCEEDED( rv) && error.IsEmpty()) {
-    ReportSuccess( name, &success);
+  if (NS_SUCCEEDED(rv) && error.IsEmpty()) {
+    ReportSuccess(name, &success);
   }
   else {
-    ImportEudoraMailImpl::ReportError( EUDORAIMPORT_ADDRESS_CONVERTERROR, name, &error);
+    ImportEudoraMailImpl::ReportError(EUDORAIMPORT_ADDRESS_CONVERTERROR, name, &error);
   }

don't need separate nulling out of localMailAccount, and can use else if (...) on one line:
             if (localMailAccount && *localMailAccount)
             {
-              NS_RELEASE( *localMailAccount);
+              NS_RELEASE(*localMailAccount);
               *localMailAccount = nsnull;
             }
           }
           else
           {
             if (localMailAccount)
             {
               *localMailAccount = pAccount;
-              NS_IF_ADDREF( pAccount);
+              NS_IF_ADDREF(pAccount);
             }
           }
this:

   if (fName.LowerCaseEqualsLiteral("eudora nicknames"))
-    return( false);
-  return( true);
+    return false;
+  return true;

can be return !fName.LowerCaseEqualsLiteral("eudora nicknames");

nsEudoraMac.h contains a lot of lines that should be wrapped.

don't need any of these braces:

   if (!pPath.IsEmpty()) {
-    IMPORT_LOG1( "%s", pPath.get());
+    IMPORT_LOG1("%s", pPath.get());
   }
   else {
-    IMPORT_LOG0( "Unknown");
+    IMPORT_LOG0("Unknown");
   }
   if (endLine) {
-    IMPORT_LOG0( "\n");
+    IMPORT_LOG0("\n");
   }

tabs/indentation issue:

         rv = NS_NewLocalFileInputStream(getter_AddRefs(srcInputStream), pSrc);
-        if (NS_FAILED( rv))
-    return( rv);
+        if (NS_FAILED(rv))
+    return rv;

no braces:

-    if (NS_FAILED( rv)) {
-      IMPORT_LOG0( "*** Error copying composed message to destination mailbox\n");
+    if (NS_FAILED(rv)) {
+      IMPORT_LOG0("*** Error copying composed message to destination mailbox\n");
     }

these lines can be combined (unwrapped)

-    rv = pDst->Write( kComposeErrorStr,
-      strlen( kComposeErrorStr),
-      &written );
+    rv = pDst->Write(kComposeErrorStr,
+      strlen(kComposeErrorStr),
+      &written);

no braces:
+    if (NS_FAILED(rv)) {
+      IMPORT_LOG0("*** Error writing to destination mailbox\n");
     }

return on its own line:

-    if (NS_FAILED( rv)) return( rv);
-    if (bytesRead != PRUint32(count)) return( NS_ERROR_FAILURE);
+    if (NS_FAILED(rv)) return rv;
+    if (bytesRead != PRUint32(count)) return NS_ERROR_FAILURE;

remove these commented out lines:

-// ::RegOpenKeyEx( HKEY_CURRENT_USER, "Software\\Accounts", 0, KEY_QUERY_VALUE | KEY_ENUMERATE_SUB_KEYS, &sKey) == ERROR_SUCCESS)
+// ::RegOpenKeyEx(HKEY_CURRENT_USER, "Software\\Accounts", 0, KEY_QUERY_VALUE | KEY_ENUMERATE_SUB_KEYS, &sKey) == ERROR_SUCCESS)

same comments as before for this duplicated code:
             if (localMailAccount && *localMailAccount)
             {
-              NS_RELEASE( *localMailAccount);
+              NS_RELEASE(*localMailAccount);
               *localMailAccount = nsnull;
             }
           }
           else
           {
             if (localMailAccount)
             {
               *localMailAccount = pAccount;
-              NS_IF_ADDREF( pAccount);
+              NS_IF_ADDREF(pAccount);
             }

tabs/indentation:

+              pFile->GetLastModifiedTime(&modDate1);
               if (modDate2 > modDate1)
-                                  pLocalFile->InitWithFile( entry);
+                                  pLocalFile->InitWithFile(entry);

this can be one line:

-  hr = lpContainer->GetContentsTable( 0,
+  hr = lpContainer->GetContentsTable(0,
     &lpAB);

tabs/indentation:
            for(j = 0; j < pVal->Value.MVszA.cValues; j++) {
-        CStrToUnicode( (const char *) (pVal->Value.MVszA.lppszA[j]), tmp);
+        CStrToUnicode((const char *) (pVal->Value.MVszA.lppszA[j]), tmp);
                 val += tmp;
                 val.Append(NS_ConvertASCIItoUTF16(TR_OUTPUT_EOL));

I notice that these methods are commented out CWabIterateProcess::HandleListUser, etc. Might be good to figure out why.

return on its own line, or, NS_ENSURE_SUCCESS(rv, rv);
 
   rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), inFile);
-  if (NS_FAILED( rv)) return( rv);
+  if (NS_FAILED(rv)) return rv;

indentation/tabs:

@@ -217,70 +217,70 @@ nsresult nsOEAddressIterator::EnumList( 
         ULONG cbEID = lpRowAB->aRow[0].lpProps[ieidPR_ENTRYID].Value.bin.cb;
       
         // There are 2 kinds of objects - the MAPI_MAILUSER contact object
         // and the MAPI_DISTLIST contact object
         // For distribution lists, we will only consider MAILUSER
         // objects since we can't nest lists yet.
         if(lpRowAB->aRow[0].lpProps[ieidPR_OBJECT_TYPE].Value.l == MAPI_MAILUSER)
         {
-          LPMAILUSER  pUser = m_pWab->GetUser( cbEID, lpEID);
-          LPSPropValue pProp = m_pWab->GetUserProperty( pUser, PR_EMAIL_ADDRESS);

and more later in the method...

don't need braces here:

   if (!displayName.IsEmpty()) {
-    m_database->AddDisplayName( newRow, NS_ConvertUTF16toUTF8(displayName).get());
+    m_database->AddDisplayName(newRow, NS_ConvertUTF16toUTF8(displayName).get());
   }

or any of the following lines...

dont need braces here:

-  if (NS_SUCCEEDED( rv)) {
-    ReportSuccess( name, msgCount, &success);
+  if (NS_SUCCEEDED(rv)) {
+    ReportSuccess(name, msgCount, &success);
   }
   else {
-    ReportError( OEIMPORT_MAILBOX_CONVERTERROR, name, &error);
+    ReportError(OEIMPORT_MAILBOX_CONVERTERROR, name, &error);
   }

can combine if's and remove braces here:

   if (SUCCEEDED(hr))
   {
     if (NS_SUCCEEDED(source->GetPreferredName(name)))
     {
-      ReportSuccess( name, &success);
+      ReportSuccess(name, &success);
     }
   }
   else
-    ImportOEMailImpl::ReportError( OEIMPORT_ADDRESS_CONVERTERROR, name, &error);
+    ImportOEMailImpl::ReportError(OEIMPORT_ADDRESS_CONVERTERROR, name, &error);

this can be

return m_entryArray.Count();

   if (m_entryArray.Count())
-    return( true);
+    return true;
   else
-    return( false);
+    return false;

this should be return NS_SUCCEEDED(rv) && cntRead == sizeof(val);
(same thing for the next function)

-  if (NS_FAILED( rv) || (cntRead != sizeof( val)))
-    return( false);
-  return( true);
+  if (NS_FAILED(rv) || (cntRead != sizeof(val)))
+    return false;
+  return true;

long line, and don't need braces:

-  if (::RegOpenKeyEx( HKEY_CURRENT_USER, "Software\\Microsoft\\Outlook Express", 0, KEY_QUERY_VALUE, &sKey) == ERROR_SUCCESS) {
-    return( sKey);
+  if (::RegOpenKeyEx(HKEY_CURRENT_USER, "Software\\Microsoft\\Outlook Express", 0, KEY_QUERY_VALUE, &sKey) == ERROR_SUCCESS) {
+    return sKey;
   }

I believe this can use result.Adopt(ptrv) so you won't need the free.

+void nsOEStringBundle::GetStringByID(PRInt32 stringID, nsString& result)
 {
-  PRUnichar *ptrv = GetStringByID( stringID);
+  PRUnichar *ptrv = GetStringByID(stringID);
   result = ptrv;
-  FreeString( ptrv);
+  FreeString(ptrv);

looks like there are tabs/bad indentations here in CMapiApi::IterateHierarchy and CMapiApi::IterateStores

can just return the if clause here:
+  if ((PROP_TYPE(pVal->ulPropTag) == PT_ERROR) && (pVal->Value.l == E_OUTOFMEMORY)) {
+    return TRUE;
   }
-  return( FALSE);
+  return FALSE;

might as well remove the commented out lines:
    if (abortCnt >= kHungAbortCount) {
-      IMPORT_LOG0( "**** Create and send message hung\n");
-//      IMPORT_LOG1( "Headers: %s\n", m_Headers.get());
-//      IMPORT_LOG1( "Body: %s\n", m_Body.get());
+      IMPORT_LOG0("**** Create and send message hung\n");
+//      IMPORT_LOG1("Headers: %s\n", m_Headers.get());
+//      IMPORT_LOG1("Body: %s\n", m_Body.get());
       rv = NS_ERROR_FAILURE;
     }

indentation wrong:

+NS_IMETHODIMP nsOutlookImport::GetDescription(PRUnichar **name)
 {
     NS_PRECONDITION(name != nsnull, "null ptr");
     if (! name)
         return NS_ERROR_NULL_POINTER;
 
-  *name = nsOutlookStringBundle::GetStringByID( OUTLOOKIMPORT_DESCRIPTION);
+  *name = nsOutlookStringBundle::GetStringByID(OUTLOOKIMPORT_DESCRIPTION);
 
     return NS_OK;
 }
don't need these braces:
       else {
-        IMPORT_LOG1( "*** Error reading message from mailbox: %S\n", pName);
+        IMPORT_LOG1("*** Error reading message from mailbox: %S\n", pName);
       }

this can simply return NS_SUCCEEDED(rv) && written == len;
-BOOL nsOutlookMail::WriteData( nsIOutputStream *pDest, const char *pData, PRInt32 len)
+BOOL nsOutlookMail::WriteData(nsIOutputStream *pDest, const char *pData, PRInt32 len)
 {
   PRUint32    written;
-  nsresult rv = pDest->Write( pData, len, &written);
-  if (NS_FAILED( rv) || (written != len))
-    return( FALSE);
-  return( TRUE);
+  nsresult rv = pDest->Write(pData, len, &written);
+  if (NS_FAILED(rv) || (written != len))
+    return FALSE;
+  return TRUE;

don't need braces here:

         if (oldRow)
         {
           newRow = oldRow;
         }
         else
         {
-          pDb->AddCardRowToDB( newRow);
+          pDb->AddCardRowToDB(newRow);
         }
remove commented out line, and fix indentation:

-bool ImportOutFile::InitOutFile( nsIFile *pFile, PRUint32 bufSz)
+bool ImportOutFile::InitOutFile(nsIFile *pFile, PRUint32 bufSz)
 {
   if (!bufSz)
     bufSz = 32 * 1024;
   if (!m_pBuf) {
     m_pBuf = new PRUint8[ bufSz];
   }
 
-  // m_fH = UFile::CreateFile( oFile, kMacNoCreator, kMacTextFile);
+  // m_fH = UFile::CreateFile(oFile, kMacNoCreator, kMacTextFile);

nsImportGenericAddressBooks::SetData has bad indentation/tabs.

else after return here:
NS_METHOD nsIImportMimeEncodeImpl::DoWork(bool *done, bool *_retval)
 {
   if (done && _retval && m_pEncode) {
-    *_retval = m_pEncode->DoWork( done);
-    return( NS_OK);
+    *_retval = m_pEncode->DoWork(done);
+    return NS_OK;
   }
   else
-    return( NS_ERROR_FAILURE);
+    return NS_ERROR_FAILURE;

same thing in nsIImportMimeEncodeImpl::DoEncoding


returns on their own line, please, and fix return after else:

+NS_IMETHODIMP nsImportService::GetModuleWithCID(const nsCID& cid, nsIImportModule **ppModule)
 {
     NS_PRECONDITION(ppModule != nsnull, "null ptr");
     if (! ppModule)
         return NS_ERROR_NULL_POINTER;
 
   *ppModule = nsnull;
   nsresult rv = DoDiscover();
-  if (NS_FAILED( rv)) return( rv);
-  if (m_pModules == nsnull) return( NS_ERROR_FAILURE);
+  if (NS_FAILED(rv)) return rv;
+  if (m_pModules == nsnull) return NS_ERROR_FAILURE;
   PRInt32  cnt = m_pModules->GetCount();
   ImportModuleDesc *pDesc;
   for (PRInt32 i = 0; i < cnt; i++) {
-    pDesc = m_pModules->GetModuleDesc( i);
-    if (!pDesc) return( NS_ERROR_FAILURE);
-    if (pDesc->GetCID().Equals( cid)) {
+    pDesc = m_pModules->GetModuleDesc(i);
+    if (!pDesc) return NS_ERROR_FAILURE;
+    if (pDesc->GetCID().Equals(cid)) {
       *ppModule = pDesc->GetModule();
 
-      IMPORT_LOG0( "* nsImportService::GetSpecificModule - attempted to load module\n");
+      IMPORT_LOG0("* nsImportService::GetSpecificModule - attempted to load module\n");
 
       if (*ppModule == nsnull)
-        return( NS_ERROR_FAILURE);
+        return NS_ERROR_FAILURE;
       else
-        return( NS_OK);

don't need braces here:

-      if (pData->Process( pStart, cnt)) {
-        pEntry->m_list.AppendElement( pData);
+      if (pData->Process(pStart, cnt)) {
+        pEntry->m_list.AppendElement(pData);
       }


don't need braces here:

+    if (NS_SUCCEEDED(rv)) {
+      pSettings->QueryInterface(kISupportsIID, (void **)ppInterface);
     }

these if's can be combined and the braces removed:

if (NS_SUCCEEDED(rv) && str.Equals(prefStr))
  done = true; 

+    if (NS_SUCCEEDED(rv)) {
       if (str.Equals(prefStr))
         done = true;
     }


don't need braces here:
     if (!done) {
-      rv = prefs->SetCharPref( "mailnews.import.text.fieldmap", str.get());
+      rv = prefs->SetCharPref("mailnews.import.text.fieldmap", str.get());
     }

actually, looking at the code, it can be rewritten not to use a temp var at all:

if (NS_FAILED(rv) || !str.Equals(prefStr))
  rv = prefs->SetCharPref("mailnews.import.text.fieldmap", str.get());

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

I've confirmed this patch can be built both on Linux and WinXP.

Mac build is on try server:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=1503464a1d95

I will attach the difference from the previous patch for the record.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: The diff

Comment 7

[David :Bienvenu]

Comment on attachment 608270 [details] [diff] [review]
The diff

looks good, thx!

one more thing - can you remove the spaces after nsCOMPtr here? No need for re-review

+  nsCOMPtr <nsIInputStream> srcInputStream;
+  nsCOMPtr <nsIInputStream> tocInputStream;
+  nsCOMPtr <nsIOutputStream> mailOutputStream;

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Remove the spaces after nsCOMPtr

Thank you, David!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/355c22d76613