[cfe-dev] Problem with a change on newFrontendActionFactory

[David Blaikie]

On Thu, Jun 26, 2014 at 12:52 AM, Manuel Klimek <klimek at google.com> wrote:
> On Thu, Jun 26, 2014 at 1:25 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Tue, Jun 3, 2014 at 12:44 AM, Manuel Klimek <klimek at google.com> wrote:
>> > On Tue, Jun 3, 2014 at 12:16 AM, David Blaikie <dblaikie at gmail.com>
>> > wrote:
>> >>
>> >> On Mon, Jun 2, 2014 at 1:34 AM, Manuel Klimek <klimek at google.com>
>> >> wrote:
>> >> > Sorry for the late answer. I had it unread in my inbox and was
>> >> > mulling
>> >> > over
>> >> > various aspects to it.
>> >> > My general problem with the whole thing is that even if your solution
>> >> > is
>> >> > pareto-better (which I personally don't think, but let's assume), I
>> >> > don't
>> >> > think it matters enough to be worth the effort of change.
>> >> > So, I'm not opposed to interface changes, I'm opposed to interface
>> >> > changes
>> >> > that don't provide significant enough value to pay for the cost. I am
>> >> > also
>> >> > not sure which changes exactly we're now talking about - I think it
>> >> > would
>> >> > help if you could split this CL in to the parts that (I hope) are
>> >> > non-controversial, so we have solved the actual problem, and the
>> >> > parts
>> >> > that
>> >> > are controversial.
>> >> >
>> >> > On Wed, May 7, 2014 at 6:07 PM, David Blaikie <dblaikie at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> On Wed, May 7, 2014 at 4:46 AM, Manuel Klimek <klimek at google.com>
>> >> >> wrote:
>> >> >> > On Tue, May 6, 2014 at 6:38 PM, David Blaikie <dblaikie at gmail.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Tue, May 6, 2014 at 12:42 AM, Manuel Klimek
>> >> >> >> <klimek at google.com>
>> >> >> >> wrote:
>> >> >> >> > On Mon, May 5, 2014 at 10:54 PM, David Blaikie
>> >> >> >> > <dblaikie at gmail.com>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> On Mon, May 5, 2014 at 8:42 AM, Manuel Klimek
>> >> >> >> >> <klimek at google.com>
>> >> >> >> >> wrote:
>> >> >> >> >> > On Mon, May 5, 2014 at 5:14 PM, David Blaikie
>> >> >> >> >> > <dblaikie at gmail.com>
>> >> >> >> >> > wrote:
>> >> >> >> >> >>
>> >> >> >> >> >> On Mon, May 5, 2014 at 3:10 AM, Manuel Klimek
>> >> >> >> >> >> <klimek at google.com>
>> >> >> >> >> >> wrote:
>> >> >> >> >> >> > On Thu, May 1, 2014 at 10:21 PM, Richard Smith
>> >> >> >> >> >> > <richard at metafoo.co.uk>
>> >> >> >> >> >> > wrote:
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> On Thu, May 1, 2014 at 1:19 PM, Nico Weber
>> >> >> >> >> >> >> <thakis at chromium.org>
>> >> >> >> >> >> >> wrote:
>> >> >> >> >> >> >>>
>> >> >> >> >> >> >>> On Thu, May 1, 2014 at 1:17 PM, David Blaikie
>> >> >> >> >> >> >>> <dblaikie at gmail.com>
>> >> >> >> >> >> >>> wrote:
>> >> >> >> >> >> >>> > On Thu, May 1, 2014 at 1:12 PM, Richard Smith
>> >> >> >> >> >> >>> > <richard at metafoo.co.uk>
>> >> >> >> >> >> >>> > wrote:
>> >> >> >> >> >> >>> >> On Thu, May 1, 2014 at 12:55 PM, Etienne Ollivier
>> >> >> >> >> >> >>> >> <eollivier at bsu.edu>
>> >> >> >> >> >> >>> >> wrote:
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> Hello,
>> >> >> >> >> >> >>> >>> I updated my clang repository recently and I an
>> >> >> >> >> >> >>> >>> error
>> >> >> >> >> >> >>> >>> appeared
>> >> >> >> >> >> >>> >>> that
>> >> >> >> >> >> >>> >>> was
>> >> >> >> >> >> >>> >>> not
>> >> >> >> >> >> >>> >>> there before:
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> error: no viable conversion from
>> >> >> >> >> >> >>> >>> 'std::unique_ptr<FrontendActionFactory>'
>> >> >> >> >> >> >>> >>> to
>> >> >> >> >> >> >>> >>>       'clang::tooling::ToolAction *'
>> >> >> >> >> >> >>> >>>         return
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> Tool.run(newFrontendActionFactory<MyPluginASTAction>());
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> It is because newFrontendActionFactory has been
>> >> >> >> >> >> >>> >>> changed
>> >> >> >> >> >> >>> >>> to
>> >> >> >> >> >> >>> >>> work
>> >> >> >> >> >> >>> >>> with
>> >> >> >> >> >> >>> >>> std::unique_ptr. So if I change my code to
>> >> >> >> >> >> >>> >>>    return
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> Tool.run(&(*newFrontendActionFactory<MyPluginASTAction>()));
>> >> >> >> >> >> >>> >>
>> >> >> >> >> >> >>> >>
>> >> >> >> >> >> >>> >> You can use .get() rather than the slightly
>> >> >> >> >> >> >>> >> non-obvious
>> >> >> >> >> >> >>> >> &*.
>> >> >> >> >> >> >>> >>
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> it works. The only little problem is that it can be
>> >> >> >> >> >> >>> >>> confusing
>> >> >> >> >> >> >>> >>> for
>> >> >> >> >> >> >>> >>> users
>> >> >> >> >> >> >>> >>> since is not the way it is written in the
>> >> >> >> >> >> >>> >>> documentation,
>> >> >> >> >> >> >>> >>> like
>> >> >> >> >> >> >>> >>> on
>> >> >> >> >> >> >>> >>> this
>> >> >> >> >> >> >>> >>> pages:
>> >> >> >> >> >> >>> >>> http://clang.llvm.org/docs/LibTooling.html
>> >> >> >> >> >> >>> >>>
>> >> >> >> >> >> >>> >>> http://clang.llvm.org/docs/LibASTMatchersTutorial.html
>> >> >> >> >> >> >>> >>
>> >> >> >> >> >> >>> >>
>> >> >> >> >> >> >>> >> Thanks, I've updated the documentation.
>> >> >> >> >> >> >>> >
>> >> >> >> >> >> >>> > I'm trying to understand how the ownership used to
>> >> >> >> >> >> >>> > work/is
>> >> >> >> >> >> >>> > meant
>> >> >> >> >> >> >>> > to
>> >> >> >> >> >> >>> > work now...
>> >> >> >> >> >> >>>
>> >> >> >> >> >> >>> The result of newFrontendActionFactory() used to be
>> >> >> >> >> >> >>> leaked.
>> >> >> >> >> >> >>> Now
>> >> >> >> >> >> >>> it's
>> >> >> >> >> >> >>> freed at the end-of-statement cleanup of the returned
>> >> >> >> >> >> >>> (invisible)
>> >> >> >> >> >> >>> unique_ptr temporary.
>> >> >> >> >> >> >>
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> Why do we need to heap-allocate the
>> >> >> >> >> >> >> FrontendActionFactory
>> >> >> >> >> >> >> at
>> >> >> >> >> >> >> all?
>> >> >> >> >> >> >
>> >> >> >> >> >> >
>> >> >> >> >> >> > Technically we don't. There's just some ways to create
>> >> >> >> >> >> > the
>> >> >> >> >> >> > FrontendActionFactory via templated factory functions
>> >> >> >> >> >>
>> >> >> >> >> >> The current factories don't seem to make dynamic choices
>> >> >> >> >> >> (or
>> >> >> >> >> >> even
>> >> >> >> >> >> templated choices) about which type to return (I may've
>> >> >> >> >> >> missed
>> >> >> >> >> >> something, though) - and the internal templating could
>> >> >> >> >> >> still
>> >> >> >> >> >> be
>> >> >> >> >> >> implemented via a ctor template instead, I think.
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > How would it store the pointer to the FactoryT*
>> >> >> >> >> > ConsumerFactory?
>> >> >> >> >>
>> >> >> >> >> I'm not sure I understand - it just takes it as a ctor
>> >> >> >> >> parameter
>> >> >> >> >> and
>> >> >> >> >> stores it, doesn't it? Same as when the factory function is
>> >> >> >> >> used.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Sure, but then we a templated constructor wouldn't be enough,
>> >> >> >> > we'd
>> >> >> >> > need
>> >> >> >> > a
>> >> >> >> > templated class. If that's what you meant from the start, I
>> >> >> >> > misunderstood.
>> >> >> >>
>> >> >> >> Sorry, yes, I'm not sure what I was trying to say above (I hadn't
>> >> >> >> looked at the code in detail at that point - so I was probably
>> >> >> >> being
>> >> >> >> a
>> >> >> >> bit vague/wrong). But, yes - as you can see in the patches, the
>> >> >> >> types
>> >> >> >> can just be used directly.
>> >> >> >>
>> >> >> >> >> You talk about needing these things to not move around - so
>> >> >> >> >> you
>> >> >> >> >> can
>> >> >> >> >> point to them - but even that doesn't seem relevant to the
>> >> >> >> >> construction phase. If these factories returned by value and
>> >> >> >> >> then
>> >> >> >> >> that
>> >> >> >> >> value was passed by const-ref to Tool.run, what would break?
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Well, we can't return a class with virtual methods by value,
>> >> >> >> > can
>> >> >> >> > we?
>> >> >> >> > We'd
>> >> >> >> > need to get rid of the factories if we want to not use pointers
>> >> >> >> > (and
>> >> >> >> > you're
>> >> >> >> > doing that in both patches).
>> >> >> >>
>> >> >> >> Not necessarily. The factories could return by value:
>> >> >> >>
>> >> >> >> struct base { ... };
>> >> >> >> struct d1 : base { ... };
>> >> >> >> struct d2 : base { ... };
>> >> >> >>
>> >> >> >> d1 factory() { return d1(); }
>> >> >> >> d2 factory(int) { return d2(); }
>> >> >> >>
>> >> >> >> and usage could either be:
>> >> >> >>
>> >> >> >> d1 x = factory();
>> >> >> >> d2 y = factory(3);
>> >> >> >>
>> >> >> >> or:
>> >> >> >>
>> >> >> >> base &x = factory();
>> >> >> >> base &y = factory(3);
>> >> >> >
>> >> >> >
>> >> >> > Having the factory "leak" what concrete object it passes back
>> >> >> > defeats
>> >> >> > the
>> >> >> > purpose of the factory.
>> >> >>
>> >> >> Not necessarily - you said one of the benefits was template argument
>> >> >> deduction and I'm just demonstrating that that can be achieved
>> >> >> without
>> >> >> dynamic allocation.
>> >> >
>> >> >
>> >> > I don't see how that argument gets us any further. My arguments are
>> >> > always
>> >> > based on the full context of a specific implementation, not specific
>> >> > theoretical issues.
>> >> >
>> >> >>
>> >> >> >> But all except 1 caller I touched are just passing the result
>> >> >> >> straight
>> >> >> >> to Tool.run, so this change doesn't affect them. The one caller
>> >> >> >> that
>> >> >> >> did:
>> >> >> >
>> >> >> >
>> >> >> > The problem with an interface intended for use outside of Clang's
>> >> >> > tree
>> >> >> > is
>> >> >> > that I don't think arguing based on what we find in Clang's tree
>> >> >> > (which
>> >> >> > are
>> >> >> > mostly tests) is good enough.
>> >> >>
>> >> >> We have 4-5 tools, that seemed like a reasonable bunch of use-cases.
>> >> >>
>> >> >> I'm happy to go and look inside Google (where I assume the vast
>> >> >> majority of tools are so far) and see if we see anything else.
>> >> >>
>> >> >> Are there other things you think would be appropriate to use to
>> >> >> evaluate
>> >> >> this?
>> >> >
>> >> >
>> >> > Patch it into the internal code-base, make sure all reverse
>> >> > dependency's
>> >> > tests pass.
>> >> >
>> >> >>
>> >> >>
>> >> >> >> unique_ptr<factory> p;
>> >> >> >> if (x)
>> >> >> >>   p = newFactory();
>> >> >> >> else if (y)
>> >> >> >>   p = newFactory(a);
>> >> >> >> else
>> >> >> >>   p = newFactory(b);
>> >> >> >> Tool.run(p);
>> >> >> >>
>> >> >> >> and the change was to roll the "Tool.run" call up into the call
>> >> >> >> site,
>> >> >> >> which wasn't overly burdensome.
>> >> >> >>
>> >> >> >> >> I don't
>> >> >> >> >> think anything would - the constructor of the
>> >> >> >> >> FrontendActiorFactories
>> >> >> >> >> don't appear to leak pointers to themselves. So if you prefer
>> >> >> >> >> the
>> >> >> >> >> factory function syntax, you can keep that. The prototype
>> >> >> >> >> patches
>> >> >> >> >> attached do not, though (in case you're right about
>> >> >> >> >> immovability).
>> >> >> >> >>
>> >> >> >> >> > Sure, a
>> >> >> >> >> > templated class would work (basically just instantiate the
>> >> >> >> >> > FrontendActionFactoryAdapter), but the problem is that then
>> >> >> >> >> > you'd
>> >> >> >> >> > always
>> >> >> >> >> > need to specify the template argument.
>> >> >> >> >>
>> >> >> >> >> Having to specify the parameter doesn't seem terribly
>> >> >> >> >> burdensome.
>> >> >> >> >
>> >> >> >> > I find having a unique_ptr return not a terrible problem, so
>> >> >> >> > I'd
>> >> >> >> > argue
>> >> >> >> > it's
>> >> >> >> > trade-offs.
>> >> >> >>
>> >> >> >> Sure (though as shown above, this particular issue doesn't have
>> >> >> >> to
>> >> >> >> be
>> >> >> >> traded off - we can still use function templates as builders and
>> >> >> >> get
>> >> >> >> the convenience of template argument deduction if it's important)
>> >> >> >>
>> >> >> >> >> > Also, there is an overload
>> >> >> >> >> > newFrontendActionFactory<FrontendActionType>(),
>> >> >> >> >> > and I think it has value that those form similar patterns.
>> >> >> >> >>
>> >> >> >> >> Which would be similarly owned directly in the caller and
>> >> >> >> >> passed
>> >> >> >> >> by
>> >> >> >> >> reference.
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> Tool.run(SimpleFrontendActionFactory<clang::ento::AnalysisAction>());
>> >> >> >> >>
>> >> >> >> >> Various cleanup/modification was required, including making
>> >> >> >> >> FrontendActionFactory's functions const (so the temporary
>> >> >> >> >> could
>> >> >> >> >> be
>> >> >> >> >> passed by const ref) - all existing implementations except the
>> >> >> >> >> weird
>> >> >> >> >> SingleFrontendActionFactory, required no changes. (well,
>> >> >> >> >> technically
>> >> >> >> >> SingleFrontendActionFactory would've required no changes if I
>> >> >> >> >> hadn't
>> >> >> >> >> fixed its raw pointer ownership to be a unique_ptr ownership -
>> >> >> >> >> then
>> >> >> >> >> the unique_ptr had to be mutable)
>> >> >> >> >>
>> >> >> >> >> It should be possible to remove the virtual dtor from
>> >> >> >> >> FrontendActionFactory hierarchy too, since it's never
>> >> >> >> >> polymorphically
>> >> >> >> >> destroyed, only polymorphically used.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > I would strongly vote against removing something because "it's
>> >> >> >> > not
>> >> >> >> > polymorphically destroyed", where we expect users of the
>> >> >> >> > library
>> >> >> >> > to
>> >> >> >> > own
>> >> >> >> > instances of it. It's very easy to introduce a place where it's
>> >> >> >> > polymorphically destroyed.
>> >> >> >>
>> >> >> >> The same would be true of any type - we don't put virtual dtors
>> >> >> >> on
>> >> >> >> all
>> >> >> >> types in case someone tries to polymorphically destroy them.
>> >> >> >> Clang
>> >> >> >> has
>> >> >> >> warnings (off by default, though, unfortunately
>> >> >> >> -Wdelete-non-virtual-dtor) that can catch this bug if it's ever
>> >> >> >> written.
>> >> >> >
>> >> >> >
>> >> >> > The problem is that if the class is to be used / extended by users
>> >> >> > of
>> >> >> > a
>> >> >> > library, you limit the users' choices when you have a class that
>> >> >> > is
>> >> >> > intended
>> >> >> > to be inherited from, but doesn't have a virtual destructor. If
>> >> >> > clang
>> >> >> > was
>> >> >> > only a program, and not a library, I'd basically agree with most
>> >> >> > points
>> >> >> > you
>> >> >> > make. Since it is a library, I put a lot of emphasis on what
>> >> >> > interface a
>> >> >> > potential user gets, how easy it is to write bugs against that
>> >> >> > interface
>> >> >> > and
>> >> >> > understand that interface, and how much flexibility the user gets
>> >> >> > to
>> >> >> > design
>> >> >> > the software the way they want to.
>> >> >> >
>> >> >> > That means, in a library that is exposed to users:
>> >> >> > - a class that's meant to be inherited from by users should have a
>> >> >> > virtual
>> >> >> > dtor
>> >> >>
>> >> >> While I realize the Tooling API is more API than most of Clang/LLVM
>> >> >> (which is all intended to be a library and is used as such to
>> >> >> varying
>> >> >> degrees) this isn't necessarily the prevailing attitude. It's come
>> >> >> up
>> >> >> a few times that we've wanted to remove virtual dtors from
>> >> >> non-polymorphicalyl owned types but currently warnings stop us from
>> >> >> doing that (-Wvirtual-dtor is on, in part because Google has that on
>> >> >> internally and we didn't have a way to disable that for LLVM without
>> >> >> disabling it for all projects that use LLVM - we now do, so we can
>> >> >> probably turn that warning off in favor of
>> >> >> -Wdelete-non-virtual-dtor).
>> >> >> This has been discussed a few times and seems to be a fairly
>> >> >> unambiguous prevailing attitude.
>> >> >>
>> >> >> http://comments.gmane.org/gmane.comp.compilers.llvm.cv, but now
>> >> >> you're
>> >> >> saying you only theoretically want to do that?s/184558
>> >> >>
>> >> >>
>> >> >> http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/184558
>> >> >>
>> >> >> Which reminds me, your concern about bugs can be alleviated, I
>> >> >> think,
>> >> >> by making the base dtor protected and the derived classes final.
>> >> >
>> >> >
>> >> > Following the links I don't see arguments about specific cases where
>> >> > we
>> >> > want
>> >> > *external* users of an interface to keep polymorphical ownership of a
>> >> > class.
>> >> > Because that's my argument, and if you disagree, I'd like to hear an
>> >> > argument on why you think this is not a useful feature to have for
>> >> > users
>> >> > of
>> >> > this specific interface.
>> >>
>> >> Because it's not a feature of our library. To me, making the dtor
>> >> virtual is the same as making any other function virtual. You do it
>> >> because it's intended to be overriden by users for use cases you have,
>> >> practically speaking, use cases of your library. Our library never
>> >> owns these things so I wouldn't make the dtor virtual.
>> >
>> >
>> > The problem is that other methods don't limit what users of the library
>> > can
>> > do with their own objects,
>>
>> It does to a degree if we put a non-virtual function in a base class
>> (or simply don't include a function (even a pure virtual one) in a
>> base class that a user would like) then the user can't use our
>> interface to invoke those functions polymorphically. If the user wants
>> to dynamically own their things, they can introduce an immediate
>> derived class of FrontendActionFactory and give their derived class a
>> virtual function - then they can own their own factories dynamically.
>> They still can't own the default factories dynamically (just as they
>> can't own vector/list/etc polymorphically).
>>
>> In any case, the first set of patches doesn't actually remove any
>> virtual dtors, it just changes the factories, factories' factory
>> functions, and common usage to not /require/ polymorphic ownership.
>> I'll look at follow up patches to see about protecting and
>> devirtualizing the dtor, etc.
>>
>> > but the destructor does. We intend users to
>> > implement an interface, so taking the possibility away from them to do
>> > ownership the way they like it seems like a rather radical move.
>> >
>> > I am with you for classes where we actually want to take ownership
>> > (let's
>> > assume we'd have a method for users to inject their own AST nodes, but
>> > we
>> > want to take ownership of the nodes they give us). In that case, I'd
>> > also
>> > vote for making the destructor protected.
>>
>> Hmm, not sure I follow the case your describing here. If we took
>> ownership based on some base class, we'd need the base class dtor to
>> be public (or befriended) and virtual.
>>
>> >> >> > - classes and methods shouldn't take ownership, unless they are
>> >> >> > basically
>> >> >> > glorified containers
>> >> >>
>> >> >> The ownership issues with runTool functions I'm not passing judgment
>> >> >> on - it was merely a mechanical change to reflect the reality of the
>> >> >> API as it stands today. If that reality can be changed/fixed, I'm OK
>> >> >> with that.
>> >> >>
>> >> >> >> > Because of that I think having virtual functions
>> >> >> >> > but not having a virtual destructor is an anti-pattern.
>> >> >> >>
>> >> >> >> I don't agree - there are lots of types that are generally not
>> >> >> >> polymorphically owned but are polymorphically used in idioms just
>> >> >> >> like
>> >> >> >> this one. Polymorphic usage doesn't imply polymorphic
>> >> >> >> ownership/destruction.
>> >> >> >
>> >> >> >
>> >> >> > No, it doesn't - but we have style rules because with C++ without
>> >> >> > rules
>> >> >> > it
>> >> >> > is so easy to shoot your foot off that even compiler engineers
>> >> >> > don't
>> >> >> > use
>> >> >> > it
>> >> >> > that way... Having a  rule to always add a virtual dtor if a class
>> >> >> > has
>> >> >> > at
>> >> >> > least one virtual function makes reasoning about code way easier,
>> >> >> > and
>> >> >> > not
>> >> >> > writing a bug is always the least costly variant (even when the
>> >> >> > compiler
>> >> >> > can
>> >> >> > catch it).
>> >> >>
>> >> >> >> > My main problem with the attached patches is actually that it
>> >> >> >> > looks
>> >> >> >> > to
>> >> >> >> > me
>> >> >> >> > like they change ownership in the runTool* functions (if I'm
>> >> >> >> > not
>> >> >> >> > missing
>> >> >> >> > something).
>> >> >> >>
>> >> >> >> Strangely enough, I didn't change the ownership of those
>> >> >> >> functions -
>> >> >> >> that API change just reflects the reality of the API. Notice
>> >> >> >> existing
>> >> >> >> callers (before my patch) passed "Factory.create()" (which
>> >> >> >> returns a
>> >> >> >> pointer the caller should take ownership of - most of the
>> >> >> >> implementations are of the form "return new ...") and the
>> >> >> >
>> >> >> >
>> >> >> > Yes, that was an ownership bug in the existing use cases.
>> >> >>
>> >> >> 'was'? Has it been fixed? (I don't see the fix)
>> >> >>
>> >> >> And where's the bug, exactly? (I just reasoned about the ownership
>> >> >> semantics of the code, I haven't tried to understand what the
>> >> >> /intended/ semantics are)
>> >> >>
>> >> >> I assume the Factory.create() functions are intended to return
>> >> >> ownership (should return std::unique_ptr<>).
>> >> >
>> >> >
>> >> > Yes.
>> >> >
>> >> >>
>> >> >> Or are you thinking that
>> >> >> Factory.create() could return some other kind of RAII wrapper that
>> >> >> actually returns ownership to the Factory so that a singleton
>> >> >> factory
>> >> >> could be used more than once as long as each use was
>> >> >> non-overlapping?
>> >> >>
>> >> >> >> implementation agrees in a somewhat circular way: runToolOnCode
>> >> >> >> creates a ToolInvocation, which builds a
>> >> >> >> SingleFrontendActionFactory
>> >> >> >> which returns the FAction from its 'create()' function... which
>> >> >> >> returns ownership.
>> >> >> >>
>> >> >> >>
>> >> >> >> At least that's my understanding of the ownership model. (other
>> >> >> >> evidence that create() returns ownership - see Tooling.cpp:259,
>> >> >> >> taking
>> >> >> >> the result of create() and initializing a unique_ptr with it)
>> >> >> >
>> >> >> >
>> >> >> > Sure, but that means create() returns ownership, not that runTool*
>> >> >> > should
>> >> >> > take it. I can see the reasons why we would want them to take
>> >> >> > ownership,
>> >> >>
>> >> >> If create returns ownership, how can runTool* not take ownership?
>> >> >
>> >> >
>> >> > std::unique_ptr<FrontendAction> Action(Factory->Create());
>> >> > runToolOnCode(Action.get(), "class X {};", ...);
>> >> >
>> >> > To me this would be the cleanest way to express ownership.
>> >>
>> >> I'm not sure what you're implying here, but if that code change were
>> >> made today it would lead to a double delete. runToolOnCode takes
>> >> ownership (because it takes the parameter and, ultimately, returns it
>> >> from FrontendActionFactory::create, which returns ownership). Are you
>> >> implying some mechanism that would lead to runToolOnCode not taking
>> >> ownership of its parameter? How?
>> >
>> >
>> > Ah, you're right, I missed that. We could of course wrap the
>> > FrontendAction
>> > (and forward the calls), but that seems like a bad solution. I take back
>> > everything I said and claim the opposite ;) (well, not everything...)
>>
>> If a FrontendAction could be wrapped and reused/returned from multiple
>> create() calls, we wouldn't need a factory - we could just use a
>> single FrontendAction all the time. That'd simplify things a bit.
>>
>> >> I don't know this code very well, I was simply propagating the
>> >> ownership semantics that were present in the code, and making them
>> >> explicit in the type system. The code change I suggested reflects the
>> >> existing ownership semantics of the code.
>> >>
>> >> Either we should commit what I've suggested (enshrining the existing
>> >> ownership semantics into the type system, making them more obvious) or
>> >> we need to change the semantics. I don't know what kind of change in
>> >> semantics would be appropriate, but I'm open to
>> >> suggestions/discussion...
>> >>
>> >> >
>> >> >>
>> >> >> In
>> >> >> any case, like I said - I was just changing the API to match the
>> >> >> semantics as they stand today. This is already the API contract -
>> >> >> and
>> >> >> correct callers are already meeting this contract by passing a
>> >> >> 'release()'d unique_ptr to runTool* functions. Changing it to
>> >> >> unique_ptr just makes that more explicit/clear/less error-prone. If
>> >> >> there's some other API design that fixes this in some other way, I'm
>> >> >> OK with that - don't really mind.
>> >> >>
>> >> >> >  but
>> >> >> > I still am torn, because I value the idea that you can use the
>> >> >> > same
>> >> >> > Action
>> >> >> > for multiple runTool* calls.
>> >> >> >
>> >> >> > The original question was about the FrontendActionFactory objects
>> >> >> > though
>> >> >> > -
>> >> >> > here I am still convinced that our interfaces should not take
>> >> >> > ownership.
>> >> >>
>> >> >> I'm confused - I haven't been advocating for FrontendActionFactories
>> >> >> to have ownership passed to Tool.run. My change only changed
>> >> >> Tool.run
>> >> >> from taking a non-owning pointer to a non-owning reference, in both
>> >> >> cases the callee owns the factory.
>> >> >>
>> >> >> My patch was just to demonstrate an answer to Richard Smith's
>> >> >> question:
>> >> >>
>> >> >> "Why do we need to heap-allocate the FrontendActionFactory at all?"
>> >> >>
>> >> >> (which is to say, by way of example, "we don't need to heap-allocate
>> >> >> the FrontendActionFactory at all")
>> >> >
>> >> >
>> >> > well, "for the use cases that are in the clang codebase". To me,
>> >> > interfaces
>> >> > are about making simple things simple for users. If you can patch it
>> >> > into
>> >> > our codebase and make the reverse deps work without making the client
>> >> > code
>> >> > significantly more complex, I'm not opposed to the change (but we
>> >> > might
>> >> > argue about what "more complex" means ...)
>> >>
>> >> Working on it.
>> >
>> >
>> > Cool, looking forward to taking a look at the change. Thanks!
>>
>> Attached the latest patches that preserve the factory factory
>> functions. The reason I asked about those is that they seem almost a
>> bit excessive in a world where they're not polymorphically
>> owning/allocated. Especially the SimpleFrontendActionFactory - why
>> would that need a factory function to build it? So I figured maybe
>> it'd be nice to drop the newFrontendActionFactory functions in the
>> process, if that's the appropriate design currently (even if it means
>> more churn) rather than keeping a backwards compatible-ish API for the
>> sake of lower churn.
>>
>> But if you feel it's the right design (to keep the factory builder
>> helper functions) irrespective of the history here, that's your call -
>> I don't feel strongly about it. I've attached patches that go that
>> way, just having newFrontendActionFactory functions return by value
>> instead of by unique_ptr.
>>
>> Does that look like what you were thinking of? Are you OK with a
>> single commit, or would you like it broken up in any particular ways?
>> (I have it in a few stages in git, but they're a bit jumbled/would
>> take some work to pry them out into nicely modular commits)
>>
>> (you can see in the internal code review the matching changes internally)
>
>
> Would you mind uploading them to phab for review?

Done: http://reviews.llvm.org/D4312
and done: http://reviews.llvm.org/D4313

> Thx!

Thanks!

> /Manuel
>
>
>>
>>
>> - David
>>
>> >
>> >>
>> >>
>> >> - David
>> >>
>> >> >
>> >> > Cheers,
>> >> > /Manuel
>> >> >
>> >> >>
>> >> >>
>> >> >> - David
>> >> >>
>> >> >>
>> >> >> >
>> >> >> > Cheers,
>> >> >> > /Manuel
>> >> >> >
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >> - David
>> >> >> >>
>> >> >> >> >
>> >> >> >> > Cheers,
>> >> >> >> > /Manuel
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> - David
>> >> >> >> >>
>> >> >> >> >> >
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >> > that we want to work
>> >> >> >> >> >> > with pointers (because they have identity - we want the
>> >> >> >> >> >> > same
>> >> >> >> >> >> > one
>> >> >> >> >> >> > to
>> >> >> >> >> >> > be
>> >> >> >> >> >> > passable to different runs and work with the data).
>> >> >> >> >> >>
>> >> >> >> >> >> In terms of pointer identity - the idea would be to create
>> >> >> >> >> >> a
>> >> >> >> >> >> FrontendActionFactory as a direct (rather than unique_ptr)
>> >> >> >> >> >> local
>> >> >> >> >> >> variable and pass it to each run call - the object would
>> >> >> >> >> >> never
>> >> >> >> >> >> move
>> >> >> >> >> >> around, so pointers to it would remain valid throughout its
>> >> >> >> >> >> lifetime.
>> >> >> >> >> >>
>> >> >> >> >> >> Or am I picturing something different from what you have in
>> >> >> >> >> >> mind?
>> >> >> >> >> >>
>> >> >> >> >> >> - David
>> >> >> >> >> >>
>> >> >> >> >> >> >
>> >> >> >> >> >> >>
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> _______________________________________________
>> >> >> >> >> >> >> cfe-dev mailing list
>> >> >> >> >> >> >> cfe-dev at cs.uiuc.edu
>> >> >> >> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> >> >> >> >> >> >>
>> >> >> >> >> >> >
>> >> >> >> >> >> >
>> >> >> >> >> >> > _______________________________________________
>> >> >> >> >> >> > cfe-dev mailing list
>> >> >> >> >> >> > cfe-dev at cs.uiuc.edu
>> >> >> >> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> >> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>