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
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.
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 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!
Created attachment 432504 [details] [diff] [review] updated to address reviewer's comment
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 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.