<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 14, 2014 at 7:40 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Nov 14, 2014 at 10:32 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 13, 2014 at 6:39 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 13, 2014 at 1:20 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+dblakie<div><br><div class="gmail_quote"><span>On Thu Nov 13 2014 at 1:53:39 AM Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Thank you for the analysis and the proposed solution!<div><br></div><div>I can reproduce the issue (with any q.cpp that is not clang-tidy clean):</div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div><div>$ clang-tidy q.cpp -- --serialize-diagnostics test.dia</div></div><div><div>*** Error in `clang_tidy': free(): invalid pointer: 0x00007fffa65bb4d8 ***</div></div><div><div>Aborted (core dumped)</div></div></blockquote><div><div class="gmail_extra"><br></div><div class="gmail_extra">The patch seems correct to me and the way to distinguish between owning and non-owning constructors seems also fine. I'll commit the patch tomorrow if nobody offers a better solution.</div></div></div></blockquote><div><br></div></span><div>I don't really see anything better under the current restrictions. Perhaps David has an idea, he has done a lot of the unique_ptr migrations in llvm.</div></div></div></blockquote><div><br></div></span><div>At a cursory glance, this is the "conditional ownership" issue that's come up in a few places (and currently we have solutions that both look like this one (T*+unique_ptr<T> where the latter may be null but otherwise they both point to the same object) or bool+T* where the bool indicates ownership)<br><br>There is a thread on llvm/cfe-dev about whether we should introduce a reusable "conditional ownership" pointer, but the response from several people (Manuel, Chandler, and, depending on the day of the week, myself, etc) is that this kind of ownership complication is a bug in the design which we should fix at the source.<br><br>I'm still not sure if that's the case (that all cases of conditional ownership like this are design bugs) - but I'm sort of curious to see how they would look if we really tried to apply that logic.<br><br>As a side note: this patch looks way too subtle/dangerous as-is, even given the necessary conditional ownership semantics. Two branches of the if, one calls func(takeX()) the other calls func(unique_ptr<T>(takeX()) - that's pretty subtle (even though the "ownsClient" condition demonstrates what's going on there).<br><br>I'm not sure how much it's worth making this a bit tidier/more reliable (maybe Diags::takeClient should return a unique_ptr and just return null whenever !Diags.ownsClient() - and have a separate "getClient" function that can be called to get a raw pointer, regardless of ownership (careful if we have an ordering issue there - if takeClient nulls out the Diags' client, then we'd need to call 'get' before 'take', if takeClient just sets "OwnsClient" to false, then we can call them in either order)) - or if it's just going to be a bit lame until we go the whole way and remove the conditional ownership of the DiagnosticConsumer all the way up (or add a first class maybe-owning pointer type).</div></div></div></div></blockquote><div><br></div></span><div>Yeah, there are too many possible options here and ideally, it would be nice to unify all cases of conditional ownership or eliminate them. I suppose, that the latter may be rather difficult to make as the scope of the change may be wide and affect many public interfaces. But in any case, we need a centralized decision on what we want to do with the conditional ownership.</div></div></div></div></blockquote><div><br></div></span><div>Yep :s</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>As for this patch, it should use the already existing getClient() function in the non-owning branch. I can commit a fix.<br></div></div></div></div></blockquote><div><br></div></span><div>Yeah, I think that'd help a lot.<br></div></div></div></div></blockquote><div><br></div><div>Committed the take -> get fix in r222130. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>(if you're feeling inclined - could you also make "takeClient" return by unique_ptr if that seems to fit (which I really hope it does)?)</div></div></div></div></blockquote><div><br></div><div>Sent you <a href="http://reviews.llvm.org/D6294">http://reviews.llvm.org/D6294</a>.</div></div>
</div></div>