How to reproduce: g++ -c foo.C Compiling the attached code using g++-4.2 generates the warning foo.C:11: warning: 'foo::bar' has a base '<unnamed>::gazonk' whose type uses the anonymous namespace I fail to see why this construct should be warned about
Created attachment 12389 [details] foo.C - testcase
As I understand it, the warning is because if you have the definition of class foo::bar, you will be violating ODR for foo.
Yes. If foo is used in multiple translation units, it violates the ODR because gazonk is a different type in each translation unit.
I still fail to understand why this would inherently violate the ODR. I agree with Andrew that if more than one translation unit sees the defintion of foo::bar then it will violate the ODR but if only one translation unit ever sees the definition of foo::bar then I see no problem with this. Externally foo::bar is exposed only as a undefined inner class. About the only thing you could use it for would be as a pointer target.
Yes, sorry, I should have said if foo::bar is used in multiple TUs, rather than foo itself. If foo::bar is defined in only one TU, and is used as an opaque type in other TUs, you're fine. Perhaps we should suppress this warning in the main input file.
Confirmed. The code makes sense and we shouldn't unconditionally warn. W.
Any news on this? It's really annoying if you've many pimpls which often use anonymous namespace.
The following patch addresses the problem. Do we want to add a flag to control this ? --- a/gcc/cp/decl2.c Mon Apr 02 15:48:00 2007 +0000 +++ b/gcc/cp/decl2.c Tue Apr 03 22:45:49 2007 +0000 @@ -1860,9 +1860,13 @@ constrain_class_visibility (tree type) int subvis = type_visibility (ftype); if (subvis == VISIBILITY_ANON) - warning (0, "\ + { + if (strcmp (main_input_filename, + DECL_SOURCE_FILE (TYPE_MAIN_DECL (ftype)))) + warning (0, "\ %qT has a field %qD whose type uses the anonymous namespace", - type, t); + type, t); + } else if (IS_AGGR_TYPE (ftype) && vis < VISIBILITY_HIDDEN && subvis >= VISIBILITY_HIDDEN) @@ -1877,9 +1881,13 @@ constrain_class_visibility (tree type) int subvis = type_visibility (TREE_TYPE (t)); if (subvis == VISIBILITY_ANON) - warning (0, "\ + { + if (strcmp (main_input_filename, + DECL_SOURCE_FILE (TYPE_MAIN_DECL (TREE_TYPE (t))))) + warning (0, "\ %qT has a base %qT whose type uses the anonymous namespace", - type, TREE_TYPE (t)); + type, TREE_TYPE (t)); + } else if (vis < VISIBILITY_HIDDEN && subvis >= VISIBILITY_HIDDEN) warning (OPT_Wattributes, "\
(In reply to comment #8) > The following patch addresses the problem. > Do we want to add a flag to control this ? Except if you look at the file name, you could get a case where you are no longer warning about.
(In reply to comment #9) > (In reply to comment #8) > > The following patch addresses the problem. > > Do we want to add a flag to control this ? > > Except if you look at the file name, you could get a case where you are no > longer warning about. That's why I asked if we want to add a flag to enable/disable this - although I think my patch is a reasonable compromise as a default, it's not perfect (it can never be perfect unless we do whole-program analysis). The patch prevents the warning IF the anonymous namespace was in the same main source file that it's currently compiling (e.g. *.cc file). I think this covers bulk of the false positives with very small number of the false negatives.
(In reply to comment #5) > Yes, sorry, I should have said if foo::bar is used in multiple TUs, rather than > foo itself. If foo::bar is defined in only one TU, and is used as an opaque > type in other TUs, you're fine. > > Perhaps we should suppress this warning in the main input file. The only remaining insecurity is when the same source is compiled twice in the same program or when a source is also include. These cases are probably rare enough to not warrant an option.
Subject: Bug number PR 29365 A patch for this bug has been added to the patch tracker. The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-04/msg00140.html
Subject: Bug 29365 Author: spark Date: Mon Apr 16 17:49:02 2007 New Revision: 123879 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=123879 Log: gcc/ChangeLog: 2007-04-16 Seongbae Park <seongbae.park@gmail.com> PR c++/29365 * cp/decl2.c (constrain_class_visibility): Do not warn about the use of anonymous namespace in the main input file. gcc/testsuite/ChangeLog: 2007-04-16 Seongbae Park <seongbae.park@gmail.com> PR c++/29365 Testcase for c++ anonymous namespace warning * g++.dg/warn/anonymous-namespace-1.C: New test * g++.dg/warn/anonymous-namespace-1.h: New test Added: trunk/gcc/testsuite/g++.dg/warn/anonymous-namespace-1.C trunk/gcc/testsuite/g++.dg/warn/anonymous-namespace-1.h Modified: trunk/gcc/ChangeLog trunk/gcc/cp/decl2.c trunk/gcc/testsuite/ChangeLog
Fixed.
I think there are still some kind of problem. If I try to compile bar.C using g++ -c bar.C then where g++ is g++ (GCC) 4.3.0 20070416 (experimental) (Hm, I'd wish for a revision number in there somewhere) then I get an ICE:
Created attachment 13375 [details] Test showing that the current fix causes an ICE
I'm testing the following patch (which fixes the ICE). It turned out to be a latent bug in the warning code not handling pointer types correctly. Index: cp/decl2.c =================================================================== --- cp/decl2.c (revision 123879) +++ cp/decl2.c (working copy) @@ -1856,7 +1856,7 @@ constrain_class_visibility (tree type) for (t = TYPE_FIELDS (type); t; t = TREE_CHAIN (t)) if (TREE_CODE (t) == FIELD_DECL && TREE_TYPE (t) != error_mark_node) { - tree ftype = strip_array_types (TREE_TYPE (t)); + tree ftype = strip_pointer_or_array_types (TREE_TYPE (t)); int subvis = type_visibility (ftype); if (subvis == VISIBILITY_ANON) Index: c-common.c =================================================================== --- c-common.c (revision 123879) +++ c-common.c (working copy) @@ -3894,6 +3894,15 @@ strip_pointer_operator (tree t) return t; } +/* Recursively remove pointer or array type from TYPE. */ +tree +strip_pointer_or_array_types (tree t) +{ + while (TREE_CODE (t) == ARRAY_TYPE || POINTER_TYPE_P (t)) + t = TREE_TYPE (t); + return t; +} + /* Used to compare case labels. K1 and K2 are actually tree nodes representing case labels, or NULL_TREE for a `default' label. Returns -1 if K1 is ordered before K2, -1 if K1 is ordered after Index: c-common.h =================================================================== --- c-common.h (revision 123879) +++ c-common.h (working copy) @@ -727,6 +727,7 @@ extern bool c_promoting_integer_type_p ( extern int self_promoting_args_p (tree); extern tree strip_array_types (tree); extern tree strip_pointer_operator (tree); +extern tree strip_pointer_or_array_types (tree); extern HOST_WIDE_INT c_common_to_target_charset (HOST_WIDE_INT); /* This is the basic parsing function. */
(In reply to comment #14) > Fixed. will it be backported to 4.2 branch?
Subject: Re: Unnecessary anonymous namespace warnings Yes, the latest patch seems to solve the problem with the ICE for me as well. Thanks.
It looks like this will not be backported to 4.2 as its not strictly a regression. See <http://gcc.gnu.org/ml/gcc/2007-05/msg00067.html>. If you use this sort of pimpl and anonymous namespaces or similar, and you want to use -Werror, you'll need to patch your own sources for now.
Created attachment 13525 [details] another testcase.
with patched 4.2 i still get a warning: zoo.cpp:3: warning: ‘X’ has a field ‘X::pimpl_’ whose type uses the anonymous namespace
Confirmed. I'm working on a fix. This is due to template instantiations marked as anonymous.
(In reply to comment #23) > Confirmed. I'm working on a fix. > This is due to template instantiations marked as anonymous. any news?
(In reply to comment #24) > any news? I have (or had, since I seem to have lost it) a patch that will prevent the invalid warning for the template case, but it will effectively prevent the valid warning for other template case as well, and I haven't found an easy way to make it accurate yet. I'll get back to this once dataflow branch is integrated and stablized in the mainline, unless someone else fixes this before then.
(In reply to comment #25) > (In reply to comment #24) > > any news? > > I have (or had, since I seem to have lost it) a patch that will prevent the > invalid warning for the template case, but it will effectively prevent the > valid warning for other template case as well, and I haven't found an easy way > to make it accurate yet. I'll get back to this once dataflow branch is > integrated and stablized in the mainline, unless someone else fixes this before > then. 4.2.1 won't have dataflow merge. we need a fix, or switch to disable this broken feature.
> 4.2.1 won't have dataflow merge. we need a fix, or switch to disable > this broken feature. Seongbae knows dataflow will not be in 4.2, what he is trying to say, which I think you misunderstood, is that he will be working on this again after the merge happens as he is busy with that. It is not like GCC is a closed source program either, you can try to make a fix for the issue too.
(In reply to comment #27) > It is not like GCC is a closed source program either, > you can try to make a fix for the issue too. Andrew, real world is not so simple ;) some time ago Manuel López-Ibáñez helps me create official patch: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=7302#c22 patch was submitted: http://gcc.gnu.org/ml/gcc-patches/2007-03/msg01347.html added to tracker: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=7302#c23 and pinged: http://gcc.gnu.org/ml/gcc-patches/2007-04/msg00604.html http://gcc.gnu.org/ml/gcc-patches/2007-05/msg00110.html c.a. 3 months and no effects for tested trivial patch. no accecpt, no reject, no commit, no fun. in current situation i feel no reason to working on another patch while the GCC-Team redirect external patches to /dev/null. moreover, gcc lists are full of patch-ping^N-noeffects. i think people are not motivated to cooperate with GCC-Team.
(In reply to comment #28) > (In reply to comment #27) > > > It is not like GCC is a closed source program either, > > you can try to make a fix for the issue too. > > Andrew, real world is not so simple ;) [snip] > c.a. 3 months and no effects for tested trivial patch. > no accecpt, no reject, no commit, no fun. > in current situation i feel no reason to working > on another patch while the GCC-Team redirect external > patches to /dev/null. It is true that patches get lost sometimes. But that is true for everybody, not only for external patches. If you check http://www.dberlin.org/patches/patches/list, there are patches pending since 2006 from top maintainers such as Andrew Pinski. I guess they forgot about them or just don't have time to keep pinging them. (That is why the patch tracker is such a great idea, I may decide to take his patch, update it and ping it myself). > moreover, gcc lists are full of patch-ping^N-noeffects. > i think people are not motivated to cooperate with GCC-Team. That may be true. (I think it is to some extent). But there are also lots of patches that get 5 pings and then get reviewed and committed. Sending a ping for a patch takes less than 1 second (at least with gmail). Sometimes you need to ping individual maintainers (CC them) one by one until someone replies (in your case, perhaps diagnostics maintainer and C++ front-end ones) but without being annoying because that will put reviewers off. Unfortunately, this penalises the sporadic contributor that wants to commit a patch before sending another. You don't need to wait. If you have 12 patches waiting approval, ping all of them. (It will make more noise than a lonely ping from time to time).
The following patch will essentially disable the warning for template instantiations in the anonymous namespace. Index: gcc/cp/decl2.c =================================================================== --- gcc/cp/decl2.c (revision 125999) +++ gcc/cp/decl2.c (working copy) @@ -1850,7 +1850,9 @@ constrain_class_visibility (tree type) if (subvis == VISIBILITY_ANON) { if (strcmp (main_input_filename, - DECL_SOURCE_FILE (TYPE_MAIN_DECL (ftype)))) + DECL_SOURCE_FILE (TYPE_MAIN_DECL (ftype))) + && !(CLASS_TYPE_P (ftype) + && CLASSTYPE_USE_TEMPLATE (ftype))) warning (0, "\ %qT has a field %qD whose type uses the anonymous namespace", type, t); @@ -1871,7 +1873,9 @@ constrain_class_visibility (tree type) if (subvis == VISIBILITY_ANON) { if (strcmp (main_input_filename, - DECL_SOURCE_FILE (TYPE_MAIN_DECL (TREE_TYPE (t))))) + DECL_SOURCE_FILE (TYPE_MAIN_DECL (TREE_TYPE (t)))) + && !(CLASS_TYPE_P (TREE_TYPE (t)) + && CLASSTYPE_USE_TEMPLATE (TREE_TYPE (t)))) warning (0, "\ %qT has a base %qT whose type uses the anonymous namespace", type, TREE_TYPE (t)); I don't know whether it's possible to do this warning accurately, as I can't figure out how to recover where the template instantiation has happened at this point in compilation. DECL_SOURCE_FILE (TYPE_MAIN_DECL (ftype)) points to the source file of the declaration of the template itself, not where the template instantiation happened.
Subject: Bug 29365 Author: jason Date: Wed Aug 22 17:23:37 2007 New Revision: 127711 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=127711 Log: PR c++/29365 * pt.c (outermost_tinst_level): New function. * lex.c (in_main_input_context): New function. * decl2.c (constrain_class_visibility): Use it to avoid warning about uses of the anonymous namespace in the main input file. Added: trunk/gcc/testsuite/g++.dg/warn/anonymous-namespace-3.C trunk/gcc/testsuite/g++.dg/warn/anonymous-namespace-3.h Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/cp-tree.h trunk/gcc/cp/decl2.c trunk/gcc/cp/lex.c trunk/gcc/cp/pt.c trunk/gcc/testsuite/g++.dg/warn/anonymous-namespace-1.C trunk/gcc/testsuite/g++.dg/warn/anonymous-namespace-2.C
Subject: Bug 29365 Author: jason Date: Wed Aug 22 20:40:30 2007 New Revision: 127716 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=127716 Log: PR c++/29365 * pt.c (outermost_tinst_level): New function. * lex.c (in_main_input_context): New function. * decl2.c (constrain_class_visibility): Use it to avoid warning about uses of the anonymous namespace in the main input file. Modified: branches/gcc-4_2-branch/gcc/cp/ChangeLog branches/gcc-4_2-branch/gcc/cp/cp-tree.h branches/gcc-4_2-branch/gcc/cp/decl2.c branches/gcc-4_2-branch/gcc/cp/lex.c branches/gcc-4_2-branch/gcc/cp/pt.c
(In reply to comment #33) > Fixed. > one more testcase: $ cat X.hpp namespace { class Impl; } struct X { ~X(); Impl* pimpl_; }; $ cat X.cpp #include "X.hpp" X::~X() { } $ g++ -Wall -c X.cpp In file included from X.cpp:1: X.hpp:3: warning: 'X' has a field 'X::pimpl_' whose type uses the anonymous namespace gcc version 4.2.2 20070828
Subject: Re: Unnecessary anonymous namespace warnings On 28 Aug 2007 19:40:14 -0000, pluto at agmk dot net <gcc-bugzilla@gcc.gnu.org> wrote: > > > ------- Comment #34 from pluto at agmk dot net 2007-08-28 19:40 ------- > (In reply to comment #33) > > Fixed. > > > > one more testcase: > > $ cat X.hpp > namespace { class Impl; } > struct X > { > ~X(); > Impl* pimpl_; > }; > > $ cat X.cpp > #include "X.hpp" > X::~X() > { > } In this case, the warning is correct as Impl will be different in each Translation Unit so X can never be the same between two of them. -- Pinski
(In reply to comment #35) > In this case, the warning is correct as Impl will be different in each > Translation Unit so X can never be the same between two of them. That's what I thought too at first, but the request is valid anyway as long as you never use the pointer to the class from anywhere except the one implementation file in which the class is defined. Never using means not even passing around the pointer, i.e. the class better not has an inline function in the header file in which the pointer is used. However, as long as the entire handling of the pointer is inside a single TU, nothing is wrong with the code. Now, whether that's good style is clearly a different matter... W.
Subject: Re: Unnecessary anonymous namespace warnings On 28 Aug 2007 21:41:05 -0000, bangerth at dealii dot org <gcc-bugzilla@gcc.gnu.org> wrote: > That's what I thought too at first, but the request is valid anyway as long > as you never use the pointer to the class from anywhere except the one > implementation file in which the class is defined. No it is not, it is still violating One definition rule as struct X will have a different member type for pimpl_ in each TU (this violates the whole idea of types being exported). This is going to be true no matter what, even if you change the warning. Now you can use a "void*" to get around this issue and should solve the issue and not violate the C++ ODR. This warning is not about style, it is about warning when you are most likely going to violat C++'s One definition rule with anonymous namespaces and you do violate it here as explained above. Thanks, Andrew Pinski
(In reply to comment #37) > No it is not, it is still violating One definition rule as struct X > will have a different member type for pimpl_ in each TU (this violates > the whole idea of types being exported). It is a good question in itself whether pimpl_ has a type at all -- it's a pointer to an incomplete type in any case :-) > This warning is not about style, it is about warning when you are most > likely going to violat C++'s One definition rule with anonymous > namespaces and you do violate it here as explained above. Yes, I agree that whoever uses this idiom is walking a very fine line indeed, and most definitely is violating at least good style. I can certainly live with a warning. W.
Subject: Re: Unnecessary anonymous namespace warnings On 29 Aug 2007 03:15:04 -0000, bangerth at dealii dot org <gcc-bugzilla@gcc.gnu.org> wrote: > It is a good question in itself whether pimpl_ has a type at all -- it's a > pointer to an incomplete type in any case :-) All types in C++ are exported (well except for anonymous namespace types) including incomplete types. So the following two TUs are invalid when combined together. TU1: extern struct a *b; TU2: extern struct c *b; It does not matter in C++ if it is an incomplete type because the type is based on the name rather than compatibility rules (like what is done for C). So again this warning is correct based on the One definition rule. Thanks, Andrew Pinski
Subject: Re: Unnecessary anonymous namespace warnings "Andrew Pinski" <pinskia@gmail.com> writes: | On 29 Aug 2007 03:15:04 -0000, bangerth at dealii dot org | <gcc-bugzilla@gcc.gnu.org> wrote: | > It is a good question in itself whether pimpl_ has a type at all -- it's a | > pointer to an incomplete type in any case :-) | | All types in C++ are exported (well except for anonymous namespace | types) including incomplete types. What does not could mean in C++? -- Gaby
Subject: Bug 29365 Author: jason Date: Mon Oct 22 18:12:36 2007 New Revision: 129554 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=129554 Log: PR c++/32470 * name-lookup.c (push_namespace_with_attrs): Fold back into... (push_namespace): Here. (handle_namespace_attrs): New fn for the attr code. (leave_scope): Don't pop_visibility. * name-lookup.h (struct cp_binding_level): Remove has_visibility. * parser.c (cp_parser_namespace_definition): Call handle_namespace_attrs and pop_visibility as appropriate. PR c++/33094 * decl.c (make_rtl_for_nonlocal_decl): It's ok for a member constant to not have DECL_EXTERNAL if it's file-local. * decl2.c (get_guard): Copy visibility from the guarded variable. PR c++/29365 * pt.c (outermost_tinst_level): New function. * lex.c (in_main_input_context): New function. * decl2.c (constrain_class_visibility): Use it to avoid warning about uses of the anonymous namespace in the main input file. Modified: branches/redhat/gcc-4_1-branch/gcc/cp/ChangeLog branches/redhat/gcc-4_1-branch/gcc/cp/cp-tree.h branches/redhat/gcc-4_1-branch/gcc/cp/decl.c branches/redhat/gcc-4_1-branch/gcc/cp/decl2.c branches/redhat/gcc-4_1-branch/gcc/cp/lex.c branches/redhat/gcc-4_1-branch/gcc/cp/name-lookup.c branches/redhat/gcc-4_1-branch/gcc/cp/name-lookup.h branches/redhat/gcc-4_1-branch/gcc/cp/parser.c branches/redhat/gcc-4_1-branch/gcc/cp/pt.c