Created attachment 617200 [details]
Error build log
Yesterday I've tried to build Thunderbird with Apples Clang 4.0. This produces a lot of errors in the kind of:
case value evaluates to 2147483649, which cannot be narrowed to type
'PRInt32' (aka 'int') [-Wc++11-narrowing]
case value evaluates to 2153066780, which cannot be narrowed to type 'int'
I've attached the full error log. It's build with the 10.7 SDK. I get no errors in Gecko.
Apple clang version 4.0 (tags/Apple/clang-421.0.11) (based on LLVM 3.1svn)
Nomis can you try to make patches to fix these ?
Maybe. I'm currently trying to workaround this, because I'm not sure how to fix it. But if somebody has an idea how to fix this, I can try it and make a patch.
I've asked :espindola about that and he told me that something like this can happen if build in c++11, because there are more restrictions on narrowing. If build in c++03, this is just a warning and a workaround would be to pass -Wno-c++11-narrowing. So I've tried building with -Wno-c++11-narrowing, and this indeed repressed the errors. But sadly it also produced new errors.
I have seen tons of these warnings (in Core) too when I just switched to gcc 4.7. Only that it is a warning (not error) so the build succeeds.
I am not sure we can change the values of the error constants to be less then max value of PRInt32. Maybe we should make the variables PRUint32 ('position' and 'code').
But I do not see the warning in these two files mentioned (on gcc 4.7).
As I read, Clang is a lot stricter as C++ compiler in warnings and errors than gcc, maybe thats the reason you do not see this with gcc. You mean changing the PRInt32 to PRUint32 could help?
Yes, can you try that? Of course, try to check if those 2 variables do not take negative values at some other place.
OK, it seems PRUint32 works at least for nsDirPrefs.cpp, I don't see an error there anymore (and no new error). Do you think the same will also work for nsSmtpProtocol.cpp? There are a lot of PRInt32 and I'm unsure which of them I should change.
I said the 'code' variable.
Until now I was not able to do the same for nsSmtpProtocol.cpp, it only produces me new errors. I will still try to get this work with PRUint32. In the meantime I will attach the WIP patch I currently have for nsDirPrefs.cpp. I also was able to make a workaround patch by passing -Wno-c++11-narrowing, I will also attach this.
Created attachment 617991 [details] [diff] [review]
WIP - change PRInt32 to PRUint32
This fixes the errors in nsDirPrefs.cpp for me.
Created attachment 617995 [details] [diff] [review]
Workaround - pass -Wno-c++11-narrowing and port Bug 727145
This patch lets me build Thunderbird with Apples Clang 4.0. It passes -Wno-c++11-narrowing to the compiler and ports Bug 727145. The port is neccessary to prevent errors like:
/In file included from
In file included from ../../../mozilla/dist/include/nsIScriptContext.h:46:
In file included from ../../../mozilla/dist/include/jsfriendapi.h:43:
In file included from ../../../mozilla/dist/include/jsclass.h:48:
../../../mozilla/dist/include/jsapi.h:2149:1: error: 'JS_GetNaNValue' has
C-linkage specified, but returns user-defined type 'jsval'
(aka 'JS::Value') which is incompatible with C
(:espíndola pointed me to this)
Nomis are these patches complete ?
The patch for nsDirPrefs.cpp is complete, but I was not able to fix the error for nsSmtpProtocol.cpp. The workaround patch works for building TB, but TB is build with this patch a bit crashy (don't know why). I currently switched back to Apples Clang 3.1
Created attachment 633846 [details] [diff] [review]
With this patch and the patch from Bug 764469 I can build TB with Apples Clang 4.0. And it also seems to not interfere with older versions of Apples Clang. Tested with Apples Clang 4.0 on 10.8 and Apples Clang 3.1 on 10.7.4.
Comment on attachment 633846 [details] [diff] [review]
I won't get to this anytime soon, and I'm primarily a windows dev and don't have clang installed.
(In reply to Nomis101 from comment #0)
> case value evaluates to 2147483649, which cannot be narrowed to type
> 'PRInt32' (aka 'int') [-Wc++11-narrowing]
> case DIR_POS_DELETE:
What happens if you simply replace DIR_POS_DELETE with -2 and DIR_POS_APPEND with -1 ? I believe that's what is trying to be represented, and I think clang is evaluating them as unsigned with more than 32 bits, when we do really want them as a 32 bit signed.
> 12: error:
> case value evaluates to 2153066780, which cannot be narrowed to type
> case NS_ERROR_SMTP_GREETING:
The problem here is that NS_ERROR_SMTP_GREETING (and co) is actually a macro, and ends up using NS_ERROR_GENERATE_FAILURE, which as you'll see is a nsresult type. nsresult is an unsigned int, so this code is assuming its possible to get those values squeezed in. Generally because both are 32 bits, there's no long-term errors, but obviously compilers are being upgraded to warn against these issues.
I think the correct solution is to change the "int code" parameter to nsExplainErrorDetails to "nsresult code" and adapt any callers as necessary. Then they will all use the correct types.
Comment on attachment 633846 [details] [diff] [review]
I don't think this is quite right to do based on my comments above, if we did do it I'd want to see it in mozilla-central first.
(In reply to Mark Banner (:standard8) from comment #16)
> (In reply to Nomis101 from comment #0)
> > /Volumes/Developer/comm-central/mailnews/addrbook/src/nsDirPrefs.cpp:404:9:
> > error:
> > case value evaluates to 2147483649, which cannot be narrowed to type
> > 'PRInt32' (aka 'int') [-Wc++11-narrowing]
> > case DIR_POS_DELETE:
> > ^
> What happens if you simply replace DIR_POS_DELETE with -2 and DIR_POS_APPEND
> with -1 ?
Yes, this works.
> I think the correct solution is to change the "int code" parameter to
> nsExplainErrorDetails to "nsresult code" and adapt any callers as necessary.
> Then they will all use the correct types.
Hm, currently I'm not sure how to do that. I try to find out how to fix this this way.
Created attachment 637976 [details] [diff] [review]
Do you mean like this? This builds now for me and the build seems stable and doesn't crash. This was so surprisingly easy...
Comment on attachment 637976 [details] [diff] [review]
>-#define DIR_POS_APPEND 0x80000000
>-#define DIR_POS_DELETE 0x80000001
I think I'd prefer these to just be changed to const PRInt32 ...
>-nsresult nsExplainErrorDetails(nsISmtpUrl * aSmtpUrl, int code, ...)
>+nsresult nsExplainErrorDetails(nsISmtpUrl * aSmtpUrl, nsresult code, ...)
Yep its pretty much that simple, treat the code as a proper nsresult ;-) For consistencies sake, it'd be good to change the "int errorcode" to "nsresult errorcode" here as well:
as that's calling nsExplainErrorDetails that you're altering. All the other instances where nsExplainErrorDetails is called use the constants, and hence don't need changing.
Created attachment 638049 [details] [diff] [review]
Like this? v2
OK, this one also builds (with clang).
Created attachment 638099 [details] [diff] [review]
Now with a proper header :)
My, what a descriptive commit message. Please provide something a bit more...descriptive...next time?