<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 3, 2015 at 3:54 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I suppose I could also finish off the rest of the concrete symbol types before going in.<br></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote">On Tue Feb 03 2015 at 3:52:13 PM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Fair enough, it's not a huge deal either way. I've made this and some of the other suggested changes locally.<br><br><div>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? </div></div></blockquote></div></div></div></blockquote><div><br>Looks good to me.<br><br>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.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div> I will probably try to go in with this sometime tomorrow if not.<span style="color:rgb(34,34,34)"> </span></div></div></blockquote></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div class="gmail_quote">On Tue Feb 03 2015 at 3:29:14 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 3, 2015 at 3:25 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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*>(<u></u>PDBSymbol::getRawSymbol())-><u></u>someDIAOnlyMethod().<br></div></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>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.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br><br>- David<br> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"></div><div><div><br><div class="gmail_quote">On Tue Feb 03 2015 at 3:19:11 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 3, 2015 at 3:11 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">================<br>
Comment at: include/llvm/DebugInfo/PDB/<u></u>IPDBRawSymbol.h:193<br>
@@ +192,3 @@<br>
+ virtual bool isVirtualInheritance() const = 0;<br>
+ virtual bool isVolatileType() const = 0;<br>
+};<br>
----------------<br>
dblaikie wrote:<br>
> How're these functions going to communicate failure?<br>
Undefined in theory, probably false in practice. You shouldn't call methods on the raw interface unless you know it's a valid method.<br>
<br>
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.<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>Ah - that's a bit different from what I was thinking from our previous discussion.<br><br>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)</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br><br>- David<br> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
<a href="http://reviews.llvm.org/D7356" target="_blank">http://reviews.llvm.org/D7356</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/<u></u>settings/panel/<u></u>emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div></div></div></blockquote></div>
</div></div></blockquote></div></div></div></blockquote></div></blockquote></div>
</div></div></blockquote></div><br></div></div>