r217474 - Unique_ptrify PPCallbacks ownership.

[David Blaikie]

On Sep 10, 2014 10:27 PM, "Craig Topper" <craig.topper at gmail.com> wrote:
>
> You'll notice half the places in clang that called addPPCallbacks ended
up with a cast to std::unique_ptr in the call to addPPCallbacks for the
same reason.

If that api takes ownership, I'd still vote for it to take by unique_ptr
even if many callers will still want to refer to the object later.
Otherwise its hard to tell who's responsible for cleanup.

> But given that I find that kind of ugly I think I'll revert the interface
part of the change.
>
>
> On Wed, Sep 10, 2014 at 12:13 PM, Kim Gräsman <kim.grasman at gmail.com>
wrote:
>>
>> Hi Craig,
>>
>> On Wed, Sep 10, 2014 at 6:53 AM, Craig Topper <craig.topper at gmail.com>
wrote:
>> > Author: ctopper
>> > Date: Tue Sep  9 23:53:53 2014
>> > New Revision: 217474
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=217474&view=rev
>> > Log:
>> > Unique_ptrify PPCallbacks ownership.
>> >
>> > Unique_ptr creation stil needs to be moved earlier at some of the call
sites.
>>
>> I'm not happy with this change at all. It precludes a PPCallbacks also
>> serving other duties. E.g. in IWYU we have:
>>
>>     IwyuPreprocessorInfo* const preprocessor_consumer
>>         = new IwyuPreprocessorInfo();
>>     compiler.getPreprocessor().addPPCallbacks(preprocessor_consumer);
// here
>>     compiler.getPreprocessor().addCommentHandler(preprocessor_consumer);
>>  // here
>>
>>     VisitorState* const visitor_state
>>         = new VisitorState(&compiler, *preprocessor_consumer);  // and
here
>>     return std::unique_ptr<IwyuAstConsumer>(new
IwyuAstConsumer(visitor_state));
>>
>> Our PPCallbacks impl also happens to be a comment handler, plus we
>> want to share the data it's collected with our AST consumer.
>>
>> The unique_ptr completely kills this scenario, and alternative
>> designed seem much more complex.
>>
>> Would you consider reverting?
>>
>> Thanks,
>> - Kim
>
>
>
>
> --
> ~Craig
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140911/c2f60daa/attachment.html>