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

RESOLVED FIXED in Thunderbird 3.1b2

Status

MailNews Core
Database
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Walter Meinl, Assigned: Walter Meinl)

Tracking

unspecified
Thunderbird 3.1b2
x86
OS/2

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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
(Assignee)

Updated

8 years ago
Blocks: 470329
Hardware: Other → x86
OS/2 is not a directly supported platform, but patches are accepted.

Comment 2

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 6

8 years ago
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.
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+
(Assignee)

Comment 8

8 years ago
Created attachment 432504 [details] [diff] [review]
updated to address reviewer's comment
Attachment #432455 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 years ago
status-thunderbird3.1: --- → beta2-fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b2
(Assignee)

Comment 11

8 years ago
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+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: comm-1.9.1
Checked in: http://hg.mozilla.org/releases/comm-1.9.1/rev/bea0763ec436
status-thunderbird3.0: --- → .5-fixed
Keywords: checkin-needed
Whiteboard: comm-1.9.1
You need to log in before you can comment on or make changes to this bug.