Closed
Bug 730147
Opened 13 years ago
Closed 13 years ago
Need style fixes in mailnews/import codes.
Categories
(MailNews Core :: Import, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 14.0
People
(Reporter: hiro, Assigned: hiro)
Details
Attachments
(1 file, 4 obsolete files)
891.08 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → hiikezoe
Assignee | ||
Comment 1•13 years ago
|
||
$ find mailnews/import -type f -regex ".*[h|cpp]$" | xargs sed -i -e 's/(\s/(/' -e 's/\s)/)/' -e 's/return(\(.*\))/return \1/
Attachment #607028 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 2•13 years ago
|
||
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); }
Attachment #607028 -
Attachment is obsolete: true
Attachment #607028 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 3•13 years ago
|
||
$ 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
Attachment #607821 -
Flags: review?(dbienvenu)
Comment 4•13 years ago
|
||
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());
Attachment #607821 -
Flags: review?(dbienvenu) → review+
Assignee | ||
Comment 5•13 years ago
|
||
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.
Attachment #607821 -
Attachment is obsolete: true
Attachment #608269 -
Flags: review+
Assignee | ||
Comment 6•13 years ago
|
||
Comment 7•13 years ago
|
||
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;
Attachment #608270 -
Flags: review+
Assignee | ||
Comment 8•13 years ago
|
||
Thank you, David!
Attachment #608269 -
Attachment is obsolete: true
Attachment #608517 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Attachment #608270 -
Attachment is obsolete: true
Comment 9•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
You need to log in
before you can comment on or make changes to this bug.
Description
•