Alternative inclusion of <math.h> or <cmath> which generate an error for isnan (Bug #3204)
| Status: | Done | Start date: | 2013-08-08 |
|---|---|---|---|
| Priority: | Normal | Due date: | |
| Assignee: |  Roman Donchenko |
% Done: | 0% |
| Category: | build/install | ||
| Target version: | 3.0-alpha | ||
| Affected version: | branch 'master' (2.9) | Operating System: | Windows |
| Difficulty: | Easy | HW Platform: | x86 |
| Pull request: |
Description
While trying to compile with mingw 4.7.2 (and ninja), compiling of motion_estimators.cpp will fail stating that isnan requires a std:: in front. However, adding it might break compiling in other platforms. Origin of the problem is that both <math.h> and <cmath> are used across the project. The first guarantees to put math functions in the global namespace, the second in the std:: namespace. Note that they are also allowed to put math functions in the other namespace, with an implementation dependent situation.
motion_estimators.cpp includes directly <math.h> but indirectly, through other includes, <cmath>. Inclusions of both <cmath> and <math.h> should be avoided. IMO, all #include <math.h> (which is probably deprecated, 57 instances across the project) should be changed in #include <cmath>, and std:: should be added where necessary.
Related discussion in Stack Overflow: http://stackoverflow.com/questions/18128899/is-isnan-in-the-std-namespace-more-in-general-when-is-std-necessary-optio
By the way, note that isnan is not supposed to be there in C++98.
Associated revisions
Merge pull request #3204 from parafin:ximea_unix-2.4
History
Updated by Victor Kocheganov over 11 years ago
Hello Claudio,
thank you for submitting the ticket.
Roma, could you please comment it?
- Category set to build/install
- Status changed from New to Open
- Difficulty set to Easy
- Target version set to 2.4.7
- Assignee set to Roman Donchenko
Updated by Roman Donchenko over 11 years ago
There are two issues at play here.
One is the inconsistent usage of <cmath> vs <math.h>, which I agree is kinda bad. I don't have time ATM to mass-replace <math.h> with <cmath> everywhere and then presumably insert std:: or a bunch of @using@s everywhere else, and I'm afraid it will be quite a voluminous change, especially in what used to be C code. If you wish, you can try to do it and submit a pull request.
Two is that this file uses isnan even though we have perfectly good cvIsNaN available. This should be easy to fix, of course. You can submit a pull request for this; if not - well, I'll get to it myself some time...
Updated by Ramiro Pereira de Magalhães over 11 years ago
I was taking a look at this one on branch 2.4 and looks like someone is almost done with this issue, right? I submited a pull request with a mass replace I did (https://github.com/Itseez/opencv/pull/1344), just in case I'm mistaken, but ATM there is not much left to do.
Updated by Roman Donchenko over 11 years ago
I'm closing this, because this particular issue in motion_estimators.cpp has been fixed by https://github.com/Itseez/opencv/pull/1253. The header inconsistency is more of a general nuisance than a concrete issue, so I don't think it should keep this open.
- Status changed from Open to Done
- Target version changed from 2.4.7 to 3.0-alpha
Updated by Ramiro Pereira de Magalhães over 11 years ago
Oh, no! I just found out that the issue has been closed but I worked for some time to solve it properly. Please see https://github.com/Itseez/opencv/pull/1350.
Well, I ask you the following: leave this issue open and see if the patch works fine. If it doesn't, then let us forget this sad waste of time and drop the atempt to fix it. Otherwise, integrate the patch to the OpenCV code base. What do you think? If it works, at least it will be one less nuisance for you do deal with.