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

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[aleth [:aleth]]

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.

Comment 2

[:aceman]

Attachment: check NS_NewISupportsArray

So this brings linux in line with the other platforms.

The LDAP failure still remains. I can look at that later.

Comment 3

[Jorg K (CEST = GMT+2)]

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

Comment 4

[:aceman]

Must be too late :)

Comment 5

[:aceman]

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.

Comment 6

[aleth [:aleth]]

(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.

Comment 7

[:aceman]

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.

Comment 8

[Jorg K (CEST = GMT+2)]

(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).

Comment 9

[aleth [:aleth]]

(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.

Comment 10

[aleth [:aleth]]

--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).

Comment 11

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d3f2850ed21660a9523cd92f208dffdc6f44b21

Comment 12

[aleth [:aleth]]

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.

Comment 13

[aleth [:aleth]]

Looks like it's now failing in mailnews/ (comment 11)?

Comment 14

[aleth [:aleth]]

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.

Comment 15

[aleth [:aleth]]

Attachment: Part 1: Allow compiler warnings in ldap [landed in comment #74]

Comment 16

[Jorg K (CEST = GMT+2)]

(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 17

[Jorg K (CEST = GMT+2)]

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.

Comment 18

[:aceman]

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=]

Comment 19

[:aceman]

(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

Comment 20

[aleth [:aleth]]

(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.

Comment 21

[aleth [:aleth]]

Ah, looks like I midaired with aceman's comments, sorry.

Comment 22

[aleth [:aleth]]

(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?

Comment 23

[:aceman]

(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.

Comment 24

[:aceman]

(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);

Comment 25

[aleth [:aleth]]

(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?

Comment 26

[:aceman]

(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.

Comment 27

[aleth [:aleth]]

(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 ;)

Comment 28

[Jorg K (CEST = GMT+2)]

(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.

Comment 29

[:aceman]

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.

Comment 30

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 32

[:aceman]

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)

Comment 33

[Jorg K (CEST = GMT+2)]

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)) {

Comment 34

[aleth [:aleth]]

(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

Comment 35

[:aceman]

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.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: WIP - real issues.

Attaching Aceman's latest WIP from try.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: WIP - syntactic issues.

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.

Comment 38

[:aceman]

No problem, thanks. I will not work on it for the next 10 hours :)

Comment 39

[Jorg K (CEST = GMT+2)]

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.

Comment 40

[Jorg K (CEST = GMT+2)]

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.

Comment 41

[:aceman]

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).

Comment 42

[Jorg K (CEST = GMT+2)]

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?

Comment 43

[:aceman]

(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.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 4th patch to make it compile, link, run and even work on Windows.

Only some tweaks to Mork.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cbd63ada86e8

Comment 45

[Jorg K (CEST = GMT+2)]

Or did I miss something and I need to set
ac_add_options --enable-warnings-as-errors
(since this is only set in automation)?

Comment 46

[:aceman]

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.

Comment 47

[Jorg K (CEST = GMT+2)]

(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

Comment 48

[Jorg K (CEST = GMT+2)]

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/[e-mail]-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 49

[:aceman]

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?

Comment 50

[Jorg K (CEST = GMT+2)]

No idea. But where does the |previous definition of '_WIN32_WINNT'| come from? There is no problem locally.

Comment 51

[aleth [:aleth]]

(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

Comment 52

[:aceman]

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...

Comment 53

[Jorg K (CEST = GMT+2)]

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'

Comment 54

[Jorg K (CEST = GMT+2)]

(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.

Comment 55

[aleth [:aleth]]

(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.

Comment 56

[Jorg K (CEST = GMT+2)]

Please ask something I can answer :-(

Comment 57

[aleth [:aleth]]

(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.

Comment 58

[:aceman]

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.

Comment 59

[Jorg K (CEST = GMT+2)]

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

Comment 60

[Jorg K (CEST = GMT+2)]

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.

Comment 61

[aleth [:aleth]]

(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...

Comment 62

[Jorg K (CEST = GMT+2)]

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.

Comment 63

[aleth [:aleth]]

(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.

Comment 64

[Jorg K (CEST = GMT+2)]

(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. :-(

Comment 65

[aleth [:aleth]]

(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 ?

Comment 66

[Jorg K (CEST = GMT+2)]

(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.

Comment 67

[Jorg K (CEST = GMT+2)]

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.

Comment 68

[aleth [:aleth]]

(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.

Comment 69

[Jorg K (CEST = GMT+2)]

Well, did it a little differently since the above was nonsense ;-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1d6caf195e61

Comment 70

[:aceman]

(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’

Comment 71

[Jorg K (CEST = GMT+2)]

Attachment: 4th patch for Windows (MAPI fixed and ical perhaps fixed).

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

Comment 72

[alta88]

(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

Comment 73

[Jorg K (CEST = GMT+2)]

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 74

[Jorg K (CEST = GMT+2)]

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

Comment 75

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: WIP - real issues.

Needs to be applied first.

Comment 76

[Jorg K (CEST = GMT+2)]

Attachment: Part 3: WIP - syntactic issues.

Part 2 and Part 3 refreshed with Aceman's latest work from:
Real - https://hg.mozilla.org/try-comm-central/rev/4b8b8041abf3f1c8d1eba498ce95393c1fabb563
Syntactic - https://hg.mozilla.org/try-comm-central/rev/6b95983bad36057de296352fe47591b635f21358

Comment 77

[Jorg K (CEST = GMT+2)]

Attachment: Part 4: WIP - Windows

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

Comment 78

[:aceman]

(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.

Comment 79

[Jorg K (CEST = GMT+2)]

Attachment: Part 4: Windows

Maybe we can land this so whatever Aceman does will always compile on Windows.

Comment 80

[Stefan Sitter [:ssitter]]

Bug 1212068 still has a patch for the warnings that were previously reported on Windows. Maybe it helps with current bustage.

Comment 81

[Jorg K (CEST = GMT+2)]

Aceman has the hunks that relate to signed/unsigned from attachment 8670459 [details] [diff] [review] from bug 1212068 covered.

Comment 82

[:aceman]

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.

Comment 83

[:aceman]

Attachment: Part 2: WIP - real issues v2

Comment 84

[:aceman]

Attachment: Part 3: WIP - syntactic issues v2

Comment 85

[Jorg K (CEST = GMT+2)]

Aleth, your "win-reduced" won't work, you need the ical stuff, see:https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-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.

Comment 86

[aleth [:aleth]]

(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.

Comment 87

[Jorg K (CEST = GMT+2)]

Has anyone considered to ignore warnings in Mork like we do in LDAP for the time being?

Comment 88

[aleth [:aleth]]

Attachment: Part 5: Allow compiler warnings in libical [landed in comment 93]

Comment 89

[aleth [:aleth]]

Comment on attachment 8780821 [details] [diff] [review]
Part 4: Windows

lgtm. I'll let aceman decide how it fits in with his patch series.

Comment 90

[:aceman]

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).

Comment 91

[aleth [:aleth]]

(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 92

[:aceman]

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 93

[:aceman]

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

Comment 94

[Jorg K (CEST = GMT+2)]

Attachment: Part 4: Windows (v2) [landed in comment 104]

(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.

Comment 95

[Jorg K (CEST = GMT+2)]

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.

Comment 96

[:aceman]

(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.

Comment 97

[:aceman]

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.

Comment 98

[Jorg K (CEST = GMT+2)]

(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 99

[:aceman]

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.

Comment 100

[:aceman]

(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.

Comment 101

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #99)
> Attachment #8780846 [details] [diff] - Flags: review?([e-mail]) → review+
Please land or include in your try pushes as you deem fit ;-)

Comment 102

[:aceman]

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).

Comment 103

[Jorg K (CEST = GMT+2)]

(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 104

[:aceman]

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.

Comment 105

[:aceman]

Attachment: Part 2: real issues v3

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

Comment 106

[:aceman]

Attachment: Part 3: syntactic issues v3

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.

Comment 107

[Magnus Melin [:mkmelin]]

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 ;)

Comment 108

[Jorg K (CEST = GMT+2)]

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!!

Comment 109

[aleth [:aleth]]

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.

Comment 110

[:aceman]

(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 111

[Jorg K (CEST = GMT+2)]

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.

Comment 112

[Jorg K (CEST = GMT+2)]

As discussed on IRC:
You might as well change both these:
https://dxr.mozilla.org/comm-central/search?q=kABAIMInstantProperty&redirect=false

Comment 113

[:aceman]

(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.

Comment 114

[Frank-Rainer Grahl (:frg)]

Attachment: portable.patch

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 115

[:aceman]

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

Comment 116

[:aceman]

Attachment: Part 2: real issues v4

Updated final patch, with minor tweaks for OS X *AppleEncoding* files, thanks to aleth.

Comment 117

[:aceman]

Attachment: Part 3: syntactic issues v4 [landed in comment 126]

Final patch, makes OS X build (except mork).

Comment 118

[:aceman]

Attachment: Part 6: warnings in mork [landed in comment 130]

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++"

Comment 119

[aleth [:aleth]]

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 120

[aleth [:aleth]]

Comment on attachment 8780875 [details] [diff] [review]
Part 3: syntactic issues v4 [landed in comment 126]

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

Thanks!

Comment 121

[:aceman]

(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 122

[Jorg K (CEST = GMT+2)]

Comment on attachment 8780876 [details] [diff] [review]
Part 6: warnings in mork [landed in comment 130]

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

Comment 123

[Jorg K (CEST = GMT+2)]

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.

Comment 124

[:aceman]

Attachment: Part 2: real issues v4.1 [landed in comment 131]

Comment 125

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #122)
> Comment on attachment 8780876 [details] [diff] [review]
> Part 6: warnings in mork
> 
> Do we really need four reviewers here?
> Magnus, Aleth and myself have been through it.

The more the better :) But mork was never marked as reviewed yet.

Comment 126

[:aceman]

Comment on attachment 8780875 [details] [diff] [review]
Part 3: syntactic issues v4 [landed in comment 126]

https://hg.mozilla.org/comm-central/rev/28876b046ec271296611b271d6329937889dc694

Comment 127

[Jorg K (CEST = GMT+2)]

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 128

[Jorg K (CEST = GMT+2)]

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).

Comment 129

[Jorg K (CEST = GMT+2)]

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 130

[:aceman]

Comment on attachment 8780876 [details] [diff] [review]
Part 6: warnings in mork [landed in comment 130]

https://hg.mozilla.org/comm-central/rev/3e4c9e3a9099b0bb077255abb75463784ee929e5

Comment 131

[:aceman]

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

Comment 132

[:aceman]

All platforms building now and tests seem unaffected.
I think we are done here.
Thanks to aleth and jorgk for quick cooperation :)

Comment 133

[Intermittent Failures Robot]

5 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 5

Platform breakdown:
* osx-10-7: 2
* linux64: 2
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1294260&startday=2016-08-08&endday=2016-08-14&tree=all

Comment 134

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

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