[cfe-dev] Problem with a change on newFrontendActionFactory

Thu Jun 26 00:52:35 PDT 2014

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?

Thx!
/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
> >> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140626/cb222f7e/attachment.html>