Closed Bug 549238 Opened 14 years ago Closed 14 years ago

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

Categories

(MailNews Core :: Database, defect)

x86
OS/2
defect
Not set
major

Tracking

(thunderbird3.1 beta2-fixed, thunderbird3.0 .5-fixed)

RESOLVED FIXED
Thunderbird 3.1b2
Tracking Status
thunderbird3.1 --- beta2-fixed
thunderbird3.0 --- .5-fixed

People

(Reporter: wuno, Assigned: wuno)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Blocks: 470329
Hardware: Other → x86
OS/2 is not a directly supported platform, but patches are accepted.
(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.
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.
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.
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.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → wuno
Status: NEW → ASSIGNED
Attachment #432455 - Flags: review?(bugmail)
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!
Attachment #432455 - Flags: review?(bugmail) → review+
Attachment #432455 - Attachment is obsolete: true
Keywords: checkin-needed
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
Attachment #432504 - Flags: superreview?(bugzilla)
Attachment #432504 - Flags: review+
Attachment #432504 - Flags: superreview?(bugzilla) → superreview+
Checked in: http://hg.mozilla.org/comm-central/rev/d7c84d29a71a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b2
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.
Attachment #432504 - Flags: approval-thunderbird3.0.5?
Attachment #432504 - Flags: approval-thunderbird3.0.5? → approval-thunderbird3.0.5+
Keywords: checkin-needed
Whiteboard: comm-1.9.1
Keywords: checkin-needed
Whiteboard: comm-1.9.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: