On 06/30/2011 03:56 AM, Richard Guenther wrote:
>> > It is only used by darwin at present - so nothing is split out for any 
>> > other
>> > target (the default hook is simply 'false').
> Yes, I've seen that.  Still a hook like this should be generally useful,
> and you still process through some pieces of assemble_variable
> while you choose to skip some other piece - it should probably be
> documented which part of assemble_variable is supposed to be handled
> by the hook.
> 
> I'll defer to Richard for this (and the approval).
> 

I definitely want to see better documentation.  I don't know how to
review the patch at the moment.  Some background for us non-darwin
programmers would help as well; I don't know what's legal and what
isn't when it comes to these sorts of odd non-definitions.

You're changing the set of variables emitted for non-darwin here
too, as far as I can tell.  Is that actually desired?  I can't
imagine what these variables are intended to achieve, given that
they're byte sized and DECL_INITIAL = error_mark_node.

I can say that you need to watch the formatting.  There are missing
spaces after commas, and incorrect indentation.

> +  if (DECL_ATTRIBUTES (decl)
> +      && (meta = lookup_attribute ("OBJC1META", DECL_ATTRIBUTES (decl)))) 

Do not assign to variables inside if conditions.  No need to check
for DECL_ATTRIBUTES non-null before calling lookup_attribute.

Reply via email to