On Fri, Nov 14, 2014 at 7:40 PM, David Blaikie <[email protected]> wrote:
> > > On Fri, Nov 14, 2014 at 10:32 AM, Alexander Kornienko <[email protected]> > wrote: > >> >> >> On Thu, Nov 13, 2014 at 6:39 PM, David Blaikie <[email protected]> >> wrote: >> >>> >>> >>> On Thu, Nov 13, 2014 at 1:20 AM, Manuel Klimek <[email protected]> >>> wrote: >>> >>>> +dblakie >>>> >>>> On Thu Nov 13 2014 at 1:53:39 AM Alexander Kornienko <[email protected]> >>>> wrote: >>>> >>>>> Thank you for the analysis and the proposed solution! >>>>> >>>>> I can reproduce the issue (with any q.cpp that is not clang-tidy >>>>> clean): >>>>> >>>>> $ clang-tidy q.cpp -- --serialize-diagnostics test.dia >>>>> *** Error in `clang_tidy': free(): invalid pointer: 0x00007fffa65bb4d8 >>>>> *** >>>>> Aborted (core dumped) >>>>> >>>>> >>>>> 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. >>>>> >>>> >>>> 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. >>>> >>> >>> 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) >>> >>> 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. >>> >>> 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. >>> >>> 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). >>> >>> 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). >>> >> >> 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. >> > > Yep :s > > >> As for this patch, it should use the already existing getClient() >> function in the non-owning branch. I can commit a fix. >> > > Yeah, I think that'd help a lot. > Committed the take -> get fix in r222130. > > (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)?) > Sent you http://reviews.llvm.org/D6294.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
