<div dir="ltr">Reviving this old thread and <a class="gmail_plusreply" id="plusReplyChip-0" href="mailto:jperaza@google.com" tabindex="-1">+Joshua Peraza</a>  who's also interested in this.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 13, 2017 at 4:17 PM Leonard Mosescu <<a href="mailto:mosescu@google.com">mosescu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Thanks Greg,<br><blockquote type="cite" style="color:rgb(80,0,80);font-size:12.8px"><div dir="ltr"><div>1. Expose the opaque ptr as an opaque handle() </div><div>     - this is an easy, quick and convenient solution for many SBxxx types but it may not work for all</div></div></blockquote></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px">That would be nice, but that won't always work with how LLDB is currently coded for SBFrame and possibly SBThread. These objects will be problems as they can come and go and the underlying object isn't always the same even through they lock onto the same logical object. SBThread and SBFrame have "lldb::ExecutionContextRefSP </span><span style="font-size:12.8px">m_opaque_sp" members. The execution context reference is a class that contains weak pointers to the lldb_private::Thread and lldb_private::StackFrame objects, but it also contains the thread ID and frame ID so it can reconstitute the value lldb_private::Thread and lldb_private::StackFrame even if the weak pointer isn't valid. So the opaque handle will work for many objects but not all.</span></blockquote><div><br></div><div>Indeed. One, relatively small but interesting benefit of the opaque handle type is that it opens the possibility of generic "handle maps" (I'll elaborate below)</div><div><blockquote type="cite" style="color:rgb(80,0,80);font-size:12.8px"><div dir="ltr"><div>2. Design and implement a consistent, first class identity/ordering/hashing for all the SBxxx types</div><div>     - perhaps the most elegant and flexible approach, but also the most work</div></div></blockquote></div><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">I would be fine with adding new members to classes we know we want to hash and order, like by adding:<br>uint32_t SB*::GetHash();<br>bool SB*::operator==(const SB*& ohs);<br>bool SB*::operator<(const SB*& ohs);<br>Would those be enough?</blockquote></div><div> </div><div>I think so. If we use the standard containers as reference, technically we only need operator< to satisfy the <a href="http://en.cppreference.com/w/cpp/concept/Compare" target="_blank">Compare</a> concept. (also, a small nit - size_t would be a better type for the hash value). Also, both the hashing and the compare can be implemented as non-member functions (or even specializing std::hash, std::less for SBxxx types). A few minor concerns:</div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div>a. if we keep things like SBModule::operator==() unchanged, it's not going to be the same as the equiv(a, b) for the case where a and b have null opaque pointers (not sure if this breaks anything, but I wouldn't want to be the first to debug a case where this matter)</div><div>b. defining just the minimum set of operations may be technically enough but it may look a bit weird to have a type define < but none of the other relational operators.</div><div>c. if some of the hash/compare implementation end up going through multiple layers (the execution context with thread, frame IDs example) the performance characteristics can be unpredictable, right? </div></blockquote><div><br></div><div>For context, the use case that brought this to my attention is managing a set of data structures that contain custom data associated with modules, frames, etc. It's easy to create, let's say a MyModule from a SBModule, but if later on I get the module for a particular frame, SBFrame::GetModule() will return a SBModule, which I would like to map to the corresponding MyModule instance. Logically this would require a SBModule -> MyModule map. The standard associative containers (map or unordered_map) would make this trivial if SBxxx types satisfy the key requirements. </div><div><br></div><div>Another option for maintaining such a mapping, suggested by Mark Mentovai, is to use provision for an "user data" tag associated with every SBxxx object (this tag can simply be a void*, maybe wrapped with type safe accessors). This would be extremely convenient for the API users (since they don't have to worry about maintaining any maps themselves) but implementing it would hit the same complications around the synthesized instances (like SBFrame) and it may carry a small price - one pointer per SBxxx instance even if this facility is not used. I personally like this approach and in this particular case it has the additional benefit of being additive (we can graft it on with minimal risk of breaking existing stuff), although it still seems nice to have consistent identity semantics for the SBxxx types. </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 13, 2017 at 12:40 PM, Greg Clayton <span dir="ltr"><<a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><br><div><span><blockquote type="cite"><div>On Dec 13, 2017, at 11:44 AM, Leonard Mosescu via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:</div><br class="gmail-m_5038734868273431791m_4182136635983653466Apple-interchange-newline"><div><div dir="ltr">LLDB's C++ API deals with SBxxx objects, most of which are PIMPL-style wrappers around an opaque pointer to the internal implementation. These SBxxx objects act as handles and are passed/returned by value, which is generally convenient, except for the situations where one would need to keep track of object identities, ex. using them as keys in associative containers.<div><br></div><div>As far as I can tell, there's a bit of inconsistency in the current state:</div><div><br></div><div>1. Some types, ex. SBThread, SBTarget, SBSection, ... offer ==, != that map directly to the corresponding operator on the opaque pointer (good!), but:</div><div>    .. there are no ordering operators, nor obvious ways to hash the objects</div><div>2. SBModule offer the == , != operators, but:</div><div>    ... the implementations for == and != are not exactly forwarded to the corresponding operator on the opaque pointer (1)<br></div><div>3. Things like SBFrame offer IsEqual() in addition to ==, !=, creating a bit of confusion</div><div>4. Other types (ex. SBProcess, SBSymbol, SBBlock) don't offer any kind of comparison operations.</div><div><br></div><div>IMO it would be nice to have a consistent "handle type" semantics regarding identity, ordering and hashing. I can see the following options:</div><div><br></div><div>1. Expose the opaque ptr as an opaque handle() </div><div>     - this is an easy, quick and convenient solution for many SBxxx types but it may not work for all</div></div></div></blockquote><div><br></div></span>That would be nice, but that won't always work with how LLDB is currently coded for SBFrame and possibly SBThread. These objects will be problems as they can come and go and the underlying object isn't always the same even through they lock onto the same logical object. SBThread and SBFrame have "lldb::ExecutionContextRefSP m_opaque_sp" members. The execution context reference is a class that contains weak pointers to the lldb_private::Thread and lldb_private::StackFrame objects, but it also contains the thread ID and frame ID so it can reconstitute the value lldb_private::Thread and lldb_private::StackFrame even if the weak pointer isn't valid. So the opaque handle will work for many objects but not all.<span><br><blockquote type="cite"><div dir="ltr"></div></blockquote><br><blockquote type="cite"><div><div dir="ltr"><div>2. Design and implement a consistent, first class identity/ordering/hashing for all the SBxxx types</div><div>     - perhaps the most elegant and flexible approach, but also the most work</div></div></div></blockquote><div><br></div></span>I would be fine with adding new members to classes we know we want to hash and order, like by adding:</div><div><br></div><div>uint32_t SB*::GetHash();</div><div>bool SB*::operator==(const SB*& ohs);</div><div><div>bool SB*::operator<(const SB*& ohs);</div><div><br></div><div>Would those be enough?</div><div><br></div><div><blockquote type="cite"><div dir="ltr"></div></blockquote></div></div><span><div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Any thoughts on this? Did I miss anything fundamental here?</div></div></div></blockquote><div><br></div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Thanks,</div><div>Lemo.</div><div><br></div><div>(1) example of operator== from SBModule:</div><div><br></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div><div style="font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback";line-height:18px;white-space:pre-wrap"><div><font size="1"><span style="color:rgb(0,0,255)">bool</span> SBModule::<span style="color:rgb(175,0,219)">operator</span>==(<span style="color:rgb(0,0,255)">const</span> SBModule &rhs) <span style="color:rgb(0,0,255)">const</span> {</font></div></div></div><div><div style="font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback";line-height:18px;white-space:pre-wrap"><div><font size="1">  <span style="color:rgb(175,0,219)">if</span> (m_opaque_sp)</font></div></div></div><div><div style="font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback";line-height:18px;white-space:pre-wrap"><div><font size="1">    <span style="color:rgb(175,0,219)">return</span> m_opaque_sp.<span style="color:rgb(121,94,38)">get</span>() == rhs.<span style="color:rgb(0,16,128)">m_opaque_sp</span>.<span style="color:rgb(121,94,38)">get</span>();</font></div></div></div><div><div style="font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback";line-height:18px;white-space:pre-wrap"><div><font size="1">  <span style="color:rgb(175,0,219)">return</span> <span style="color:rgb(0,0,255)">false</span>;</font></div></div></div><div><div style="font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback";line-height:18px;white-space:pre-wrap"><div><font size="1">}</font></div></div></div></blockquote></div></div></blockquote><br></div></span><div>So I would leave this up to the SB classes so that we can change the implementation if needed so I would prefer to add methods to each SB object for hashing and comparison.</div><span class="gmail-m_5038734868273431791HOEnZb"><font color="#888888"><div><br></div><div>Greg Clayton</div><div><br></div><br></font></span></div></blockquote></div><br></div>
</blockquote></div>