This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: More informative ODR warnings


> 
> I can't find the code for the SampleFormat thing, but the rest of them
> look like ODR violations to me.

I think it is some define renaming the field, I was also puzled by this one.
> 
> > /aux/hubicka/firefox/netwerk/sctp/datachannel/DataChannel.h:64:0: warning: field âmSpaâ (of type âstruct BufferedMsgâ) violates one definition rule   [-Wodr]
> >    struct sctp_sendv_spa *mSpa;
> >  ^
> > ../../../../dist/include/mozilla/net/DataChannel.h:64:0: note: a field of same name but different type is defined in another translation unit
> > /aux/hubicka/firefox/netwerk/streamconv/converters/nsMultiMixedConv.cpp:466:0: warning: field âmBufferâ (of type âstruct AutoFreeâ) violates one definition rule   [-Wodr]
> >    char *mBuffer;
> >  ^
> > /aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp:307:0: note: a field with different name is defined in another translation unit
> 
> it seems to me this doesn't get at the real issue that the type names
> are the same but the fields are different. maybe "a type of the same
> name with different fields defined here"?

This is what I print when I see name mismatch. It usually means completely different structure/field.
It speaks of fields names rather than type names.  Better wording is welcome.
> 
> >    void *mPtr;
> >  ^
> > lto1: note: Conflicting compilation units: /aux/hubicka/firefox/dom/base/nsJSEnvironment.cpp and /aux/hubicka/build-firefox491-inst4/netwerk/streamconv/converters/Unified_cpp_netwerk_streamconv_converters0.cpp
> > ../../dist/include/zipstruct.h:47:25: warning: field âMOZ_Z_crc32â (of type âstruct ZipCentral_â) violates one definition rule   [-Wodr]
> > ../../dist/include/zipstruct.h:47:0: note: a field with different name is defined in another translation unit
> 
> is it worth poking into why this case doesn't have the caret stuff?
> 
> > +  /* In Firefox it is a common bug to have same types but in
> > +     different namespaces.  Be a bit more informative on
> > +     this.  */
> 
> hm? I make no claim to being a spec lawyer, but I thought something like
> namespace foo {
>   struct bar { int x; };
> }
> 
> namespace baz {
> 	struct bar { float b; };
> 	}
> was legal? or are you refering to some other construct?

This is legal as long as you don't define

struct foo {
  struct bar a;
 }

Where foo is same name but bar is once from foo and once from baz.

There are cases where "struct bar" is in an namespace in one unit, but
it is toplevel in another.
> 
> > +  if (TYPE_CONTEXT (t1) && TYPE_CONTEXT (t2)
> > +      && (((TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL)
> > +	    != (TREE_CODE (TYPE_CONTEXT (t2)) == NAMESPACE_DECL))
> > +	   || (TREE_CODE (TYPE_CONTEXT (t1)) == NAMESPACE_DECL
> > +	       && (DECL_NAME (TYPE_CONTEXT (t1)) !=
> > +		   DECL_NAME (TYPE_CONTEXT (t2))))))
> 
> imho that would be a lot more readable with some temporary variables,
> but shrug

What? stepping away from old-school GCC writting style???
> 
> > +    inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
> > +	    "type %qT should match type %qT but is defined "
> > +	    "in different namespace  ",
> > +	    t1, t2);
> > +  else
> > +    inform (DECL_SOURCE_LOCATION (TYPE_NAME (t1)),
> > +	    "type %qT should match type %qT",
> > +	    t1, t2);
> > +  inform (DECL_SOURCE_LOCATION (TYPE_NAME (t2)),
> > +	  "the incompatible type is defined here");
> 
> I didn't see any of these come up in the list of warnings at the
> beginning of your mail?

I get it here:
../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: warning: field ï~@~Xsample_format_ï~@~Y (of type ï~@~Xstruct AudioDecoderConfigï~@~Y) violates one definition rule   [-Wodr]
../../../dist/include/mp4_demuxer/audio_decoder_config.h:112:0: note: a field of same name but different type is defined in another translation unit
   SampleFormat sample_format_;
 ^
../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: type ï~@~XSampleFormatï~@~Y should match type ï~@~XAVSampleFormatï~@~Y
../../../dist/include/mp4_demuxer/audio_decoder_config.h:41:0: note: the incompatible type is defined here
 enum SampleFormat {

Which suggest that autdio_decoder_config.h is sometimes included with define turing SampleFormat to XSampleFormat
and sometimes to XAVSampleFormat. Not the most readable warning around, but probably can't do much better.

The carret diagnostics should not get confused by relative names - I was thinking about turing all locations into absolute names, but
that is probably not coolest thing either.
> 
> > @@ -445,31 +955,23 @@ add_type_duplicate (odr_type val, tree t
> >      {
> >        bool merge = true;
> >        bool base_mismatch = false;
> > -      bool warned = 0;
> >        unsigned int i,j;
> > +      bool warned = 0;
> 
> false?

Yep.

Thanks!
I will prepare updated patch with all the comments.
Honza
> 
> Trev


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]