[PATCH] Remove DW_TAG_invalid

[Duncan P. N. Exon Smith]

> On 2015-Feb-06, at 17:06, Frédéric Riss <friss at apple.com> wrote:
> 
>> 
>> On Feb 6, 2015, at 4:47 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>>> 
>>> On 2015-Feb-06, at 16:28, Frédéric Riss <friss at apple.com> wrote:
>>> 
>>>> 
>>>> On Feb 6, 2015, at 4:00 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>> DW_VIRTUALITY_* (the last DW_* thing that my upcoming assembly patches
>>>> use) is a bit different from DW_TAG, DW_LANG and DW_ATE.
>>>> 
>>>> There are only three valid values:
>>>> 
>>>> - 0x0 (none),
>>>> - 0x1 (virtual), and
>>>> - 0x2 (pure_virtual),
>>>> 
>>>> and there isn't a `DW_VIRTUALITY_lo_user` or `DW_VIRTUALITY_hi_user`.
>>>> 
>>>> The assembly code needs to know the valid numeric range, so I've added
>>>> a `DW_VIRTUALITY_max` that also points at `0x2`.  I've also added a
>>>> `DW_VIRTUALITY_invalid` to `LLVMConstants` to return as an error code
>>>> from `getVirtuality()`.
>>>> 
>>>> I was about to commit without asking, but since it's a little different
>>>> I thought I'd check.  Does this make sense to others?
>>> 
>>> This makes sense, it LGTM.
>> 
>> r228473 and r228474.
>> 
>>> This change made me wonder if you shouldn’t define _invalid constants for the the ones where you used a simple 0 as an error return. It would make the parser code more homogenous, wouldn’t it?
>> 
>> I prefer `0`s because of this pattern:
>> 
>>    if (unsigned Lang = dwarf::getLanguage(LangString)) {
>>      // ...
>>    }
> 
> Yeah, it’s nice to be able to use it like that. It’d actually be nicer to use it like that for every enum...
> 
>> I was even thinking of proposing that `dwarf::getTag()` return 0.
>> ("Deep inspection" via `git grep` tells me that my introduced uses
>> are the *only* ones; previously this was dead code!  I should have
>> noticed that at the time.)  
> 
> I actually thought we were already doing that. And there are some places like (DIELoc and DIEBlock constructors come to mind) where we use a 0 tag as some kind of invalid value marker.
> 

Well, this patch removes DW_TAG_invalid.  I'll wait for someone
else to chime in though (maybe there's a reason we weren't using 0?).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-DW_TAG_invalid.patch
Type: application/octet-stream
Size: 3716 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150206/e568998f/attachment.obj>
-------------- next part --------------


>> In that case `DW_VIRTUALITY` would be
>> the only one with a special "invalid" tag.
>> 
>> However, I suppose consistency is important too.  I'm happy to
>> change the others to `DW_*_invalid` if that's better.  I'll let the
>> rest of you folks decide.
> 
> I don’t  think it’s worth, your style argument above makes sense. Was just a thought anyway.
> 
> Fred