Closed Bug 1402750 Opened 2 years ago Closed 2 years ago

Replace Assign/Append/Insert/Equals("") or (NS_LITERAL_(C)STRING) with Assign/Append/Insert/EqualsLiteral("") or (u"")

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(5 files, 8 obsolete files)

18.29 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
155.17 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
35.37 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
13.17 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
6.64 KB, patch
aceman
: review+
Details | Diff | Splinter Review
See bug 870698 comment #20 for details.

That looks like an easy sed job.
Done with sed
  find mailnews -name *.cpp  -exec sed -i -e 's/\.Assign("/.AssignLiteral("/' {} \;
and compiles.
Attachment #8911829 - Flags: review?(acelists)
Done with sed
  find mailnews -name *.cpp  -exec sed -i -e 's/\.Assign(NS_LITERAL_CSTRING("/.AssignLiteral("/' {} \;
and manual removal of extra parenthesis and compiles.
Attachment #8911834 - Flags: review?(acelists)
Attachment #8911834 - Attachment description: 1402750-replacewith-XXXLiteral-2.patch → 1402750-replacewith-XXXLiteral-2.patch - part 2
Comment on attachment 8911829 [details] [diff] [review]
1402750-replacewith-XXXLiteral.patch - part 1

Review of attachment 8911829 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks.
Will you also do some of the Assing/Append/Equals(NS_LITERAL_(C)STRING()) games?

::: mailnews/addrbook/src/nsAbAddressCollector.cpp
@@ +210,5 @@
>    // username@aol.com (America Online)
>    // username@cs.com (Compuserve)
>    // username@netscape.net (Netscape webmail)
>    // are all AIM screennames.  autocollect that info.
> +  if (domain.EqualsLiteral("aol.com") || domain.Equals("cs.com") ||

Why not EqualsLiteral on cs.com ?

@@ +216,2 @@
>      aCard->SetPropertyAsAUTF8String(kScreenNameProperty, Substring(aEmail, 0, atPos));
> +  else if (domain.EqualsLiteral("gmail.com") || domain.Equals("googlemail.com"))

Why not EqualsLiteral on googlemail.com ? Insufficient 'sed' rule?

::: mailnews/addrbook/src/nsAbBoolExprToLDAPFilter.cpp
@@ +67,5 @@
>              nsCString name;
>              rv = childCondition->GetName (getter_Copies (name));
>              NS_ENSURE_SUCCESS(rv, rv);
>  
> +            if(name.EqualsLiteral("card:nsIAbCard"))

Space after if.

::: mailnews/addrbook/src/nsAbCardProperty.cpp
@@ +127,5 @@
>  
>  NS_IMETHODIMP nsAbCardProperty::GetUuid(nsACString &uuid)
>  {
>    // If we have indeterminate sub-ids, return an empty uuid.
> +  if (m_directoryId.EqualsLiteral("") || m_localId.Equals(""))

Maybe .IsEmpty() on both? Or is that not the same?

::: mailnews/addrbook/src/nsAbLDAPDirectory.cpp
@@ +243,5 @@
>    // Now we need to send an update which will ensure our indicators and
>    // listeners get updated correctly.
>  
>    // See if they both start with ldaps: or ldap:
> +  bool newIsNotSecure = StringHead(tempLDAPURL, 5).EqualsLiteral("ldap:");

StringBeginsWith?

@@ +248,3 @@
>  
>    if (oldUrl.IsEmpty() ||
> +      StringHead(oldUrl, 5).EqualsLiteral("ldap:") != newIsNotSecure)

StringBeginsWith?

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +404,5 @@
>  }
>  
>  bool nsMsgFilterList::StrToBool(nsCString &str)
>  {
> +  return str.EqualsLiteral("yes") ;

Drop the space.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +2949,5 @@
>            nsresult irv;
>            version = buffer.ToInteger(&irv);
>            continue;
>          }
> +        if (Substring(buffer, 0, 4).EqualsLiteral("uri="))

StringBeginsWith?

@@ +3013,5 @@
>                  break;
>              } while (!grandParent && !isServer);
>            }
>          }
> +        else if (dbFolderInfo && Substring(buffer, 0, 6).EqualsLiteral("scope="))

StringBeginsWith?

@@ +3025,5 @@
>              dbFolderInfo->SetCharProperty(kSearchFolderUriProp, buffer);
>              AddVFListenersForVF(virtualFolder, buffer, rdf, msgDBService);
>            }
>          }
> +        else if (dbFolderInfo && Substring(buffer, 0, 6).EqualsLiteral("terms="))

StringBeginsWith?

@@ +3030,5 @@
>          {
>            buffer.Cut(0, 6);
>            dbFolderInfo->SetCharProperty("searchStr", buffer);
>          }
> +        else if (dbFolderInfo && Substring(buffer, 0, 13).EqualsLiteral("searchOnline="))

StringBeginsWith?

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
@@ +540,5 @@
>          // important: leave out sender field. Too strong of an indicator
>          break;
>      case 'x': // (2) X-Mailer / user-agent works best if it is untokenized, just fold the case and any leading/trailing white space
>          // all headers beginning with x-mozilla are being changed by us, so ignore
> +        if (Substring(headerName, 0, 9).EqualsLiteral("x-mozilla"))

StringBeginsWith?

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1114,5 @@
>    }
>    else
>      tokenStr.Assign(tempFolderName);
>  
>    if ((int32_t(PL_strcasecmp(tokenStr.get(), "INBOX"))==0) && (strcmp(tokenStr.get(), "INBOX") != 0))

Can this ugliness be improved?
Attachment #8911829 - Flags: review?(acelists) → review+
Comment on attachment 8911834 [details] [diff] [review]
1402750-replacewith-XXXLiteral-2.patch - part 2

Review of attachment 8911834 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1916,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>      if (curLine.IsEmpty())
>        break;
>      msgHeaders.Append(curLine);
> +    msgHeaders.AppendLiteral("\r\n");

Would it be safe to use CRLF constant as in e.g. nsImapProtocol.cpp:8785 ?
Attachment #8911834 - Flags: review?(acelists) → review+
Done with sed
  find mailnews -name *.cpp  -exec sed -i -e 's/\.Assign(NS_LITERAL_STRING("/.AssignLiteral(u"/' {} \;
and manual removal of extra parenthesis and compiles.
I was afraid, I'd get into a general clean-up discussion :-(
I fixed what was asked for.
Attachment #8911829 - Attachment is obsolete: true
Attachment #8911856 - Flags: review+
Oops, missed one.
Attachment #8911856 - Attachment is obsolete: true
Attachment #8911860 - Flags: review+
Comment on attachment 8911848 [details] [diff] [review]
1402750-replacewith-XXXLiteral-3.patch - part 3

Review of attachment 8911848 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +825,1 @@
>                  newValue.AppendLiteral("\"");

Why does this second "\"" not need the 'u'? Or does it still work but have some perf hit as discussed in bug 870698 as the char type doesn't match?

::: mailnews/base/src/nsMsgFolderDataSource.cpp
@@ +1017,2 @@
>      nameString.AppendInt(unreadMessages);
>      nameString.Append(u')');

This one now looks inconsistent.

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +366,2 @@
>        errorMsg.Append(errorString);
>        errorMsg.AppendLiteral("?]");

Inconsistent without 'u'.
Attachment #8911848 - Flags: review?(acelists) → review+
I can add the missing 'u's, but maybe we should stick with |nameString.Append(u')');|, in fact, we might consider using Assign('x') or Append('x') as per bug 870698 comment #16 instead of one-char strings.

What do you think?
(In reply to Jorg K (GMT+2) from comment #9)
Yes, but maybe we can wait for the answer why it is better.
In mailnews there are 21 occurrences of |.AssignLiteral(".");|, all pre-existing and 11O of |.AppendLiteral("x");|, 58 pre-existing, 52 added here, of these 49 coming from Append("x").
OK, for a single character append use Append('x');
Attachment #8911860 - Attachment is obsolete: true
Attachment #8911957 - Flags: review+
for a single character append use Append('x');
Attachment #8911834 - Attachment is obsolete: true
Added missing "u"s and switched a single character append.
Attachment #8911848 - Attachment is obsolete: true
Attachment #8911964 - Flags: review+
Attachment #8911959 - Attachment description: 1402750-replacewith-XXXLiteral-2.patch (v2) → 1402750-replacewith-XXXLiteral-2.patch -part 2 (v2)
OK, single character literals now use Assign('') and Append('').
Attachment #8911973 - Flags: review?(acelists)
Comment on attachment 8911973 [details] [diff] [review]
1402750-single-character.patch - part 4 (v1)

Review of attachment 8911973 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
@@ +470,5 @@
>      // the filter works as a term to &
>      //
>      if (urlFilter[0] != '(')
>      {
>        searchFilter = NS_LITERAL_CSTRING("(&(");

What about AssignLiteral here? I think bug 870698 there is talk that operator= does not have the optimizations yet.

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +479,5 @@
>        NS_MsgGetUntranslatedPriorityName(priorityValue, priorityName);
>  
>        // Output format: [X-Priority: <pValue> (<pName>)]
>        priorityValueString.AppendLiteral(" (");
>        priorityValueString += priorityName;

Append?
Attachment #8911973 - Flags: review?(acelists) → review+
Oops, there were five that should have been converted to Append().
Attachment #8911957 - Attachment is obsolete: true
Attachment #8911996 - Flags: review+
removed five hunks that went into part 1.

(In reply to :aceman from comment #16)
> What about AssignLiteral here?
Done.

> >        priorityValueString += priorityName;
> Append?
I don't know how + is mapped, most likely onto Append(), no? We have many concatenations with + in the system.
Attachment #8911973 - Attachment is obsolete: true
Attachment #8912005 - Flags: review+
(In reply to Jorg K (GMT+2) from comment #18)
> > >        priorityValueString += priorityName;
> > Append?
> I don't know how + is mapped, most likely onto Append(), no? We have many
> concatenations with + in the system.

Sure, I do not propose to change it everywhere. This single occurrence just appeared nicer to me in the row of append calls that would be created.
Fixed wrong single character assign/appends.
Attachment #8911964 - Attachment is obsolete: true
Attachment #8912035 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fdc9f3574d6e
Replace [Assign|Append|Insert|Equals]("") with [Assign|Append|Insert|Equals]Literal(""). r=aceman
https://hg.mozilla.org/comm-central/rev/337c6fe6e0e7
Replace [Assign|Append|Insert|Equals](NS_LITERAL_CSTRING("")) with [Assign|Append|Insert|Equals]Literal(""). r=aceman
https://hg.mozilla.org/comm-central/rev/a921d6d8617f
Replace [Assign|Append|Insert|Equals](NS_LITERAL_STRING("")) with [Assign|Append|Insert|Equals]Literal(u""). r=aceman
https://hg.mozilla.org/comm-central/rev/15ac102a32ac
Use Assign('') and Append('') for single character literals. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
What about cases like this:
https://dxr.mozilla.org/comm-central/rev/eb81a4091d3cd18a78ba26caa8b62cd2945aa322/mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp#250

#define NEW_FOLDER "seamonkey"
newSeamonkeyData->Append(NS_LITERAL_STRING(NEW_FOLDER));

I assume the sed expression didn't match these (missing "). After the current set of patches is picked up by DXR,
you may want to search for Assing/Append(NS_LITERAL_ , there aren't too many occurrences left.
Thanks for the comment. I can do
  #define NEW_FOLDER u"seamonkey"
  newSeamonkeyData->AppendLiteral(NEW_FOLDER);
but then I'd have to check the other uses of NEW_FOLDER.
(In reply to :aceman from comment #22)
> #define NEW_FOLDER "seamonkey"
> newSeamonkeyData->Append(NS_LITERAL_STRING(NEW_FOLDER));
Note: nsCOMPtr<nsIFile> newSeamonkeyData; - not a string!!
Notes:
Remaining "Append(NS_LITERAL_STRING(" are for nsIFile not strings.
SUMMARY_SUFFIX and FOLDER_SUFFIX are used as char16_t apart from one exception. See bug 870698 comment #16:
  "there is no EqualLiteral() function overloaded for char16_t string literals."
Attachment #8912330 - Flags: review?(acelists)
Comment on attachment 8912330 [details] [diff] [review]
1402750-append.patch (v1)

Review of attachment 8912330 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/public/msgCore.h
@@ +29,5 @@
>  /*
>   * The suffix we use for folder subdirectories.
>   */
> +#define FOLDER_SUFFIX u".sbd"
> +#define FOLDER_SUFFIX8 ".sbd"

Would it work to do '#define FOLDER_SUFFIX u(FOLDER_SUFFIX8)' or similar?
Attachment #8912330 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/591d9466d703
Replace Append(NS_LITERAL_STRING("")) with AppendLiteral(u""). r=aceman
(In reply to :aceman from comment #26)
> > +#define FOLDER_SUFFIX u".sbd"
> > +#define FOLDER_SUFFIX8 ".sbd"
> 
> Would it work to do '#define FOLDER_SUFFIX u(FOLDER_SUFFIX8)' or similar?

What's the answer to this? :)
The answer is that I don't understand what you're trying to achieve. Protect against a genius programmer who will changed one define and not the other and a reviewer who won't notice?

Something like
#define FOLDER_SUFFIX8 ".sbd"
#define FOLDER_SUFFIX u FOLDER_SUFFIX8
might work, but I don't see the benefit, so I didn't try.

The C pre-processor takes care of this so IMHO your suggestion will evaluate to u(".sbd") and mine to u ".sbd", maybe the latter would work, but I doubt it.

Since I really don't have any other problems right now, I tried it just for you and I get:
nsMsgDBFolder.cpp(3883): error C2065: 'u': undeclared identifier.

There might be a trick to glue the 'u' to the evaluation of FOLDER_SUFFIX8, but I don't know it.
The ## operator does the glueing, but u##FOLDER_SUFFIX8 gives uFOLDER_SUFFIX8. You could possibly do something like in
https://en.wikipedia.org/wiki/C_preprocessor#Token_concatenation
but anything you do will exceed in complexity simply writing the string twice.

Should I research this further? :-(
Depends on: 1404003
You need to log in before you can comment on or make changes to this bug.