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


+@opindex Wodr
+@opindex Wno-odr
+@opindex Wodr
+Warn about One Definition Rule violations during link time optimization.
+Require @option{-flto-odr-type-merging} to be enabled. Enabled by default

Duplicated @opindex Wodr. (@item is missing)
"Requires". Period after "default"

But according to current practice (which I dislike), this should be

@item -Wno-odr
@opindex Wodr
@opindex Wno-odr
Disable warnings about One Definition Rule violations during link time
optimization.
These warnings are enabled by default and require
@option{-flto-odr-type-merging} to be enabled.

See the example for -Woverflow just above where you added Wodr.


+      if (!warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (t1)), OPT_Wodr,
+       "type %qT violates one definition rule  ",
+       t1))
+        return;

Why the trailing whitespace within the warning message?

+      if (!warning_at (DECL_SOURCE_LOCATION (st1), OPT_Wodr,
+       "field %qD (of type %qT) violates one definition rule  ",
+       st1, t1))

I agree with others that say that "(of type %qT)" is understood as %qD
being of type %qT. I would also suggest to drop the parentheses in the
new message.

+    inform (UNKNOWN_LOCATION, "Conflicting compilation units: %s and %s",
+    IDENTIFIER_POINTER (name),
+    IDENTIFIER_POINTER (name1));

GNU-standard diagnostics do not start with uppercase. What do you
think about using %qs here?

+        G_("a type with attributes "
+   "is defined in another translation unit"

Is this a "type with different attributes"?

+      bool warned = 0;

We should really use true/false for booleans.


+ FIXME: disable for now; because ODR types are now build during
+ streaming in, the variants do not need to be linked to the type,
+ yet.  We need to do the merging in cleanup pass to be implemented
+ soon.  */
       if (!flag_ltrans && merge
+  && 0
   && TREE_CODE (val->type) == RECORD_TYPE
   && TREE_CODE (type) == RECORD_TYPE
   && TYPE_BINFO (val->type) && TYPE_BINFO (type)
@@ -569,7 +1076,6 @@ add_type_duplicate (odr_type val, tree t
       == master_binfo)
     set_type_binfo ((*val->types)[i], TYPE_BINFO (type));
  }
-      BINFO_TYPE (TYPE_BINFO (type)) = val->type;
     }
   else

Why commit this part if it is disabled? There seems to be other parts
of the code that are known to not work but will be committed. Perhaps
I am misunderstanding and this is going to a development branch and
not trunk.

And tescases? Shouldn't there be one testcase for each possible
diagnostic? Otherwise we get diagnostics that are never tested and
they stop to work (or they even didn't work in the first place) even
though there are huge chunks of code devoted to them.

Cheers,

Manuel.


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