search

jarvis657 / snappy

Automatically exported from code.google.com/p/snappy
Other
0 stars 0 forks source link

Warning compiling with clang -Wheader-hygiene #70

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Turning snappy on inside a Chromium build with the clang compiler hits the 
error:

In file included from ../../third_party/snappy/src/snappy-internal.h:34:
../../third_party/snappy/src/snappy-stubs-internal.h:64:17:error: using 
namespace directive in global context in header [-Werror,-Wheader-hygiene]
using namespace std;
                ^
1 error generated.

More details about the flag: 
http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors#TO
C-Using-namespace-in-a-header

This is not a Chromium-specific check. We could suppress the warning but an 
upstream fix is preferable.

Original issue reported on code.google.com by jsbell@chromium.org on 27 Feb 2013 at 10:52

GoogleCodeExporter commented 9 years ago 
Unfortunately this is not so easy to fix; I know it's suboptimal, but this 
header is at least never exported to anything outside Snappy itself.

I assume it wouldn't be better if we did a bunch of “using std::foo;” 
instead of the using namespace?

Original comment by se...@google.com on 27 Feb 2013 at 10:55

GoogleCodeExporter commented 9 years ago 
Yes, I think the check is fairly naive. I was going to try that myself.

FWIW, with just a linux chromium build (i.e. not a full set of tests), and 
simply removing the "using namespace std;" declaration, I only had a handful of 
errors around std::min; and only needed to add "using std::min;" to get a 
compile.

Original comment by jsbell@chromium.org on 27 Feb 2013 at 11:02

GoogleCodeExporter commented 9 years ago 
Okay, a full configure/make pass later, this appears to be sufficient for a 
linux/clang build - but I'm not advocating this as the actual patch!:

//using namespace std;                                                          

using std::min;
using std::max;
using std::string;
#include <vector>
using std::vector;
#include <algorithm>
using std::nth_element;
using std::sort;
#include <iostream>
using std::cerr;
using std::endl;

Original comment by jsbell@chromium.org on 27 Feb 2013 at 11:12

GoogleCodeExporter commented 9 years ago 
Is this actually a lot better, though?

I understand the point of the warning. However, Snappy primarily exists for 
Google's code base, where things are a bit different (as I'm sure you know). 
I'm fine with some ugliness creeping through the cracks into the open-source 
version, as long as this is not a big problem for Chromium.

Original comment by se...@google.com on 27 Feb 2013 at 11:18

GoogleCodeExporter commented 9 years ago 
I'm sorry, I misread your question.

Replacing the "using namespace std;" with a bunch of "using std::foo;" *does* 
satisfy clang's -Wheader-hygiene warning. That would remove the need for a 
suppression.

Original comment by jsbell@chromium.org on 27 Feb 2013 at 11:18

GoogleCodeExporter commented 9 years ago 
and that using declarations will stay in the header? that means people will be 
able write const string& foo, vector<Blah> all over the place. While we try to 
prefix these with std:: in Chromium

Original comment by tfar...@chromium.org on 27 Feb 2013 at 11:21

GoogleCodeExporter commented 9 years ago 
This header is only used internally by snappy, so there is not much danger of 
the declarations leaking.

The high-level question is: do we suppress the warning on the Chromium side, or 
make an appropriate change on the snappy side. 

(I would not consider putting using std::foo; declarations in public header 
appropriate, but it's not a public header. That said, there are a very small 
number of source files and the appropriate usings std::foo; declarations could 
be added easily enough.)

Original comment by jsbell@chromium.org on 27 Feb 2013 at 11:25

GoogleCodeExporter commented 9 years ago 
Putting using std::foo; in the .cc file makes them leak into Google's code 
base. This means anyone changing the files in Google have to worry about one 
more thing, instead of containing the opensource-specific hacks in the 
-internal.h file.

I don't really think that changing “using namespace std;” into “using 
std::foo;” in the .h file is an appropriate fix. While it satisfies the 
warning right now, there's no guarantee it will in the future; the spirit of 
the warning would seem to indicate that “using std::foo;” is taboo too, 
right? That sounds like a short-term band-aid no better than just disabling the 
warning for that file.

Original comment by se...@google.com on 27 Feb 2013 at 11:30

GoogleCodeExporter commented 9 years ago 
I agree that putting using std::foo; in the header isn't a much better fix.

In chromium we use fully qualified names in headers, and use "using std::foo;" 
in implementation files where it improves readability.

Original comment by jsbell@chromium.org on 27 Feb 2013 at 11:32

GoogleCodeExporter commented 9 years ago 
So, I don't think we can remove this using namespace std; anytime soon. You're 
right in that snappy.cc only really needs std::min<>, and we can fix that, but 
snappy_unittest.cc wants tons of stuff around vector, string, sort, etc.

In short, I think this warning is a false positive _and_ hard to work around on 
our side (short of having tons of “using std::foo”, which is not going to 
be a long-term solution) -- the point is to not have using-declarations leak 
over to users of a header file, and here the header file is internal. So I'm 
closing this one as not-a-bug.

Original comment by se...@google.com on 14 Jun 2013 at 11:41

GoogleCodeExporter commented 9 years ago 
This is not a false positive. Having using statements in headers is crummy 
code. You can just put the using statement as is in the two cc files that need 
it. (I.e. say "using namespace std;" in your cc files, instead of the header.)

Original comment by thakis@chromium.org on 1 Aug 2014 at 8:43