[PATCH] Support: Add dwarf::getVirtuality()

[Frédéric Riss]

> 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.

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150206/c3411cd7/attachment.html>