Closed Bug 1294260 Opened 8 years ago Closed 8 years ago

Fix Mailnews compiler warnings after they got upgraded to errors in bug 1292463 (Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure)

Categories

(Thunderbird :: General, defect)

defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 51.0

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

Attachments

(6 files, 15 obsolete files)

989 bytes, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1.36 KB, patch
aceman
: review+
Details | Diff | Splinter Review
1.31 KB, patch
aceman
: review+
Details | Diff | Splinter Review
105.68 KB, patch
aleth
: review+
Details | Diff | Splinter Review
199.32 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
22.55 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Looks like warnings got upgraded to errors, so we need to fix some warnings or disable the "upgrade".

Errors can be seen here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=943e97688ec297a2d710728b84ae7b7fdcd33ef3
The only real issue here is ldap/. Is there a convenient way to turn off some compiler flags for ldap only? Not sure if it's set up as a subconfigure.
Flags: needinfo?(Pidgeot18)
Attached patch check NS_NewISupportsArray (obsolete) — Splinter Review
So this brings linux in line with the other platforms.

The LDAP failure still remains. I can look at that later.
Attachment #8779916 - Flags: review?(jorgk)
Attachment #8779916 - Flags: review?(jorgk) → review+
Comment on attachment 8779916 [details] [diff] [review]
check NS_NewISupportsArray

Hmm, funny commit message ;-)
Bug 1294260 - check return code of NS_NewISupportsArray() - bustage fix. r=jorgk,aleth a=bustage-fix CLOSED TREE
Attachment #8779916 - Flags: review+
Must be too late :)
Strangely, after applying the patch locally and updating m-c I do not get the LDAP compile failures seen on try server. Instead, I get an internal linker error (in ld). I do not know yet if it is a manifestation of the same problem, but I doubt that.
(In reply to :aceman from comment #5)
> Strangely, after applying the patch locally and updating m-c I do not get
> the LDAP compile failures seen on try server. Instead, I get an internal
> linker error (in ld). I do not know yet if it is a manifestation of the same
> problem, but I doubt that.

You probably have to set
ac_add_options --enable-warnings-as-errors
in your mozconfig as it's only turned on by default when MOZ_AUTOMATION is set.
I can't do that if that would apply to whole tree. There are a million of warnings in m-c and some in c-c. I can try removing -Wextra but I do not remember it being warning free even before that.
(In reply to :aceman from comment #7)
> I can't do that if that would apply to whole tree. There are a million of
> warnings in m-c and some in c-c. I can try removing -Wextra but I do not
> remember it being warning free even before that.
That's what I don't understand either. If anyone were to turn warnings to errors, then M-C just would not compile any more. So my questions are:
What is MOZ_AUTOMATION? How do they manage to compile M-C? (Maybe they do *not* convert warnings to errors there.) And why does C-C code get treated differently? I'm not sure, but we have more warnings than in LDAP, so we can't possibly fix all warnings now. See bug 1232306 which never went anywhere apart from fixing warnings in one particular file (ImportCharSet.cpp).
(In reply to :aceman from comment #7)
> I can't do that if that would apply to whole tree. There are a million of
> warnings in m-c and some in c-c. I can try removing -Wextra but I do not
> remember it being warning free even before that.

Yes, I think I'm not understanding something about what Bug 1292463 actually did.
--enable-warnings-as-errors was enabled in the m-c mozconfigs in bug 703121 and modified in bug 1198334. The description there gives some information and explains why it's restricted to automation builds (MOZ_AUTOMATION is set for the buildbot jobs, but not locally).
Flags: needinfo?(Pidgeot18)
So to summarize, there's been a steady effort under way for years to treat warnings as errors whereever possible in automation builds. It's now the default unless ALLOW_COMPILER_WARNINGS is explicitly set for a subdirectory (generally only the case for imported libraries) or for a particular build config. You may see a ton of warnings in your local builds as your config is slightly different, but if you saw the same warning on try your build would fail.
Looks like it's now failing in mailnews/ (comment 11)?
Flags: needinfo?(acelists)
We are seeing these warnings-as-errors problems on c-c now because c-c mozconfigs never followed the warnings-as-errors defaults used by m-c (as described in comment 12) but now they also apply to c-c.
(In reply to aleth [:aleth] from comment #13)
> Looks like it's now failing in mailnews/ (comment 11)?
Looks like these are somewhat expected:
https://dxr.mozilla.org/comm-central/source/obj-x86_64-pc-linux-gnu/.mozbuild/warnings.json#1675
I'm not sure what this file does.

I'd say, either allow compiler warnings there too or fix them ;-)
This is caused by declaring them here:
https://dxr.mozilla.org/comm-central/source/mailnews/import/src/nsImportAddressBooks.cpp#47
Maybe try changing
NS_IMETHOD BeginImport(nsISupportsString *successLog, nsISupportsString *errorLog, bool *_retval) ;
to include the class name or move those declarations to the class in the .h file.
Comment on attachment 8780069 [details] [diff] [review]
Part 1: Allow compiler warnings in ldap [landed in comment #74]

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

Sure, if it works. I looked at the code, it's a horrible mix of unsigned and signed via their own types. Not fun to fix at all.
Attachment #8780069 - Flags: review?(acelists) → review+
Yes, I also agree with ignoring warnings in LDAP.

But these missing overrides should be fixed

I'll update my patch with further fixes for the new issues.
I wonder why they only crop up now, when we saw this NS_NewISupportsArray occurrence yesterday but the compiler didn't flag it.

/builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/base/search/src/nsMsgFilter.cpp:175:51: error: ignoring return value of 'nsresult NS_NewISupportsArray(nsISupportsArray**)', declared with attribute warn_unused_result [-Werror=unused-result]

I wonder how to fix this one:

/builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/local/src/nsMsgLocalStoreUtils.cpp:305:68: error: format '%lld' expects argument of type 'long long int', but argument 2 has type 'int64_t {aka long int}' [-Werror=format=]
Flags: needinfo?(acelists)
(In reply to Jorg K (GMT+2, PTO during summer) from comment #16)
> I'd say, either allow compiler warnings there too or fix them ;-)
> This is caused by declaring them here:
> https://dxr.mozilla.org/comm-central/source/mailnews/import/src/
> nsImportAddressBooks.cpp#47
> Maybe try changing
> NS_IMETHOD BeginImport(nsISupportsString *successLog, nsISupportsString
> *errorLog, bool *_retval) ;
> to include the class name or move those declarations to the class in the .h
> file.

I think they override a generic function in the parent class in another file, so moving just to the child class in .h should not work.

I think this just needs to add the 'override' keyword.

https://dxr.mozilla.org/comm-central/source/mozilla/dom/html/UndoManager.cpp#103
(In reply to Jorg K (GMT+2, PTO during summer) from comment #16)
> (In reply to aleth [:aleth] from comment #13)
> > Looks like it's now failing in mailnews/ (comment 11)?
> Looks like these are somewhat expected:
> https://dxr.mozilla.org/comm-central/source/obj-x86_64-pc-linux-gnu/.
> mozbuild/warnings.json#1675
> I'm not sure what this file does.

You're looking at part of the output of a build - a json file containing the warnings encountered. That's an objdir. Not sure why dxr is indexing it.

> I'd say, either allow compiler warnings there too or fix them ;-)

It makes some sense to allow warnings in ldap as its an external library, and it might be better to replace it rather than to fix it. For mailnews/ imho it would make more sense to fix the warnings in line with m-c policy. Looking at the current failures it seems to me they are not unexpected (eg. one we were wondering yesterday on #maildev about why we weren't seeing it ;) ).

But if there are too many warnings we could temporarily allow them.
Ah, looks like I midaired with aceman's comments, sorry.
(In reply to :aceman from comment #18)
> I wonder how to fix this one:
> 
> /builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/local/src/
> nsMsgLocalStoreUtils.cpp:305:68: error: format '%lld' expects argument of
> type 'long long int', but argument 2 has type 'int64_t {aka long int}'
> [-Werror=format=]

It's in a debug-only printf: https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgLocalStoreUtils.cpp?q=nsMsgLocalStoreUtils.cpp&redirect_type=direct#305

Probably fixed by removing one 'l' or an explicit cast?
(In reply to aleth [:aleth] from comment #20)
> It makes some sense to allow warnings in ldap as its an external library,
> and it might be better to replace it rather than to fix it.

Yes.

> For mailnews/
> imho it would make more sense to fix the warnings in line with m-c policy.
> Looking at the current failures it seems to me they are not unexpected (eg.
> one we were wondering yesterday on #maildev about why we weren't seeing it
> ;) ).

Yes :)

> But if there are too many warnings we could temporarily allow them.

Is there a possibility to disable warning classes? E.g. we can't yet fix the warning about some NS_ERROR values not being in nsresult enumeration. But some others we can fix, there even shouldn't be many, I cleaned them up once in a while.
(In reply to aleth [:aleth] from comment #22)
> > /builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/local/src/
> > nsMsgLocalStoreUtils.cpp:305:68: error: format '%lld' expects argument of
> > type 'long long int', but argument 2 has type 'int64_t {aka long int}'
> > [-Werror=format=]
> 
> Probably fixed by removing one 'l' or an explicit cast?

http://stackoverflow.com/questions/9225567/how-to-print-a-int64-t-type-in-c
Now I wonder which solution is more ugly :)

printf("%" PRId64 "\n", my_int);
printf("%lld", (long long)my_int);
(In reply to :aceman from comment #23)
> > But if there are too many warnings we could temporarily allow them.
> 
> Is there a possibility to disable warning classes? E.g. we can't yet fix the
> warning about some NS_ERROR values not being in nsresult enumeration. But
> some others we can fix, there even shouldn't be many, I cleaned them up once
> in a while.

In principle you can do that in configure, *if* there is a way to override mozilla/build/moz.configure/warnings.configure for TB builds. But not per directory.

What's the problem with that enumeration error? Is that a known bug?
(In reply to aleth [:aleth] from comment #25)
> What's the problem with that enumeration error? Is that a known bug?

Yes, see bug 783526 comment 15.
(In reply to :aceman from comment #26)
> (In reply to aleth [:aleth] from comment #25)
> > What's the problem with that enumeration error? Is that a known bug?
> 
> Yes, see bug 783526 comment 15.

Looks like years-old technical debt coming back to bite ;)
(In reply to :aceman from comment #19)
> I think this just needs to add the 'override' keyword.
I'm aware of 'override' but I'm not sure it's the right thing to do. Where would the parent class be?
Take FindAddressBooks, that's just a method from nsIImportAddressBooks that needs to be implemented, not overridden.
For example, Becky just has:
NS_IMETHODIMP
nsBeckyAddressBooks::FindAddressBooks(nsIFile *aLocation,
                                      nsIArray **_retval)

Many others have two:
mailnews/import/vcard/src/nsVCardImport.cpp
NS_IMETHOD FindAddressBooks(nsIFile *location, nsIArray **_retval); <=== Why?
NS_IMETHODIMP ImportVCardAddressImpl::FindAddressBooks(

See:
https://dxr.mozilla.org/comm-central/search?q=FindAddressBooks&redirect=false

I'm not sure what the first one does.
The message is like this:

mailnews/import/src/nsImportAddressBooks.cpp:47:14: error: 'GetData' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  NS_IMETHOD GetData(const char *dataId, nsISupports **_retval);
             ^
objdir-tb/dist/include/nsIImportGeneric.h:33:14: note: overridden virtual function is here
  NS_IMETHOD GetData(const char * dataId, nsISupports * *_retval) = 0;
             ^
So it says what overrides what. I've seen this construct in more places in mailnews. There is some generic class (e.g. a msgserver) and then the specific server types override the functions in that generic class.
Hmm, yes, I can see nsIImportGeneric.idl. But I was looking at this Mac compile error:
mailnews/import/vcard/src/nsVCardImport.cpp:56:14: error: 'FindAddressBooks' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
Maybe OE, Outlook, VCard, Text and Becky all override the generic one in nsIImportAddressBooks.idl. In this case 'override' is the thing to add. So when will it be ready ;-)
Ok, I have progressed much with the patches, results can be seen on try.

I need to take a break now, the next open issues are:
-Windows:
build/objdir-tb/mozilla-config.h(145): error C2220: warning treated as error - no 'object' file generated
build/objdir-tb/mozilla-config.h(145): warning C4005: '_WIN32_WINNT': macro redefinition
build/objdir-tb/mozilla-config.h(145): note: command-line arguments:  see previous definition of '_WIN32_WINNT'

-Linux:
In file included from db/mork/src/morkArray.cpp:17:0:
db/mork/src/morkNode.h:223:21: error: ‘virtual mork_uses morkNode::AddStrongRef(morkEnv*)’ was hidden [-Werror=overloaded-virtual]
   virtual mork_uses    AddStrongRef(morkEnv* ev);
                     ^
In file included from db/mork/src/morkEnv.h:14:0,
                 from db/mork/src/morkArray.cpp:21:
db/mork/src/morkObject.h:92:21: error:   by ‘virtual mork_uses morkObject::AddStrongRef(nsIMdbEnv*)’ [-Werror=overloaded-virtual]
   NS_IMETHOD_(mork_uses) AddStrongRef(nsIMdbEnv* ev);
                     ^
-Linux & OS X:
mailnews/base/search/src/nsMsgLocalSearch.cpp:279:9: error: case value '2153054213' not in enumerated type 'nsresult' [-Werror=switch]
(Thanks to Jorg I replaced this with ifs for now so we could look beyond this spot, but other similar places exist)
Just on the last item:
nsresult is an enum owned by M-C:
https://dxr.mozilla.org/comm-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/mozilla/xpcom/base/nsError.h#122

I've seen something about "extending" enums, but no idea how relevant that is:
https://reviews.llvm.org/D2334 (bottom of the page).

Or from bug 1081010:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1081010&attachment=8503016
switch (static_cast<int>(variable)) {

For nsresult we should try:
switch (static_cast<uint32_t>err)) {
(In reply to :aceman from comment #32)
> -Linux:
> In file included from db/mork/src/morkArray.cpp:17:0:
> db/mork/src/morkNode.h:223:21: error: ‘virtual mork_uses
> morkNode::AddStrongRef(morkEnv*)’ was hidden [-Werror=overloaded-virtual]
>    virtual mork_uses    AddStrongRef(morkEnv* ev);
>                      ^
> In file included from db/mork/src/morkEnv.h:14:0,
>                  from db/mork/src/morkArray.cpp:21:
> db/mork/src/morkObject.h:92:21: error:   by ‘virtual mork_uses
> morkObject::AddStrongRef(nsIMdbEnv*)’ [-Werror=overloaded-virtual]
>    NS_IMETHOD_(mork_uses) AddStrongRef(nsIMdbEnv* ev);

Two options for this:
https://dxr.mozilla.org/comm-central/source/mozilla/dom/base/DOMException.h#10
https://dxr.mozilla.org/comm-central/source/mozilla/dom/mobilemessage/MobileMessageCursorCallback.h#39
Should we rename this bug already so that people can find it? We probably do not port much from the m-c bug, we actually fix the fallout.
Severity: normal → major
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Summary: Port |Bug 1292463 - Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure| to TB → Fix Mailnews compiler warnings after they got upgraded to errors in bug 1292463 (Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure)
Attached patch WIP - real issues. (obsolete) — Splinter Review
Attaching Aceman's latest WIP from try.
Attached patch WIP - syntactic issues. (obsolete) — Splinter Review
Attaching Aceman's latest WIP from try.

I'll see what's happening on Windows based on those two patches and will submit my modifications separately.

Let me know if that's a bad idea due to double/wasted work.
Attachment #8779916 - Attachment is obsolete: true
No problem, thanks. I will not work on it for the next 10 hours :)
OK, I will see what I can done in the next few hours and add another additional patch. I'm afraid that the Windows compiler is a lot less fussy, so what compiles on Windows won't necessarily compile elsewhere. I don't think it complains about the "fallthrough". At least I might find a few more overrides. Oops, compilation just failed in Mork.
I need to do this:
  // The first declaration of AddStrongRef is to suppress -Werror,-Woverloaded-virtual.
#ifndef XP_WIN
  NS_IMETHOD_(mork_uses) AddStrongRef(morkEnv* ev) override;
#endif

Nice, huh? OK, I won't comment on every bit, I'll just hand over in the afternoon.
It could be better to check for the MS C compiler, not for Windows.
That mork changes were needed locally for me (it think I am almost done on linux). They are not (yet?) visible on try on any platform (not even linux on try as I have the m-c patch that skips the enumeration errors locally).
My compile went through, but I have a link error:
public: virtual __thiscall nsIMdbHeap::~nsIMdbHeap(void)".
I'll fix that.

Have you tried:
switch (static_cast<uint32_t>(err)) {
from comment #33?
(In reply to Jorg K (GMT+2, PTO during summer) from comment #42)
> My compile went through, but I have a link error:
> public: virtual __thiscall nsIMdbHeap::~nsIMdbHeap(void)".
> I'll fix that.

Yeah, I added that but didn't get to the link phase so I haven't yet hit that.
This was supposed to fix an error about "deleting an object of abstract class with non-virtual destructor may have unexpected results" (or similar).

> Have you tried:
> switch (static_cast<uint32_t>(err)) {
> from comment #33?

Not yet.
Or did I miss something and I need to set
ac_add_options --enable-warnings-as-errors
(since this is only set in automation)?
Yes, you need to set that option if you want to find all the errors locally (without manually reading all the compiler output for warnings). Without it, local build may pass even without any patch (the same warnings will not abort it).

Seems like there is still the mozilla-config.h problem I mentioned yesterday on IRC.
(In reply to :aceman from comment #46)
> Yes, you need to set that option if you want to find all the errors locally
That's not so useful. If at all, it should stop on a local build.

> Seems like there is still the mozilla-config.h problem I mentioned yesterday
> on IRC.
Hmm, yes.
mozilla-config.h(145): error C2220: warning treated as error - no 'object' file generated
That line says:
#define _WINDOWS 1
Perhaps that's somehow redefined?

Error C2220 is: https://msdn.microsoft.com/en-us/library/ksk5k0ta.aspx
Hmm, I added ac_add_options --enable-warnings-as-errors to .mozconfig.
I touched all .h, .c and .cpp files in
calendar, chat, db, editor, im, ldap, mail, mailnews, testing.
The I ran mozilla/mach build
Result: Lots of warnings coming out of LDAP, no other warning. After a few minutes everything is compiled.
What am I doing wrong?

Oh, apparently nothing. Reading through:
https://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-cbd63ada86e807aaf85391b563259b7e611ed819/try-comm-central-win32/try-comm-central-win32-bm76-try1-build2.txt.gz
here is the problem:
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/objdir-tb/mozilla-config.h(145): error C2220: warning treated as error - no 'object' file generated
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/objdir-tb/mozilla-config.h(145): warning C4005: '_WIN32_WINNT': macro redefinition
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/objdir-tb/mozilla-config.h(145): note: command-line arguments:  see previous definition of '_WIN32_WINNT'
Comment on attachment 8780424 [details] [diff] [review]
4th patch to make it compile, link, run and even work on Windows.

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

::: db/mork/src/morkCursor.h
@@ +49,5 @@
>      mdb_count* outCount);
>  
>    NS_IMETHOD AddWeakRef(nsIMdbEnv* ev);
>    // The first declaration of AddStrongRef is to suppress -Werror,-Woverloaded-virtual.
> +#ifndef XP_WIN

As I asked, would ifndef _MSC_VER work for these cases?
No idea. But where does the |previous definition of '_WIN32_WINNT'| come from? There is no problem locally.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #50)
> No idea. But where does the |previous definition of '_WIN32_WINNT'| come
> from? There is no problem locally.

I don't know anything about this macro, but maybe here?
https://dxr.mozilla.org/comm-central/source/mailnews/mapi/mapihook/build/moz.build#15
And it is also defined in the Makefile in that directory.
This needs to look into the specific mozilla-config.h file generated on that Windows system. On Linux, this macro is not there. But Jorg said the mentioned line 145 in the file contains some other macro...
I saw that too, OK, let's try it.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=471014faab63

Same patch as before plus:
diff --git a/mailnews/mapi/mapihook/build/moz.build b/mailnews/mapi/mapihook/build/moz.build
--- a/mailnews/mapi/mapihook/build/moz.build
+++ b/mailnews/mapi/mapihook/build/moz.build

-DEFINES['_WIN32_WINNT'] = '0x400'
+if not CONFIG['_WIN32_WINNT']:
+    DEFINES['_WIN32_WINNT'] = '0x400'
(In reply to :aceman from comment #52)
> And it is also defined in the Makefile in that directory.
I don't see that.

> This needs to look into the specific mozilla-config.h file generated on that
> Windows system. On Linux, this macro is not there. But Jorg said the
> mentioned line 145 in the file contains some other macro...
Looks like and off-by-one:
144: #define _WIN32_WINNT 0x502
145: #define _WINDOWS 1

See comment #48 to see that the problem appears to be _WIN32_WINNT and not _WINDOWS.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #53)
> -DEFINES['_WIN32_WINNT'] = '0x400'
> +if not CONFIG['_WIN32_WINNT']:
> +    DEFINES['_WIN32_WINNT'] = '0x400'

What's the meaning of the value here? It seems to be set to different values in different places.
Please ask something I can answer :-(
(In reply to Jorg K (GMT+2, PTO during summer) from comment #56)
> Please ask something I can answer :-(

Maybe someone on #build knows? Or check hg blame for who added that define?
I ask because your change means that if _WIN32_WINNT was already defined, it may end up with a different value than 0x400.
I read somewhere that it indicates the lowest Windows version the code requires. So we probably want it to be 0x400 in this folder. Better would be to unset the existing value and set to 0x400, not preserve its value.
In fact, it looks like a Windows version:
4 = Win 95
502 = +/-XP
6 = Vista
See for example:
https://dxr.mozilla.org/comm-central/source/mozilla/python/psutil/psutil/_psutil_windows.c#690
If the overall program requires XP, it doesn't make much sense to require Win95 to compile the MAPI/Outlook stuff, does it? Anyway, see that the try says.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #60)
> If the overall program requires XP, it doesn't make much sense to require
> Win95 to compile the MAPI/Outlook stuff, does it? Anyway, see that the try
> says.

It's probably set to 4 as it's old code, and as other parts of the codebase set it to 6 probably the only thing that matters is that 4 < 6...
OK,
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=471014faab63
didn't work (see comment #53)
and removing it altogether in
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=25c19f435c71
didn't work either:

c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/mapiDLL/MapiDll.cpp(251): error C2220: warning treated as error - no 'object' file generated
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/mapiDLL/MapiDll.cpp(251): warning C4996: 'MAPILogoff': was declared deprecated
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/mapiDLL/MapiDll.cpp(159): note: see declaration of 'MAPILogoff'
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/mapiDLL/MapiDll.cpp(275): warning C4996: 'MAPILogoff': was declared deprecated
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/mapiDLL/MapiDll.cpp(159): note: see declaration of 'MAPILogoff'
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mozilla/config/rules.mk:950: recipe for target 'MapiDll.obj' failed
mozmake.exe[5]: *** [MapiDll.obj] Error 2

Most likely Aceman was right (comment #58) that it needs to be set to 0x400 and then reset. No idea how to achieve this.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #62)
> c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/
> mapiDLL/MapiDll.cpp(251): error C2220: warning treated as error - no
> 'object' file generated
> c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/
> mapiDLL/MapiDll.cpp(251): warning C4996: 'MAPILogoff': was declared
> deprecated
> c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/
> mapiDLL/MapiDll.cpp(159): note: see declaration of 'MAPILogoff'
> c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/
> mapiDLL/MapiDll.cpp(275): warning C4996: 'MAPILogoff': was declared
> deprecated
> c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mailnews/mapi/
> mapiDLL/MapiDll.cpp(159): note: see declaration of 'MAPILogoff'
> c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mozilla/config/
> rules.mk:950: recipe for target 'MapiDll.obj' failed
> mozmake.exe[5]: *** [MapiDll.obj] Error 2

That's a different warning in a different folder, so maybe it worked after all?

> Most likely Aceman was right (comment #58) that it needs to be set to 0x400
> and then reset. No idea how to achieve this.

It's interesting that that seems the only place in c-c or m-c where _WIN32_WINNT is defined in a moz.build. Maybe it could be done the same way as it is done elsewhere.
(In reply to aleth [:aleth] from comment #63)
> That's a different warning in a different folder, so maybe it worked after
> all?
What worked after all? Sure, the double definition is gone, but now something else doesn't compile that needs the lower version. :-(
(In reply to Jorg K (GMT+2, PTO during summer) from comment #64)
> (In reply to aleth [:aleth] from comment #63)
> > That's a different warning in a different folder, so maybe it worked after
> > all?
> What worked after all? Sure, the double definition is gone, but now
> something else doesn't compile that needs the lower version. :-(

I did put a question mark there.

My point is: does a DEFINES in mailnews/mapi/mapihook/build affect mailnews/mapi/mapiDLL at all ?
(In reply to aleth [:aleth] from comment #65)
> My point is: does a DEFINES in mailnews/mapi/mapihook/build affect
> mailnews/mapi/mapiDLL at all ?
I'd say so. The latter is most likely compiled later. But just ignore me, I really have no idea.
Maybe take the _WIN32_WINNT out of mailnews/mapi/mapihook/build/moz.build and put
#define _WIN32_WINNT 0x0400
into MapiDll.cpp with some trickery to preserve and restore the previous value:

#ifdef _WIN32_WINNT
#define save_WIN32_WINNT _WIN32_WINNT
#endif
#define _WIN32_WINNT 0x0400
and at the end
#undef _WIN32_WINNT
#ifdef save_WIN32_WINNT
#define _WIN32_WINNT save_WIN32_WINNT
#endif
... or words to that effect.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #66)
> (In reply to aleth [:aleth] from comment #65)
> > My point is: does a DEFINES in mailnews/mapi/mapihook/build affect
> > mailnews/mapi/mapiDLL at all ?
> I'd say so. The latter is most likely compiled later. But just ignore me, I
> really have no idea.

Afaik you should see the effect (or not) of a moz.build DEFINES by looking at the -D... parameters in the compiler command line in the log.
Well, did it a little differently since the above was nonsense ;-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1d6caf195e61
(In reply to Jorg K (GMT+2, PTO during summer) from comment #42)
> My compile went through, but I have a link error:
> public: virtual __thiscall nsIMdbHeap::~nsIMdbHeap(void)".
> I'll fix that.

I also have link failures from the mork changes. I look at them.

> Have you tried:
> switch (static_cast<uint32_t>(err)) {
> from comment #33?

This does not work:
mailnews/compose/src/nsMsgSendReport.cpp: In member function ‘virtual nsresult nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, nsresult*)’:
mailnews/compose/src/nsMsgSendReport.cpp:279:12: error: could not convert ‘NS_BINDING_ABORTED’ from ‘const nsresult’ to ‘unsigned int’
       case NS_BINDING_ABORTED:
            ^
mailnews/compose/src/nsMsgSendReport.cpp:280:98: error: could not convert ‘(nsresult)2153066728u’ from ‘nsresult’ to ‘unsigned int’
Latest try fixed the MAPI problems:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=af8a4880f2ee

The next problem popped up in ical:
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/calendar/libical/src/libical/icalerror.c(120): error C2220: warning treated as error - no 'object' file generated
c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/calendar/libical/src/libical/icalerror.c(120): warning C4273: 'icalerror_errors_are_fatal': inconsistent dll linkage
c:\builds\moz2_slave\tb-try-c-cen-w32-0000000000000\build\calendar\libical\src\libical\icalerror.h(82): note: see previous definition of 'icalerror_errors_are_fatal'

Since I don't know how to fix this, I'm just disabling the warning.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13c78114b6c6
Attachment #8780424 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2, PTO during summer) from comment #33)
> Just on the last item:
> nsresult is an enum owned by M-C:
> https://dxr.mozilla.org/comm-central/rev/
> 6cf0089510fad8deb866136f5b92bbced9498447/mozilla/xpcom/base/nsError.h#122
> 
> I've seen something about "extending" enums, but no idea how relevant that
> is:
> https://reviews.llvm.org/D2334 (bottom of the page).
> 
> Or from bug 1081010:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=1081010&attachment=8503016
> switch (static_cast<int>(variable)) {
> 
> For nsresult we should try:
> switch (static_cast<uint32_t>err)) {

there was a patch for switch in bug 1202150:
https://bugzilla.mozilla.org/attachment.cgi?id=8667386

it's unclear why a cast solution is suboptimal but see comments around here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1202150#c14
My push
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=08b8abd26cd5
with Aceman's previous patches actually worked, so the MAPI issue and the ical issue I saw were fixed. That goes to show that the Windows compiler is *far less* fussy then Linux or Mac.

I will repeat now with latest patches after my local compile completes.
Comment on attachment 8780069 [details] [diff] [review]
Part 1: Allow compiler warnings in ldap [landed in comment #74]

Landed LDAP patch:
https://hg.mozilla.org/comm-central/rev/50bcb5618049
Attachment #8780069 - Attachment description: Allow compiler warnings in ldap → Allow compiler warnings in ldap [landed in comment #74]
Attachment #8780069 - Attachment description: Allow compiler warnings in ldap [landed in comment #74] → Part 1: Allow compiler warnings in ldap [landed in comment #74]
Attached patch Part 2: WIP - real issues. (obsolete) — Splinter Review
Needs to be applied first.
Attachment #8780398 - Attachment is obsolete: true
Attached patch Part 4: WIP - Windows (obsolete) — Splinter Review
With this patch and part 2/3 a local build works.

Looks like I don't need the
+#ifndef _MSC_VER
   virtual ~nsIMdbHeap() {};
+#endif
I'll remove it in the next round.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=946423be1c61
Attachment #8780707 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2, PTO during summer) from comment #77)
> Looks like I don't need the
> +#ifndef _MSC_VER
>    virtual ~nsIMdbHeap() {};
> +#endif
> I'll remove it in the next round.

Yes, this didn't link for you due to missing {} which I found out  have to add to linux too. With the {} this should link on all platforms so you do not need to ifndef it.
Attached patch Part 4: Windows (obsolete) — Splinter Review
Maybe we can land this so whatever Aceman does will always compile on Windows.
Attachment #8780799 - Attachment is obsolete: true
Attachment #8780821 - Flags: review?(aleth)
Bug 1212068 still has a patch for the warnings that were previously reported on Windows. Maybe it helps with current bustage.
Aceman has the hunks that relate to signed/unsigned from attachment 8670459 [details] [diff] [review] from bug 1212068 covered.
Yes, but I already said I'm fine with landing logically grouped changes in separate bugs so that we do not land a 200KB patch here.
Attached patch Part 2: WIP - real issues v2 (obsolete) — Splinter Review
Attachment #8780795 - Attachment is obsolete: true
Attachment #8780797 - Attachment is obsolete: true
Aleth, your "win-reduced" won't work, you need the ical stuff, see:https://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-af8a4880f2ee41cf3efc93870723d8809e3dbf08/try-comm-central-win32/try-comm-central-win32-bm76-try1-build4.txt.gz

Also, if you don't remove the second definition for _WIN32_WINNT it won't compile either.

I'm not sure why you're redoing the experiments I've already done and documented here.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #85)
> I'm not sure why you're redoing the experiments I've already done and
> documented here.

Don't worry, I'm not redoing your work.
Has anyone considered to ignore warnings in Mork like we do in LDAP for the time being?
Assignee: nobody → acelists
Severity: major → blocker
Comment on attachment 8780821 [details] [diff] [review]
Part 4: Windows

lgtm. I'll let aceman decide how it fits in with his patch series.
Attachment #8780821 - Flags: review?(aleth) → review?(acelists)
Comment on attachment 8780836 [details] [diff] [review]
Part 5: Allow compiler warnings in libical [landed in comment 93]

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

OK, if libical is third-party code, it does not make much sense to maintain it when it gets pulled from upstream. I also think it is destined to be removed soon (replaced by the JS version).
Attachment #8780836 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #90)
> OK, if libical is third-party code, it does not make much sense to maintain
> it when it gets pulled from upstream. I also think it is destined to be
> removed soon (replaced by the JS version).

Makemyday also approved this on IRC.
Comment on attachment 8780821 [details] [diff] [review]
Part 4: Windows

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

::: calendar/libical/src/libical/icalerror.h
@@ +69,5 @@
>   *  @warning NOT THREAD SAFE -- recommended that you do not change
>   *           this in a multithreaded program.
>   */
>  #ifdef _MSC_VER
> +#pragma warning (disable : 4273)

Please see if this can hunk can be dropped after aleth's path (warnings allowed in libical).

::: mailnews/mapi/mapihook/build/moz.build
@@ -11,5 @@
>  
>  for var in ('REGISTER_PROXY_DLL', 'UNICODE', '_UNICODE'):
>      DEFINES[var] = True
>  
> -DEFINES['_WIN32_WINNT'] = '0x400'

The removal seems fine, but what is the replacement?

In other mail files we do:
#ifdef _WIN32_WINNT
#undef _WIN32_WINNT
#define _WIN32_WINNT 0x0600
(https://dxr.mozilla.org/comm-central/source/mail/components/shell/nsMailWinIntegration.cpp)

Can you try that with 0400? Maybe the pragma is not needed then.
Comment on attachment 8780836 [details] [diff] [review]
Part 5: Allow compiler warnings in libical [landed in comment 93]

https://hg.mozilla.org/comm-central/rev/fa297b77de80624ec40619da331c0ee306613e83
Attachment #8780836 - Attachment description: Part 5: Allow compiler warnings in libical → Part 5: Allow compiler warnings in libical [landed in comment 93]
(In reply to :aceman from comment #92)
> Please see if this can hunk can be dropped after aleth's path (warnings
> allowed in libical).
Yes, it can. I tried it and it works locally. I was under the impression that you were working on a patch to fix libical issues:
https://hg.mozilla.org/try-comm-central/rev/08a318933dd5
but I see that's not finished and I also see that libical is 3rd party software.

> The removal seems fine, but what is the replacement?
The beauty is: There is no replacement. That's the whole idea.
The only replacement is the pragma and that's quite frequently used in M-C:
https://dxr.mozilla.org/comm-central/search?q=pragma+4996&redirect=false

> In other mail files we do:
> #ifdef _WIN32_WINNT
> #undef _WIN32_WINNT
> #define _WIN32_WINNT 0x0600
> (https://dxr.mozilla.org/comm-central/source/mail/components/shell/
> nsMailWinIntegration.cpp)
> Can you try that with 0400? Maybe the pragma is not needed then.
I haven't been twiddling my thumbs either ;-)
That's been tried already:
https://hg.mozilla.org/try-comm-central/rev/01a7d1d3f89bdff43fcc2c93ec317cc9e8a58d0d#l6.8
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1d6caf195e61b9a468fcdaa10c9388abc5e19686&selectedJob=20855

There result is:
sdkddkver.h(280): fatal error C1189: #error: _WIN32_WINNT settings conflicts with _WIN32_IE setting
So I prefer not to mess with this define when a commonly used pragma is an easy way out here.
Attachment #8780821 - Attachment is obsolete: true
Attachment #8780821 - Flags: review?(acelists)
Attachment #8780846 - Flags: review?(acelists)
Aceman, I'm a little confused as to what the latest patches are to try, so maybe you can just land "Part 4" or include it in one of your try pushes. I saw that Aleth got a green Linux try push and I got green Windows try pushes, so perhaps we can just review and land those patches and worry about the fussy Mac compiler later. It would be good to have a build on two platforms.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #94)
> (In reply to :aceman from comment #92)
> > Please see if this can hunk can be dropped after aleth's path (warnings
> > allowed in libical).
> Yes, it can. I tried it and it works locally. I was under the impression
> that you were working on a patch to fix libical issues:
Yes I tried, before we got it is 3rd party code.

> There result is:
> sdkddkver.h(280): fatal error C1189: #error: _WIN32_WINNT settings conflicts
> with _WIN32_IE setting
> So I prefer not to mess with this define when a commonly used pragma is an
> easy way out here.
OK, I'm not happy with the solution as we don't really understand what the setting did. But we can try it. Watch out for mapi reports :)

It is still in the Makefile in that folder. Will you remove it or is that one never used?

(In reply to Jorg K (GMT+2, PTO during summer) from comment #95)
> Aceman, I'm a little confused as to what the latest patches are to try, so
> maybe you can just land "Part 4" or include it in one of your try pushes. I
> saw that Aleth got a green Linux try push and I got green Windows try
> pushes, so perhaps we can just review and land those patches and worry about
> the fussy Mac compiler later. It would be good to have a build on two
> platforms.

I don't think any of the greens were achieved without Part 2 and 3. But sure when Part 4 is fine we can land it soon.
But if you meant to land Parts 2 & 3 as they are (when I upload current versions) to get Linux and Windows going and then make a Part 6 for the remaining OS X issues, that is also a possibility.
Give me an hour as we think we are also very close on OS X. If that proves incorrect we can proceed with the plan with Part 6.
(In reply to :aceman from comment #96)
> OK, I'm not happy with the solution as we don't really understand what the
> setting did. But we can try it. Watch out for mapi reports :)
There won't be any since Outlook import is (permanently) disabled.

> It is still in the Makefile in that folder. Will you remove it or is that
> one never used?
I don't intend to do anything else.

(In reply to :aceman from comment #97)
> Give me an hour as we think we are also very close on OS X.
At your disposal until midnight ;-)
Comment on attachment 8780846 [details] [diff] [review]
Part 4: Windows (v2) [landed in comment 104]

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

OK, let's try it.
Attachment #8780846 - Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+2, PTO during summer) from comment #98)
> (In reply to :aceman from comment #96)
> > OK, I'm not happy with the solution as we don't really understand what the
> > setting did. But we can try it. Watch out for mapi reports :)
> There won't be any since Outlook import is (permanently) disabled.

Outlook is no Eudora and should be enabled back in the long run.

Also, AFAIK MAPI has no relation to Outlook import.
(In reply to :aceman from comment #99)
> Attachment #8780846 [details] [diff] - Flags: review?(acelists@atlas.sk) → review+
Please land or include in your try pushes as you deem fit ;-)
I'll push it now, but I will rather just comment out the define instead of removing it.
I think we have bugs about MAPI gradually deteriorating in newer Windows versions. If somebody takes it on, he may have a clue for what that define is needed (and give him some hint).
(In reply to :aceman from comment #102)
> I think we have bugs about MAPI gradually deteriorating in newer Windows
Maybe because it was stuck at Windows 95 (0x400)?
Comment on attachment 8780846 [details] [diff] [review]
Part 4: Windows (v2) [landed in comment 104]

https://hg.mozilla.org/comm-central/rev/be4c5a82c1c04686dedf282cc9fca2168c361b8b

I commented out the _WIN32_WINNT define instead of removing it, to keep some clues.
Attachment #8780846 - Attachment description: Part 4: Windows (v2). → Part 4: Windows (v2) [landed in comment 104]
Attached patch Part 2: real issues v3 (obsolete) — Splinter Review
These warnings now cause build failures on at least one platform. I separated these out as the fixes aren't just mechanical syntax changes. They may change behaviour as the warnings could have found real bugs.

Successful try runs:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=40cbb8dd3d9b3e55440983facac2a484e3fb361d
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bb91d63c03667284870bfe13421263c7b43f1203
Attachment #8780823 - Attachment is obsolete: true
Attachment #8780863 - Flags: review?(rkent)
Attachment #8780863 - Flags: review?(mkmelin+mozilla)
Attached patch Part 3: syntactic issues v3 (obsolete) — Splinter Review
This one is also needed to remove warnings on at least one platform. But these seem to be mostly syntactic changes. They just need decision if the way to silence the warning is acceptable.
Try runs are the same as in previous comment.
Attachment #8780824 - Attachment is obsolete: true
Attachment #8780865 - Flags: review?(jorgk)
Attachment #8780865 - Flags: review?(aleth)
Comment on attachment 8780863 [details] [diff] [review]
Part 2: real issues v3

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

Looks ok to me

::: mailnews/base/src/nsMsgDBView.cpp
@@ +6377,5 @@
>        // the caller needs to have adjusted m_keys before getting here, since
>        // RowCountChanged() will call our GetRowCount()
>        mTree->RowCountChanged(firstLineChanged, numChanged);
>        mRemovingRow = false;
> +      MOZ_FALLTHROUGH; // really?

I think we can skip the comment ;)
Attachment #8780863 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8780865 [details] [diff] [review]
Part 3: syntactic issues v3

You wouldn't believe it, but I looked through the whole lot.
As mentioned in IRC, the cost -> const correction needs to go into the other patch.

I'm more used to "#if 0" instead of "#if false".

Perhaps put a comment on the replace function and the ugly if (false && ...).

THANKS!!
Attachment #8780865 - Flags: review?(jorgk) → review+
Comment on attachment 8780865 [details] [diff] [review]
Part 3: syntactic issues v3

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

::: mailnews/compose/src/nsMsgAppleEncode.cpp
@@ +43,5 @@
>  **	write_stream.
>  */
>  int write_stream(
>  	appledouble_encode_object *p_ap_encode_obj,
> +	const char *out_string,

Some additional changes needed in this file - I provided a patch over IRC.
Attachment #8780865 - Flags: review?(aleth) → review-
(In reply to Magnus Melin from comment #107)
> ::: mailnews/base/src/nsMsgDBView.cpp
> @@ +6377,5 @@
> >        // the caller needs to have adjusted m_keys before getting here, since
> >        // RowCountChanged() will call our GetRowCount()
> >        mTree->RowCountChanged(firstLineChanged, numChanged);
> >        mRemovingRow = false;
> > +      MOZ_FALLTHROUGH; // really?
> 
> I think we can skip the comment ;)

It was for the reviewer, as I am not sure the place really wanted a passthrough.
Comment on attachment 8780863 [details] [diff] [review]
Part 2: real issues v3

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

I saw lots of ugly code, the winner is:
#if 0
    bool searchable=false;
    rv = m_nntpServer->QueryExtension("LISTSUBSCR",&listsubscr);
    if (NS_SUCCEEDED(rv) && listsubscr)
#else
  if (0)
#endif
  {
Terrible!!

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1727,2 @@
>      rv = msgArray->AppendElement(aSupport, false);
> +    NS_ENSURE_SUCCESS(rv, rv);

Maybe set hasMoreElements = false; in case the following doesn't succeed. Just good practise to initialise out parameters.
Attachment #8780863 - Flags: review+
As discussed on IRC:
You might as well change both these:
https://dxr.mozilla.org/comm-central/search?q=kABAIMInstantProperty&redirect=false
(In reply to Jorg K (GMT+2, PTO during summer) from comment #111)
> I saw lots of ugly code, the winner is:
> #if 0
>     bool searchable=false;
>     rv = m_nntpServer->QueryExtension("LISTSUBSCR",&listsubscr);
>     if (NS_SUCCEEDED(rv) && listsubscr)
> #else
>   if (0)
> #endif
>   {
> Terrible!!

Yes, this was interesting :) That's what we get if there is no comment on why something was disabled.
Attached patch portable.patch (obsolete) — Splinter Review
If you do additional ldap changes you might want to check this. Seems to be no longer needed for recent VC versions and shows a redefine warning.
Comment on attachment 8780868 [details] [diff] [review]
portable.patch

(In reply to Frank-Rainer Grahl from comment #114)
> If you do additional ldap changes you might want to check this. Seems to be
> no longer needed for recent VC versions and shows a redefine warning.

We have decided to ignore warnings for now in LDAP SDK as it is 3rd party code.
While there were some changes floating around here, they were finally scraped and won't be landing.

Also, I'm not sure we can decide on the correctness of this patch here quickly (we are no experts on the LDAP code). Please open a new bug for it and request a review from jcranmer. Thanks
Attachment #8780868 - Attachment is obsolete: true
Attachment #8780868 - Flags: review-
Attached patch Part 2: real issues v4 (obsolete) — Splinter Review
Updated final patch, with minor tweaks for OS X *AppleEncoding* files, thanks to aleth.
Attachment #8780863 - Attachment is obsolete: true
Attachment #8780863 - Flags: review?(rkent)
Attachment #8780874 - Flags: review?(rkent)
Final patch, makes OS X build (except mork).
Attachment #8780865 - Attachment is obsolete: true
Attachment #8780875 - Flags: review?(aleth)
This one is the final block to make OS X build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=31003c492a83d1fc502578afcdea842b5701f046

Contains some strange constructs like "if (this) {...}" or "if (&reference) {}" on which Clang complained that they should be always true in proper C++"
Attachment #8780876 - Flags: review?(rkent)
Attachment #8780876 - Flags: review?(jorgk)
Status: NEW → ASSIGNED
Comment on attachment 8780874 [details] [diff] [review]
Part 2: real issues v4

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

::: mailnews/compose/src/nsMsgAppleDoubleEncode.cpp
@@ +144,5 @@
>  	*/
>  	if (p_ap_encode_obj->s_overflow)
>  	{
> +		status = write_stream(p_ap_encode_obj,
> +		                      (const char*)(p_ap_encode_obj->b_overflow),

These tweaks were probably meant for the other patch, where the write_stream definition is changed ;) Not that it matters much...
Comment on attachment 8780875 [details] [diff] [review]
Part 3: syntactic issues v4 [landed in comment 126]

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

Thanks!
Attachment #8780875 - Flags: review?(aleth) → review+
(In reply to aleth [:aleth] from comment #119)
> These tweaks were probably meant for the other patch, where the write_stream
> definition is changed ;) Not that it matters much...

Yes, done, thanks.
Comment on attachment 8780874 [details] [diff] [review]
Part 2: real issues v4

Do we really need four reviewers here?
Magnus, Aleth and myself have been through it.
Attachment #8780874 - Flags: review?(rkent) → review+
Attachment #8780874 - Attachment is obsolete: true
Attachment #8780893 - Flags: review?(rkent)
Comment on attachment 8780875 [details] [diff] [review]
Part 3: syntactic issues v4 [landed in comment 126]

https://hg.mozilla.org/comm-central/rev/28876b046ec271296611b271d6329937889dc694
Attachment #8780875 - Attachment description: Part 3: syntactic issues v4 → Part 3: syntactic issues v4 [landed in comment 126]
Comment on attachment 8780876 [details] [diff] [review]
Part 6: warnings in mork [landed in comment 130]

Sorry, ignore my previous comment #122 for this patch, they were meant for Part 2 as can be seen in comment #123.

Anyway, I've been through this and there is nothing exciting happening. The fixes fall into five categories:

1) if (this) or if (!this)
2) if (&argument)
3) removed register
4) missing override.
5) Additions of identical functions (which Windows complains about).
Comment on attachment 8780893 [details] [diff] [review]
Part 2: real issues v4.1 [landed in comment 131]

OK, repeating comment #123 for the previous version of this patch:

Do we really need four reviewers here?
Magnus, Aleth and myself have been through it.

Can we land the two missing parts (no idea why Part 3 was landed separately). I've checked that the change in nsMsgAppleCodes.h/nsMsgAppleDoubleEncode.cpp was removed here and already landed in Part 3).
Attachment #8780893 - Flags: review?(rkent) → review+
If it makes you happy: I've done an interdiff of Part 2: v2 (r+ from Magnus and myself) and v4.1 and I fully approve those extra changes.
Comment on attachment 8780876 [details] [diff] [review]
Part 6: warnings in mork [landed in comment 130]

https://hg.mozilla.org/comm-central/rev/3e4c9e3a9099b0bb077255abb75463784ee929e5
Attachment #8780876 - Attachment description: Part 6: warnings in mork → Part 6: warnings in mork [landed in comment 130]
Comment on attachment 8780893 [details] [diff] [review]
Part 2: real issues v4.1 [landed in comment 131]

https://hg.mozilla.org/comm-central/rev/7f35b399aa0050d2cdc1f83e7aac54230b9d03e1
Attachment #8780893 - Attachment description: Part 2: real issues v4.1 → Part 2: real issues v4.1 [landed in comment 131]
All platforms building now and tests seem unaffected.
I think we are done here.
Thanks to aleth and jorgk for quick cooperation :)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
Blocks: 1296151
Blocks: 1232306
Hi, I have uploaded a set of patches to compile LDAP with -Werror=sign-compare in
Bug 1243121 - C-C ldap directory: fix -Werror=sign-compare: signed vs unsigned warnings (now treated as errors) 
(I edited the title a bit)
so that if someone is willing to modify the LDAP files, they can use it as a comparison/starting point/whatever.
Mostly it is a cast at the time of usage, but there are a few variable type declarations.

TIA
See Also: → 1243121
Depends on: 1455850
You need to log in before you can comment on or make changes to this bug.