549238 - [OS/2] build break in nsStopwatch.cpp due to -Werror=return-type

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a2pre) Gecko/20100220 SeaMonkey/2.1a1pre
Build Identifier: 

After -Werror=return-type was brought to c-c the OS/2 build breaks in nsStopwatch.
The warning emitted (before it was turned to an error):
E:/hg-src/hg/comm-1.9.1/mailnews/base/util/nsStopwatch.cpp: In static member function `static double nsStopwatch::GetRealTime()':
E:/hg-src/hg/comm-1.9.1/mailnews/base/util/nsStopwatch.cpp:132: warning: control reaches end of non-void function
E:/hg-src/hg/comm-1.9.1/mailnews/base/util/nsStopwatch.cpp: In static member function `static double nsStopwatch::GetCPUTime()':
E:/hg-src/hg/comm-1.9.1/mailnews/base/util/nsStopwatch.cpp:176: warning: control reaches end of non-void function
Same for c-c or comm-1.9.1.
These two functions contain ifdefs for xp_unix and xp_win, but return nothing for other platforms.


Reproducible: Always

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

OS/2 is not a directly supported platform, but patches are accepted.

Comment 2

[Peter Weilbacher]

(In reply to comment #1)
> OS/2 is not a directly supported platform, but patches are accepted.

OS/2 has been supported as a volunteer effort since ages, it's one of the tier-2 platforms listed on <https://developer.mozilla.org/en/Supported_build_configurations>. As you can see, the reporter is even listed there as platform owner.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Peter, I'm not sure of the intent of your message.  Ludo added me as a cc on this bug to make me aware of it and potentially solicit feedback; I closed the loop by responding on the bug.

I think your post is in agreement with mine?  OS/2 is not a tier-1 platform and is not directly supported by the Thunderbird development team responsible for this area of code.

Comment 4

[Walter Meinl]

OS/2 could probably use the UNIX bits here, at least it compiles and make xpcshell-tests in mailnews/db/gloda/test/ pass all ok.
Andrew, is any of the tests explicitly testing the stopwatch feature?
Second question, would it be a possible option to return in those two functions for not UNIX/Windows platforms NS_ERROR_NOT_IMPLEMENTED? I tried this and at least the compiler doesn't complain anymore.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

No unit test intentionally tests the stopwatch functionality; the tests actually try and take it out of the equation.  (The code that uses it is trying to infer the load on the rest of the system to maintain UI responsiveness, which is not something the unit tests care about or wants happening.)

I have no problem with a patch that returns NS_ERROR_NOT_IMPLEMENTED but if you think the UNIX bits work (it would not be surprising unless OS/2 goes out of its way to avoid looking POSIXy/UNIXy) it might make sense to just use them.

Comment 6

[Walter Meinl]

Attachment: patch

I added both, us (OS/2) to go the unix line and the "not implemented" return to implement a return type for the functions for platforms not yet included.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 432455 [details] [diff] [review]
patch

Your additional returns of NS_ERROR_NOT_IMPLEMENTED are in functions that return doubles.  That, of course, is only suitable for nsresult/NS_IMETHODIMP functions.

I don't think it's worth complicating the code for unknown unsupported platforms to conditionalize the appropriate functions.  Please just replace those lines with:
#error "nsStopwatch not supported on this platform".

r=asuth with that
Thanks!

Comment 8

[Walter Meinl]

Attachment: updated to address reviewer's comment

Comment 9

[Ludovic Hirlimann [:Usul]]

Comment on attachment 432504 [details] [diff] [review]
updated to address reviewer's comment

Carrying andrew's r+. As this is mail newscode requesting sr from Standard8

Comment 10

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/d7c84d29a71a

Comment 11

[Walter Meinl]

Comment on attachment 432504 [details] [diff] [review]
updated to address reviewer's comment

We would like to have it also for the 3.0.x series of TB.
Risk for main platforms is zero as nothing is changed for them.

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-1.9.1/rev/bea0763ec436