Move mailnews/base/util to frozen linkage

RESOLVED FIXED

Status

defect
RESOLVED FIXED
12 years ago
a month ago

People

(Reporter: prasad, Assigned: prasad)

Tracking

(Depends on 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

12 years ago
mailnews/core/util needs to be moved to frozen linkage.  It needs some changes in nsIHTMLToTextSink.h and should try getting IsUTF8 into frozen linkage.
(Assignee)

Updated

12 years ago
Depends on: 407297
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Depends on: 418981
(Assignee)

Comment 1

11 years ago
This patch moves most of the mailnews/base/util to frozen linkage.  One known part left out is the nsHashPropertyBag that is used by nsMsgTxn.

Will see if rewriting nsMsgTxn is the best way to avoid use of nsHashPropertyBag.
Attachment #304963 - Flags: review?(bugzilla)
(Assignee)

Comment 2

11 years ago
The above patch depends on the patch attached to bug 418981
(Assignee)

Comment 3

11 years ago
Attachment #304963 - Attachment is obsolete: true
Attachment #305018 - Flags: review?(bugzilla)
Attachment #304963 - Flags: review?(bugzilla)
(Assignee)

Comment 4

11 years ago
Comment on attachment 305018 [details] [diff] [review]
Fixes a silly error with the previous patch

There are too many problems with the patch, so obsoleting it.  On a sidenote I am moving implementation of nsIWritablePropertyBag from nsHashPropertyBag to nsMsgTxn. Any comments on this?
Attachment #305018 - Attachment is obsolete: true
Attachment #305018 - Flags: review?(bugzilla)
(Assignee)

Comment 5

11 years ago
The patch migrates base/util to frozen linkage.  Also, updates other parts of mailnews that get effected because of the changes here.
Attachment #305697 - Flags: review?(bugzilla)
(Assignee)

Updated

11 years ago
Depends on: 419584
(Assignee)

Comment 6

11 years ago
Not sure if it makes sense here, but this is the linking command line that did the linking:

c++  -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions -fPIC -shared -Wl,-z,defs -Wl,-h,libmsgbaseutil.so -o libmsgbaseutil.so  nsMsgLineBuffer.o nsMsgDBFolder.o nsUInt32Array.o nsMsgKeySet.o nsMsgKeyArray.o nsMsgIdentity.o nsMsgIncomingServer.o nsMsgUtils.o nsMsgProtocol.o nsMsgMailNewsUrl.o nsMsgTxn.o nsMsgI18N.o nsAdapterEnumerator.o nsImapMoveCoalescer.o nsMsgFileStream.o     -lpthread   -Wl,-rpath-link,../../../dist/bin  -L../../../dist/bin -L../../../dist/lib ../../../rdf/util/src/librdfutil_external_s.a ../../../dist/lib/libunicharutil_external_s.a -L../../../dist/bin -lxpcomglue_s -lxpcom -lxpcom_core  -L../../../dist/bin -L../../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl  -ldl -lm
Comment on attachment 305697 [details] [diff] [review]
Migrates mailnews/base/util to frozen linkage

+#include <stdlib.h>
+#include <string.h>
+#include <ctype.h>
 #include "nscore.h"
-#include "nsCRT.h"
+#include "plstr.h"
+#include "nsCRTGlue.h"

Do we really need all of these in msgCore.h? How bad is it if we drop some?

   // convert from PRUnichar* to char* due to not having Rename(PRUnichar*)
   // function in nsIFileSpec
   nsAutoString safeName(aNewName);
   NS_MsgHashIfNecessary(safeName);
-  nsCAutoString newDiskName;
-  if (NS_FAILED(NS_CopyUnicodeToNative(safeName, newDiskName)))
-    return NS_ERROR_FAILURE;
+  nsCAutoString newDiskName = NS_ConvertUTF16toUTF8(safeName);

Reading the comment, I think we can make an improvement here. We should no longer need to convert to an nsC*String as nsIFile (as it is now) has MoveTo (for nsString) and MoveToNative (for nsCString), so I think you should be safe to drop the conversion and change MoveToNative to MoveTo.

+#ifdef MOZILLA_INTERNAL_API
   *_retval = key1.Equals(key2,nsCaseInsensitiveCStringComparator());
+#else
+  *_retval = key1.Equals(key2,CaseInsensitiveCompare);
+#endif

nit: add a space after the comma please.

+  const char *lineEnding = (useCanonicalEnding) ? CRLF: MSG_LINEBREAK;

nit: missing space

-    nsCAutoString native;
-    nsAutoString roundTripped;
-    NS_CopyUnicodeToNative(str, native);
-    NS_CopyNativeToUnicode(native, roundTripped);
-    return str.Equals(roundTripped);
+    nsCAutoString native = NS_ConvertUTF16toUTF8((nsAString&)str);
+    nsAutoString roundTripped = NS_ConvertUTF8toUTF16((nsACString&)native);
+    return str.Equals((nsAString&)roundTripped);

I'm not sure about this, NS_CopyNativeToUnicode doesn't do the same on each platform: http://mxr.mozilla.org/seamonkey/source/xpcom/io/nsNativeCharsetUtils.cpp

I'm stopping here for tonight, I still need to look at nsMsgTxn.{h,cpp}, and rest of nsMsgUtils.cpp and everything after.
Comment on attachment 305697 [details] [diff] [review]
Migrates mailnews/base/util to frozen linkage

+nsresult nsMsgTxn::Init()
+{
+  if (!mPropertyHash.Init())
+    return NS_ERROR_OUT_OF_MEMORY;
+  return NS_OK;

Should be:

return mPropertyHash.Init() ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

+NS_IMETHODIMP nsMsgTxn::GetProperty(const nsAString& name, nsIVariant* * _retval)
+{
+  PRBool isFound = mPropertyHash.Get(name, _retval);
+  if (!isFound)
+    return NS_ERROR_FAILURE;
+
+  return NS_OK;

Similarly, return mPropertyHash.Get(name, _retval) ? NS_OK : NS_ERROR_FAILURE;

+NS_IMETHODIMP nsMsgTxn::SetProperty(const nsAString& name, nsIVariant *value)

ditto

+NS_IMETHODIMP nsMsgTxn::DeleteProperty(const nsAString& name)
+{
+  PRBool isFound = mPropertyHash.Get(name, nsnull);
+  if (!isFound)
+      return NS_ERROR_FAILURE;

Drop the isFound variable, and just wrap the get in the if.

Some places in this file are using 4 space indentation rather than 2.

+NS_IMETHODIMP nsMsgTxn::GetEnumerator(nsISimpleEnumerator* *_retval)
+{
+  nsresult rv;
+  nsCOMPtr<nsIMutableArray> propertyArray = 
+      do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);

Using nsCOMArray<nsIProperty> would be better than the nsIMutableArray here.

+  return propertyArray->Enumerate(_retval);
+
+  // return NS_NewArrayEnumerator(_retval, propertyArray);

You can also use NS_NewArrayEnumerator with nsCOMArray.

+IMPL_GETSETPROPERTY_AS(Bool, PRBool)
+
+NS_IMETHODIMP nsMsgTxn::GetPropertyAsAString(const nsAString & prop, 
+                                             nsAString & _retval)

I don't see why GetPropertyAsA(C)String can't use the macro.

+    NS_DECL_ISUPPORTS
+
+    NS_DECL_NSIPROPERTYBAG
+
+    NS_DECL_NSIPROPERTYBAG2
+
+    NS_DECL_NSIWRITABLEPROPERTYBAG
+
+    NS_DECL_NSIWRITABLEPROPERTYBAG2

No need for the blank lines here.

Note to self: nsMsgUtils.cpp and onwards to do.
Comment on attachment 305697 [details] [diff] [review]
Migrates mailnews/base/util to frozen linkage

-  if(pos == kNotFound)
+  if(pos == -1)

nit: add space after the if please, occurs in several places in the patch.

-    NS_CopyUnicodeToNative(str, native);
-    NS_CopyNativeToUnicode(native, roundTripped);
-    return str.Equals(roundTripped);
+    nsCAutoString native = NS_ConvertUTF16toUTF8((nsAString&)str);
+    nsAutoString roundTripped = NS_ConvertUTF8toUTF16((nsACString&)native);

Like I mentioned in comment 7 I'm not sure this is right on all platforms (several places in this patch).

+PRInt32
+FindCharInSet(nsCAutoString aString, const char* aChars) 

The first param should be const nsCString &aString, similar instances in the other FindCharInSet calls - pass the strings by reference, and you don't need the Auto bit either in params.

+  for (int i=0; i<len; i++) {

nit: should be "for (int i = 0; i < len; i++)" i.e. with spaces, occurs in a couple of places at least.

+#ifndef MOZILLA_INTERNAL_API
+NS_MSG_BASE void MsgCompressWhitespace(nsACString& aString);
+#endif

May as well just define and use it now.
Attachment #305697 - Flags: review?(bugzilla) → review-
(Assignee)

Updated

11 years ago
Summary: Move mailnews/core/util to frozen linkage → Move mailnews/base/util to frozen linkage
(Assignee)

Comment 10

11 years ago
Updated the patch based on the review comments.

The only function left out without migration is the NS_CopyNativeToUnicode, due to which linking will fail. I am trying to find alternative ways to achieve the same functionality.

A large part of code in MsgCompressWhitespace is under an #ifdef because the frozen API part uses code that is specific to frozen API and the internal API does not need that code :)

Linking will not succeed unless the NS_CopyNativeToUnicode is also migrated.
Attachment #305697 - Attachment is obsolete: true
Attachment #317853 - Flags: review?(bugzilla)
(In reply to comment #10)
> Created an attachment (id=317853) [details]
> Migrate base/util to frozen linkage - v2

I tried this on a clean tree (yep double checked that) and got:

/Users/moztest/mozilla/main/mozilla/mailnews/base/src/nsMessenger.cpp: In function 'int CompareAttachmentPartId(const char*, const char*)':
/Users/moztest/mozilla/main/mozilla/mailnews/base/src/nsMessenger.cpp:2427: error: 'NS_IS_DIGIT' was not declared in this scope
/Users/moztest/mozilla/main/mozilla/mailnews/base/src/nsMessenger.cpp:2428: error: 'NS_IS_DIGIT' was not declared in this scope
(Assignee)

Comment 12

11 years ago
This time tested both the debug and the release builds :)
Attachment #317853 - Attachment is obsolete: true
Attachment #320102 - Flags: review?(bugzilla)
Attachment #317853 - Flags: review?(bugzilla)
Comment on attachment 320102 [details] [diff] [review]
V2, Fixed the debug build bustage (comment 11)

I'm happy with this now, so r=me, but please get Neil to sr this and confirm he's happy with the NS_IS_SPACE -> isspace issue.
Attachment #320102 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 14

11 years ago
Comment on attachment 320102 [details] [diff] [review]
V2, Fixed the debug build bustage (comment 11)

Neil, can you super review this patch.  Please note that the |NS_IS_SPACE| to |isspace| that Standard8 was pointing out is in this patch.
Attachment #320102 - Flags: superreview?(neil)
Comment on attachment 320102 [details] [diff] [review]
V2, Fixed the debug build bustage (comment 11)

>+        domain = StringTail(authorEmailAddress, authorEmailAddress.Length() - atPos - 1);
domain = Substring(authorEmailAddress, atPos + 1);

>+#ifdef MOZILLA_INTERNAL_API
>     if (escapedName.LowerCaseEqualsLiteral("inbox"))
The external API sucks...

>-                                      formatStrings, 2, getter_Copies(aResult));
>+                                      formatStrings, 2, getter_Copies((nsString&)aResult));
Wow, how did that ever compile before? Please change this to use a temporary string.

>+      extension = StringTail(mAttachmentFileName,
>+                             mAttachmentFileName.Length() -
>+                             (pos + 1) /* skip the '.' */);
extension = Substring(mAttachmentFileName, pos + 1 /* skip the '.' */);

>   else if (PL_strncasecmp(uri, "mailbox", 7) == 0)
>   {
>     nsCOMPtr<nsIMailboxUrl> mailboxUrl = do_CreateInstance(kCMailboxUrl, &rv);
>     if (NS_SUCCEEDED(rv) && mailboxUrl)
>       rv = mailboxUrl->QueryInterface(NS_GET_IID(nsIURI),
>       (void**) aUrl);
>   }
>-  else if (PL_strncasecmp(uri, "news", 4) == 0)
>+  else if (strncasecmp(uri, "news", 4) == 0)
Why here and not 7 lines earlier?

>+    if (FindCharInSet(str, ILLEGAL_FOLDER_CHARS_AS_FIRST_LETTER) == 0)
>+      illegalCharacterIndex = 0;
>+    else if (RFindCharInSet(str, ILLEGAL_FOLDER_CHARS_AS_LAST_LETTER) == lastIndex)
>+      illegalCharacterIndex = lastIndex;
>+    else
>+      illegalCharacterIndex = -1;
Not your fault but I feel sure there must be an easier way of doing this.

>+    checkString.Append(NS_LITERAL_CSTRING(","));
checkString.Append(',');

>+  while (s < s_end && isspace(*s))
+        while (s2 < (s_end - 2) && isdigit(*s2))
*s/*s2 are char but isspace/isdigit require unsigned char (or EOF)... unless you can prove that these are ASCII?

>+  nsCString(aString).CompressWhitespace(PR_TRUE, PR_TRUE);
This doesn't change your original string, but as it's not exposed to IDL you can declare nsCString& aString instead.

>+    if (!NS_IsAsciiWhitespace(*cur))
NS_IsAsciiWhitespace takes a PRUnichar, which is a waste. Carefully use isspace instead?

>+    char *wend;
>+    for (wend = cur + 1; wend < end && NS_IsAsciiWhitespace(*wend); ++wend) {
>+      // nothing to do but loop
>+    }
Maybe this would be clearer as a while loop? (++wend in the body)
(Also as you trimmed whitespace already then wend should never reach end...)

>+    if (wend == cur + 1)
>+      continue;
Maybe this would be clearer as a whitespace test before the loop?

>+    // move everything forwards a bit
>+    for (char *m = cur + 1; m < end; ++m) {
>+      *m = *(m + wlen);
The internal API routines take a slightly different approach here, but I guess you'd need to know a typical use case before deciding on efficiency.

>+  // re-terminate
>+  *end = '\0';
I hope SetLength already does that!

There were a bunch of files at the end of the patch where you simply switched IS_SPACE for isspace; can you be sure that they are only passed ASCII characters?
Attachment #320102 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 16

11 years ago
updated as per Neil's comments.
Will soon post a comment on the actual changes done.
Attachment #320102 - Attachment is obsolete: true
Attachment #321315 - Flags: superreview?(neil)
(Assignee)

Comment 17

11 years ago
> 
> >+    if (FindCharInSet(str, ILLEGAL_FOLDER_CHARS_AS_FIRST_LETTER) == 0)
> >+      illegalCharacterIndex = 0;
> >+    else if (RFindCharInSet(str, ILLEGAL_FOLDER_CHARS_AS_LAST_LETTER) == lastIndex)
> >+      illegalCharacterIndex = lastIndex;
> >+    else
> >+      illegalCharacterIndex = -1;
> Not your fault but I feel sure there must be an easier way of doing this.

The cleaner way:
if (NS_LITERAL_STRING(ILLEGAL_FOLDER_CHARS_AS_FIRST_LETTER).FindChar(str[0]) != -1) ... etc; Right?  We do get a small (very small) performance gain too.

These functions will need changes when linking with frozen linkage.  Till then we can probably leave them as they were copied.


> >+  while (s < s_end && isspace(*s))
> +        while (s2 < (s_end - 2) && isdigit(*s2))
> *s/*s2 are char but isspace/isdigit require unsigned char (or EOF)... unless
> you can prove that these are ASCII?
> 

> 
> >+    // move everything forwards a bit
> >+    for (char *m = cur + 1; m < end; ++m) {
> >+      *m = *(m + wlen);
> The internal API routines take a slightly different approach here, but I guess
> you'd need to know a typical use case before deciding on efficiency.
> 

In frozen linkage I think all strings are continuous, so this would be good.  From the other side, it may not be a wise idea to manipulate the strings in other means when we are using writing iterators.


> 
> There were a bunch of files at the end of the patch where you simply switched
> IS_SPACE for isspace; can you be sure that they are only passed ASCII
> characters?
> 

Since IS_SPACE is a macro in mailnews, I just changed back all the IS_SPACE changes.  Instead, I moved the same macro that was used for NS_IS_SPACE to mailnews.
Comment on attachment 321315 [details] [diff] [review]
Updated as per Neil's comments.

>+#define IS_SPACE(VAL) \
>+  (((((intn)(VAL)) & 0x7f) == ((intn)(VAL))) && isspace((intn)(VAL)))
I thought you wanted to change to isspace((unsigned char)VAL) but this works for me too.

>+    if (!IS_SPACE(*(cur + 1)))
>+      continue;
>+
>+    // Loop through the white space
>+    char *wend = cur + 1;
>+    while (IS_SPACE(*wend)) 
>+      ++wend;
Nit: You can probably start at cur + 2; cur + 1 is now known to be a space!
Attachment #321315 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 19

11 years ago
I did not want to change the IS_SPACE just for the sake of changing it.  The old one looked perfectly fine and more importantly does not change the way the code currently works :)

Can you commit this patch for me? r=Standard8, sr=Neil
Attachment #321315 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Checking in mailnews/base/public/msgCore.h;
/cvsroot/mozilla/mailnews/base/public/msgCore.h,v  <--  msgCore.h
new revision: 1.67; previous revision: 1.66
done
Checking in mailnews/base/src/nsMessenger.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMessenger.cpp,v  <--  nsMessenger.cpp
new revision: 1.387; previous revision: 1.386
done
Checking in mailnews/base/src/nsMsgPurgeService.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgPurgeService.cpp,v  <--  nsMsgPurgeService.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in mailnews/base/util/nsImapMoveCoalescer.cpp;
/cvsroot/mozilla/mailnews/base/util/nsImapMoveCoalescer.cpp,v  <--  nsImapMoveCoalescer.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.338; previous revision: 1.337
done
Checking in mailnews/base/util/nsMsgI18N.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgI18N.cpp,v  <--  nsMsgI18N.cpp
new revision: 1.107; previous revision: 1.106
done
Checking in mailnews/base/util/nsMsgIdentity.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgIdentity.cpp,v  <--  nsMsgIdentity.cpp
new revision: 1.85; previous revision: 1.84
done
Checking in mailnews/base/util/nsMsgIdentity.h;
/cvsroot/mozilla/mailnews/base/util/nsMsgIdentity.h,v  <--  nsMsgIdentity.h
new revision: 1.28; previous revision: 1.27
done
Checking in mailnews/base/util/nsMsgIncomingServer.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgIncomingServer.cpp,v  <--  nsMsgIncomingServer.cpp
new revision: 1.267; previous revision: 1.266
done
Checking in mailnews/base/util/nsMsgKeySet.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgKeySet.cpp,v  <--  nsMsgKeySet.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in mailnews/base/util/nsMsgMailNewsUrl.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgMailNewsUrl.cpp,v  <--  nsMsgMailNewsUrl.cpp
new revision: 1.122; previous revision: 1.121
done
Checking in mailnews/base/util/nsMsgProtocol.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgProtocol.cpp,v  <--  nsMsgProtocol.cpp
new revision: 1.153; previous revision: 1.152
done
Checking in mailnews/base/util/nsMsgTxn.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgTxn.cpp,v  <--  nsMsgTxn.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in mailnews/base/util/nsMsgTxn.h;
/cvsroot/mozilla/mailnews/base/util/nsMsgTxn.h,v  <--  nsMsgTxn.h
new revision: 1.12; previous revision: 1.11
done
Checking in mailnews/base/util/nsMsgUtils.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.cpp,v  <--  nsMsgUtils.cpp
new revision: 1.145; previous revision: 1.144
done
Checking in mailnews/base/util/nsMsgUtils.h;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.h,v  <--  nsMsgUtils.h
new revision: 1.71; previous revision: 1.70
done
Checking in mailnews/imap/src/nsImapIncomingServer.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapIncomingServer.cpp,v  <--  nsImapIncomingServer.cpp
new revision: 1.381; previous revision: 1.380
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.809; previous revision: 1.808
done
Checking in mailnews/mime/src/comi18n.cpp;
/cvsroot/mozilla/mailnews/mime/src/comi18n.cpp,v  <--  comi18n.cpp
new revision: 1.134; previous revision: 1.133
done
Keywords: checkin-needed
So I thought it might be a good idea to take a look at where we now fail to compile/link in mailnews/base/util:

mailnews/base/util/nsMsgDBFolder.cpp fails to compile (in debug mode) because of an NS_ASSERTION(IsUTF8...

After that the various link errors:

NS_CreateServicesFromCategory will be fixed by bug 433316.

NS_NewArrayEnumerator, NS_NewISupportsArray and (vtable for) nsQueryElementAt should be fixed by bug 394167 or its dependencies.

NS_NewAtom (do_GetAtom), NS_RegisterStaticAtoms, NS_NewPipe, NS_ErrorAccordingToNSPR, NS_CopyUnicodeToNative and NS_CopyNativeToUnicode need investigation.
Depends on: 394167, 433316
(Assignee)

Comment 22

11 years ago
My first impressions on fixing a few of the above:

NS_NewPipe can be avoided using do_CreateInstance("@mozilla.org/pipe;1").  Then calling Init, GetInputStream and GetOutputStream.

NS_CopyUnicodeToNative and NS_CopyNativeToUnicode can be avoiding by migrating from native file names to Unicode file names in a few places.  The nsILocalFile implementations call these functions where required.
(Assignee)

Updated

11 years ago
Depends on: 378601
NS_NewAtom and NS_RegisterStaticAtoms would have to be rewritten to call the atom service to create atoms manually.
Blocks: 434748
(Assignee)

Updated

11 years ago
Depends on: 435241
(Assignee)

Updated

11 years ago
Depends on: 435242
Comment on attachment 321457 [details] [diff] [review]
Migrate base/util to frozen linkage - Fixed a nit
[Checkin: Comment 20]

>+      if (!(c & 0xC0) == 0x80 || overlong && c <= olupper ||
You didn't inline UTF8traits::isInSeq correcly; the ! needs an extra set of parentheses but you're better off switching to != i.e.
if ((c & 0xC0) != 0x80 || overlong && c <= olupper ||

Comment 25

11 years ago
rs=bienvenu for #24
(Assignee)

Comment 26

11 years ago
what is the right way of getting the above correction (#24) checked in?  Should I submit a patch for this too or since it is a small change can it be checked in giving reference to bienvenu's comment (#25) above?
Product: Core → MailNews Core

Updated

11 years ago
Blocks: 471760
(In reply to comment #24)
> (From update of attachment 321457 [details] [diff] [review])
> >+      if (!(c & 0xC0) == 0x80 || overlong && c <= olupper ||
> You didn't inline UTF8traits::isInSeq correcly; the ! needs an extra set of
> parentheses but you're better off switching to != i.e.
> if ((c & 0xC0) != 0x80 || overlong && c <= olupper ||

This was fixed by checkin to cvs on version 1.146 of nsMsgUtils.cpp at 2008-05-28 09:16.
Blocks: 467015
No longer blocks: 471760
Attachment #321457 - Attachment description: Migrate base/util to frozen linkage - Fixed a nit. → Migrate base/util to frozen linkage - Fixed a nit [Checkin: Comment 20]
No longer blocks: 467015
Depends on: 525463

I think this is all done a long time ago.

Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.