[cfe-dev] Adding type source info to ImplicitValueInitExpr.

[John McCall]

On Feb 14, 2010, at 1:57 AM, Abramo Bagnara wrote:
> Il 13/02/2010 23:53, John McCall ha scritto:
>> 
>> On Feb 13, 2010, at 5:18 AM, Enea Zaffanella wrote:
>> 
>>> John McCall wrote:
>>>> On Jan 18, 2010, at 5:24 PM, Abramo Bagnara wrote:
>>>>> Il 18/01/2010 22:33, John McCall ha scritto:
>>>>>> On Jan 18, 2010, at 3:21 AM, Enea Zaffanella wrote:
>>>>>>> The attached patch adds (optional) type source info to
>>>>>>> class ImplicitValueInitExpr: the type source info will have
>>>>>>> a valid value if the ImplicitValueInitExpr results from
>>>>>>> __builtin_offsetof constructs.
>>>>>> I think we'd rather not take this.  The primary use of 
>>>>>> ImplicitValueInitExpr does not involve a type written in the
>>>>>> source; in fact, it involves no source code at all. This
>>>>>> patch is useful solely because of the current implementation
>>>>>> of __builtin_offsetof. That implementation is terrible, and
>>>>>> it's long past time for __builtin_offsetof to get its own AST
>>>>>> class. When that happens, this patch will be unnecessary.
>>> 
>>> 
>>> Hello.
>>> 
>>> A student of mine would like to have a try fixing the thing above 
>>> (a.k.a., PR 5390), unless there already is someone working on it.
>>> 
>>> The approach we would follows is as follows:
>>> 
>>> 1) Take out OffsetOf from UnaryOperator::OpCode and have instead a
>>> dedicated class inheriting from Expr (OffsetOfExpr).
>>> 
>>> 2) Create a new class OffsetOfBaseExpr that will replace current
>>> (ab-)uses of ImplicitValueInitExpr in the context of the new class
>>> above: this new class will embed a TypeSourceInfo object, so as to
>>> be able to provide suitable source location info.
>> 
>> I don't understand why you need this second expression class;  the
>> base of an offsetof is a type, not an expression.  You should be able
>> to get by with just having a TypeSourceInfo as a field in the
>> OffsetOfExpr.
> 
> The problem don't come from offsetof base, but from offsetof member:
> once taken for granted that the more suitable way to express that is an
> Expr we'd need another Expr node to represent the typed base of such
> member expression (exactly as it works currently, only with more
> appropriate Expr nodes).
> 
> We are missing something? There is a smarter way you see?


Forgive me;  I missed an important point of your earlier message.  I am recommending that you radically restructure the representation of offsetof so that it no longer has any sub-expressions at all and simply carries all the necessary information directly on a single expression node.  This node would have an array of "chunks", much like the input Sema gets from the parser.  When the type is non-dependent, the actual offset should also be stored directly on the OffsetOfExpr node for the sake of simplicity;  it is straightforward to calculate this during validation of the offsetof, and it removes what would otherwise be a great deal of unnecessary complexity from the constant evaluator.

John.