[cfe-dev] Fwd: Review of transparent_union patch

[Daniel Dunbar]

Hi Anders,

I have a few comments...

--
+  if (S.getLangOptions().CPlusPlus) {
+    S.Diag(Attr.getLoc(),
+        diag::warn_transparent_union_attribute_not_c);
+    return;
+  }

I'm not sure we should emit this. This generally will appear in system
headers and be suppressed anyway. If the user tries to use something
with a transparent union, the type conversions should just fail.
--
     QualType FromType = From->getType();
     AssignConvertType ConvTy =
-      CheckSingleAssignmentConstraints(ToType, From);
+      CheckArgumentAssignmentConstraints(ToType, From);

What if instead of renaming this, there was just a separate method
CheckTransparentUnionAssignmentConstraints(..
and then this code becomes
--
     AssignConvertType ConvTy =
      CheckSingleAssignmentConstraints(ToType, From);
     if (ConvTy != Compatible)
       ConvTy = CheckTransparentUnionAssignmentConstraints(...)
--
this accomplishes both (a) keeping the code for this extension clear &
separate and (b) we only bother to do the extra semantic checks in the
rare case they are used instead of on every path.

--
+        if (FromType->isPointerType())
+          if (FromType->getAsPointerType()->getPointeeType()->isVoidType())
+            return Compatible;
+
+        if (rExpr->isNullPointerConstant(Context))
+          return Compatible;

There need to be implicit casts inserted of rExpr here?

+          ImpCastExprToType(rExpr, ArgType);
+          ImpCastExprToType(rExpr, UD->field_begin()->getType());

Can you add a comment for why we need both implicit casts:?

+DIAG(warn_transparent_union_attribute_not_difinition, WARNING,
+     "transparent_union attribute ignored, union type must be defined")

definition is misspelled.

Finally, could you add a Sema style test case which show cases all the
relevant cases & warnings on the attribute?

Thanks!

 - Daniel

On Tue, Jan 27, 2009 at 8:10 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jan 27, 2009, at 12:30 AM, Eli Friedman wrote:
>
>> On Mon, Jan 26, 2009 at 11:52 PM, Anders Johnsen <skabet at gmail.com>
>> wrote:
>>> On Tue, Jan 27, 2009 at 5:21 AM, Eli Friedman
>>> <eli.friedman at gmail.com> wrote:
>>>> On Mon, Jan 26, 2009 at 11:45 AM, Anders Johnsen
>>>> <skabet at gmail.com> wrote:
>>>>>> Something like the following:
>>>>>> struct x {int x; int* y;};
>>>>>> typedef struct x a __attribute((transparent_union));
>>>>>>
>>>>>> This isn't valid, but it looks like your patch accepts it.
>>>>>
>>>>> I just tested this, and it looks like it worked:
>>>>
>>>> Oh, oops, make that "union x".
>>>
>>> gcc does't complain. I'm not sure if i should do anything? Where have
>>> you read that it shouldn't allow such things?
>>
>> Hmm, I don't remember the details... if gcc accepts it, I suppose I
>> must be mistaken.
>
> GCC's attributes aren't handle consistently in the type system. Try
> this example:
>
>   union x {int x; int* y;};
>   typedef union x a __attribute((transparent_union));
>
>   void foo(union x);
>   void bar(a);
>
>   void test(int i) {
>     foo(i); // fails
>     bar(i); // okay
>   }
>
> So, a typedef of "union x" has different semantics from "union x",
> despite the fact that those types should be identical. The GCC guys
> know about these problems; see the discussion at http://gcc.gnu.org/ml/gcc/2006-10/msg00318.html
>
> I think that Clang should warn about the addition of "semantic"
> attributes (to steal Mark M.'s terminology) via typedefs, and say
> something about the typedef behaving differently from the original
> type due to the attribute. In C++ this is particularly important,
> since template instantiation needs to be based on the original type,
> even when one uses the typedef to name the instantiation (this is a
> particularly messy area in GCC).
>
> However, it seems to me that this patch is ready to go in. Any other
> issues we need to address first?
>
>        - Doug
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>