Last Comment Bug 549238 - [OS/2] build break in nsStopwatch.cpp due to -Werror=return-type
: [OS/2] build break in nsStopwatch.cpp due to -Werror=return-type
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: unspecified
: x86 OS/2
: -- major (vote)
: Thunderbird 3.1b2
Assigned To: Walter Meinl
:
Mentors:
Depends on:
Blocks: 470329
  Show dependency treegraph
 
Reported: 2010-02-28 13:49 PST by Walter Meinl
Modified: 2010-04-09 02:05 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
beta2-fixed
.5-fixed


Attachments
patch (1.68 KB, patch)
2010-03-14 15:59 PDT, Walter Meinl
bugmail: review+
Details | Diff | Splinter Review
updated to address reviewer's comment (1.71 KB, patch)
2010-03-14 23:52 PDT, Walter Meinl
ludovic: review+
standard8: superreview+
standard8: approval‑thunderbird3.0.5+
Details | Diff | Splinter Review

Description Walter Meinl 2010-02-28 13:49:58 PST
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] 2010-03-12 08:32:14 PST
OS/2 is not a directly supported platform, but patches are accepted.
Comment 2 Peter Weilbacher 2010-03-13 14:18:55 PST
(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] 2010-03-14 12:04:52 PDT
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 2010-03-14 13:40:56 PDT
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] 2010-03-14 14:21:35 PDT
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 2010-03-14 15:59:08 PDT
Created attachment 432455 [details] [diff] [review]
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] 2010-03-14 20:46:02 PDT
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 2010-03-14 23:52:19 PDT
Created attachment 432504 [details] [diff] [review]
updated to address reviewer's comment
Comment 9 Ludovic Hirlimann [:Usul] 2010-03-15 02:55:18 PDT
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) 2010-03-16 13:31:11 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/d7c84d29a71a
Comment 11 Walter Meinl 2010-03-21 03:25:21 PDT
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) 2010-04-09 02:04:37 PDT
Checked in: http://hg.mozilla.org/releases/comm-1.9.1/rev/bea0763ec436

Note You need to log in before you can comment on or make changes to this bug.