[PATCH] Initial pass at API design for DebugInfo/PDB

[David Blaikie]

On Tue, Feb 3, 2015 at 3:54 PM, Zachary Turner <zturner at google.com> wrote:

> I suppose I could also finish off the rest of the concrete symbol types
> before going in.
>
> On Tue Feb 03 2015 at 3:52:13 PM Zachary Turner <zturner at google.com>
> wrote:
>
>> Fair enough, it's not a huge deal either way.  I've made this and some of
>> the other suggested changes locally.
>>
>> I'm a little bit more comfortable with this design I have now, especially
>> now that it works directly with llvm::dyn_cast<> and llvm::isa<>.  Anyone
>> have any major objections or concerns?
>>
>
Looks good to me.

If you like you could add some unit tests (a stub implementation would test
at least the trivial wrappers - oh, that makes me think, somewhere in this
code there should be a factory that, given a PDBRawSymbol, constructs the
appropriate PDBSymbol around it - that shouldn't be duplicated in all the
user code - then you could unit test that at least) - or compile-only
tests, in either case to demonstrate the API usage.

- David


> I will probably try to go in with this sometime tomorrow if not.
>>
>
>> On Tue Feb 03 2015 at 3:29:14 PM David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>> On Tue, Feb 3, 2015 at 3:25 PM, Zachary Turner <zturner at google.com>
>>> wrote:
>>>
>>>> I was thinking it might still be useful for people who knew what they
>>>> were doing and who wanted to write some implementation-specific code to be
>>>> able to get at the native native interface.  Like in this case a raw COM
>>>> pointer.  In theory this shouldn't ever be necessary, but it's hard to say
>>>> without knowing anything about implementation strategies other than DIA how
>>>> much compatibility someone would be able to achieve, and if there ever
>>>> might be things that are only possible to implement in terms of one API but
>>>> not the other.  So I didn't want to prematurely remove the possibility to
>>>> get at the native interface, for example by writing static_cast<DIASymbol*>(
>>>> PDBSymbol::getRawSymbol())->someDIAOnlyMethod().
>>>>
>>>
>>> I tend to err on the other side - we can always add more API if we find
>>> a use for it. No need to pay for things we're not using in the interim. But
>>> ultimately up to you.
>>>
>>>
>>> - David
>>>
>>>
>>>>
>>>> On Tue Feb 03 2015 at 3:19:11 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> On Tue, Feb 3, 2015 at 3:11 PM, Zachary Turner <zturner at google.com>
>>>>> wrote:
>>>>>
>>>>>> ================
>>>>>> Comment at: include/llvm/DebugInfo/PDB/IPDBRawSymbol.h:193
>>>>>> @@ +192,3 @@
>>>>>> +  virtual bool isVirtualInheritance() const = 0;
>>>>>> +  virtual bool isVolatileType() const = 0;
>>>>>> +};
>>>>>> ----------------
>>>>>> dblaikie wrote:
>>>>>> > How're these functions going to communicate failure?
>>>>>> Undefined in theory, probably false in practice.  You shouldn't call
>>>>>> methods on the raw interface unless you know it's a valid method.
>>>>>>
>>>>>> For the purposes of a dumper who wanted to detect unknown /
>>>>>> unexpected fields, that knowledge could all be encapsulated in the
>>>>>> implementation of the raw interface.  For example, you could have
>>>>>> PDBSymbol::dump() which calls RawSymbol->dump(), and that particular
>>>>>> implementation can go to the native API instead of calling the friendly
>>>>>> accessors.
>>>>>>
>>>>>
>>>>> Ah - that's a bit different from what I was thinking from our previous
>>>>> discussion.
>>>>>
>>>>> If it's undefined behavior, then it might be appropriate to hide
>>>>> PDBRawSymbol from users entirely - they might as well be casting down to
>>>>> the specific type and using those functions instead, perhaps? (I was
>>>>> thinking clients would be able to use PDBRawSymbol and just call all the
>>>>> functions and swallow the failures when they would call the wrong functions)
>>>>>
>>>>>
>>>>> - David
>>>>>
>>>>>
>>>>>>
>>>>>> http://reviews.llvm.org/D7356
>>>>>>
>>>>>> EMAIL PREFERENCES
>>>>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>>>>>
>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150203/f7fb3aca/attachment.html>