Summary: | Colliding isinf/isnan between C99 and C++11 with GCC >=4.6 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xan Lopez <xan.lopez> | ||||||||||||
Component: | Tools / Tests | Assignee: | Allan Sandfeld Jensen <allan.jensen> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | allan.jensen, andersca, ap, buildbot, darin, dbates, evan, menard, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 86465 | ||||||||||||||
Attachments: |
|
Description
Xan Lopez
2011-04-22 15:28:24 PDT
CCing Darin, Daniel has told me he has worked on this area before. Created attachment 90789 [details]
namespacestdisnan.diff
Before we do bigger changes, this is the minimal patch needed to make things compile under GCC 4.6.0 + C++0x.
Attachment 90789 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/html/NumberInputType.cpp:51: Use 'using namespace std;' instead of 'using std::numeric_limits;'. [build/using_std] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 90792 [details]
namespacestdisnan.diff
OK style bot, have it your way.
Comment on attachment 90792 [details] namespacestdisnan.diff View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review > Source/JavaScriptCore/ChangeLog:9 > + Don't use "using namespace std;" in files where isnan or isinf are > + used, since it can lead to ambiguity in C++0x compilers. See the This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution. Is this a gcc bug, or expected behavior? (In reply to comment #5) > (From update of attachment 90792 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + Don't use "using namespace std;" in files where isnan or isinf are > > + used, since it can lead to ambiguity in C++0x compilers. See the > > This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution. I can't really think of any, but there might be. My proposed long-term solution of getting rid of math.h and always using std:: in front of the functions also requires not to use "using namespace std;", at least in the files where isnan/isinf are called, so it wouldn't be a huge improvement in that regard, I suppose. > > Is this a gcc bug, or expected behavior? The spec, AFAICU, leaves undefined whether or not the functions are also present in the global namespace (on top of being in std::), so I don't think it's a bug but just the way it happens to be on GCC. For details see section 17.6.1.2 (4) of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf It seems the core goal is that WebKit wants to use a namespace-unqualified isinf/isnan. Can't we hack the headers to make this just work? E.g. in C++0x mode, in MathExtras.h, #include the file that might define the C99 isinf. Then test for it with #ifdef, and if it's *not* present do the "using" line to get the std:: one. That way you always exit MathExtras.h with at most one isinf in scope. Comment on attachment 90792 [details] namespacestdisnan.diff View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review >>> Source/JavaScriptCore/ChangeLog:9 >>> + used, since it can lead to ambiguity in C++0x compilers. See the >> >> This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution. >> >> Is this a gcc bug, or expected behavior? > > I can't really think of any, but there might be. My proposed long-term solution of getting rid of math.h and always using std:: in front of the functions also requires not to use "using namespace std;", at least in the files where isnan/isinf are called, so it wouldn't be a huge improvement in that regard, I suppose. The right thing to do is to find a fix in MathExtras.h. The goal is to be able to call isinf and isnan directly; there must be way to do that. Please do *not* start making these changes to all the files using MathExtras.h. Created attachment 92342 [details]
Another patch
(In reply to comment #8) > (From update of attachment 90792 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review > > >>> Source/JavaScriptCore/ChangeLog:9 > >>> + used, since it can lead to ambiguity in C++0x compilers. See the > >> > >> This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution. > >> > >> Is this a gcc bug, or expected behavior? > > > > I can't really think of any, but there might be. My proposed long-term solution of getting rid of math.h and always using std:: in front of the functions also requires not to use "using namespace std;", at least in the files where isnan/isinf are called, so it wouldn't be a huge improvement in that regard, I suppose. > > The right thing to do is to find a fix in MathExtras.h. The goal is to be able to call isinf and isnan directly; there must be way to do that. > > Please do *not* start making these changes to all the files using MathExtras.h. Hi Darin, If you look at my approach it limits the modifications but there is still an issue. I have to specify std in some places otherwise I have : ../../../webkit/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1109:21: error: call of overloaded 'isnan(double&)' is ambiguous ../../../webkit/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1109:21: note: candidates are: /usr/include/bits/mathcalls.h:235:12: note: int isnan(double) /usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/../../../../include/c++/4.6.0/cmath:558:3: note: bool std::isnan(long double) /usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/../../../../include/c++/4.6.0/cmath:554:3: note: bool std::isnan(double) /usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/../../../../include/c++/4.6.0/cmath:550:3: note: bool std::isnan(float) because /usr/include/bits/mathcalls.h:235:12: note: int isnan(double) is in the global namespace and in a if condition it becomes ambiguous. Btw I didn't bother doing a changelog. It appears at least one of those files has a "using namespace std" in it. Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:39:using namespace std; I wonder how hard it would be to remove that. Unfortunately, there are 392 instances of 'using namespace std' in the WebKit codebase. Attachment 92342 [details] did not build on win: Build output: http://queues.webkit.org/results/8557732 (In reply to comment #11) > It appears at least one of those files has a "using namespace std" in it. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:39:using namespace std; > > I wonder how hard it would be to remove that. > > Unfortunately, there are 392 instances of 'using namespace std' in the WebKit codebase. A solution could be to get rid of using namespace std and fix the code accordingly and when isnan and friends are in the std then it's fine and we just namespace the WebKit implementation of isnan and friends into "std". Globally using namespace std always lead to name conflicts at some points (based on my little experience). (In reply to comment #13) > A solution could be to get rid of using namespace std We don’t want to do this for the whole project just because of the math headers being screwed up on some platform. There should not be a isnan function both in the global namespace and in namespace std on any platform. Platforms that have that have a broken C++ library and we should find a workaround for that brokenness. > Globally using namespace std always lead to name conflicts at some points (based on my little experience). What do you mean by “globally using”? Are you saying no one can ever use “using namespace std”? That does not correspond to my experience. (In reply to comment #14) > (In reply to comment #13) > > A solution could be to get rid of using namespace std > > We don’t want to do this for the whole project just because of the math headers being screwed up on some platform. There should not be a isnan function both in the global namespace and in namespace std on any platform. Platforms that have that have a broken C++ library and we should find a workaround for that brokenness. Well this I agree, it's stupid. > > > Globally using namespace std always lead to name conflicts at some points (based on my little experience). > > What do you mean by “globally using”? Are you saying no one can ever use “using namespace std”? That does not correspond to my experience. I'm saying that if you do : using namespace std; using namespace other; you may have name collisions. That's why I always used std::X but I'm fine with what you say. I'll try to come up with an acceptable workaround. (In reply to comment #15) > I'm saying that if you do : > > using namespace std; > using namespace other; > > you may have name collisions. Yes, exactly. And if you do, then you can’t use using namespace for both. But that’s not what’s happening here. (In reply to comment #16) > (In reply to comment #15) > > I'm saying that if you do : > > > > using namespace std; > > using namespace other; > > > > you may have name collisions. > > Yes, exactly. And if you do, then you can’t use using namespace for both. > > But that’s not what’s happening here. So I investigated a bit with a simple example like this : #include <stdlib.h> #include <cmath> #include <stdio.h> using namespace std; int main(int argc, char** argv) { double number = 0; if (isnan(number)) { printf("Nan\n"); } return 0; } and to compile call g++ main.cpp -std=c++0x -std=gnu++0x -o test. It reproduces the issue. Now if I turn off the c++0x support it compiles fine. The real issue is that the c++0x standard removes the prohibition on C++ headers declaring C names in the global namespace. Our problem here (even in the example) is that indirectly math.h is included therefore the error comes. I'm not really sure how the compiler can solve that. The float overloads seems new in gcc 4.6.0 that's why it only fails with 4.6.0. Yes, clearly a library bug. We need to work around it. The GCC people got this wrong. That result can’t possibly be correct! (In reply to comment #18) > Yes, clearly a library bug. We need to work around it. The GCC people got this wrong. That result can’t possibly be correct! http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48891 Just so that they help us... See bug #86465 for the same problem with GCC 4.7, but solved. Not that many files needed to have 'using namespace std' removed, and the few that did always only used std::min, std::max or std::numeric_limits. My patch in bug #86465, solves the problem for the problematic files, but moving forward I would suggest adding: using std::min using std::max using std::numeric_limits to MathExtra.h so many files might avoid having to include the whole std namespace. Also note that the style-checker currently is trying to enforce 'using namespace std' (why??) and thus trying to make the situation worse instead of better. I hadn't taken GCC 4.6 into account when making the patch for GCC 4.7. I suggest making the check in MathExtra.h into: #if !COMPILER(MSVC) && !COMPILER(RVCT) && !OS(SOLARIS) using std::isfinite; #if !COMPILER_SUPPORTS(CXX_ISINF_ISNAN) using std::isinf; using std::isnan; #endif using std::signbit; #endif And setting WTF_COMPILER_SUPPORTS_CXX_ISINF_ISNAN for compilers that has isinf and isnan in the global namespace. MathExtras.h is supposed to deal with functions from <math.h>. Since min, max, and numeric_limits are not from that file, I don’t think they belong in here. Created attachment 142978 [details]
Patch
Attachment 142978 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Co..." exit_code: 1
Source/WTF/wtf/MathExtras.h:292: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WTF/wtf/MathExtras.h:298: Use 'using namespace std;' instead of 'using std::wtf_isinf;'. [build/using_std] [4]
Source/WTF/wtf/MathExtras.h:299: Use 'using namespace std;' instead of 'using std::wtf_isnan;'. [build/using_std] [4]
Total errors found: 3 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 142982 [details]
Patch
Comment on attachment 142982 [details] Patch Clearing flags on attachment: 142982 Committed r117779: <http://trac.webkit.org/changeset/117779> All reviewed patches have been landed. Closing bug. This broke GCC build for some versions, bug 88721. |