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

Duncan P. N. Exon Smith dexonsmith at apple.com
Fri Feb 6 16:47:41 PST 2015


> 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)) {
      // ...
    }

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



More information about the llvm-commits mailing list