Problem with a change on newFrontendActionFactory

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>()));

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

Regards,
Etienne

Hi Etienne,

You're right - this change was made very recently and the documentation has
not been updated yet. Would you like to contribute a documentation patch to
fix this?

Eli

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...

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.

Ah, I was starting to reach that conclusion.

So 'run' isn't meant to take ownership? (seems fair, since it doesn't
need to copy the pointer/save it beyond the life of the function)

Where possible I've been trying to transform such APIs (that take a
non-owning pointer to an object) to take a reference, that way they
work seamlessly with unique_ptr or raw pointers (in both cases you
just dereference the thing and pass it to the other thing). JUst a
thought, not a request for action, etc.

- David

Why do we need to heap-allocate the FrontendActionFactory at all?

>>>
>>> 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 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).

>>>
>>> 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.

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

>>
>>>
>>> >>>
>>> >>> 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? 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.

Also, there is an overload newFrontendActionFactory<FrontendActionType>(),
and I think it has value that those form similar patterns.

>>
>>>
>>> >>>
>>> >>> 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.

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? 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.

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.

- David

frontend_action_factory_ownership_value.diff (40.2 KB)

frontend_action_factory_ownership_value_extra.diff (22.3 KB)

>>
>> >>
>> >>>
>> >>> >>>
>> >>> >>> 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.

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).

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.

> 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. Because of that I think having virtual functions
but not having a virtual destructor is an anti-pattern.

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).

Cheers,
/Manuel

>>
>> >>
>> >>>
>> >>> >>>
>> >>> >>> 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);

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:

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.

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.

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
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)

- David

>>
>> >>
>> >> >>
>> >> >>>
>> >> >>> >>>
>> >> >>> >>> 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.

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.

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
- classes and methods shouldn't take ownership, unless they are basically
glorified containers

> 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.

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,
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.

Cheers,
/Manuel

>>
>> >>
>> >> >>
>> >> >>>
>> >> >>> >>>
>> >> >>> >>> 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.

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?

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.cvs/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.

- 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<>). 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? 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")

- David

Ping

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.

>>
>> >>
>> >> >>
>> >> >> >>
>> >> >> >>>
>> >> >> >>> >>>
>> >> >> >>> >>> 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&gt;

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.

> - 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.

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 ...)

Cheers,
/Manuel

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.

>>
>> >>
>> >> >>
>> >> >> >>
>> >> >> >>>
>> >> >> >>> >>>
>> >> >> >>> >>> 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.

> - 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?

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.

- David

> 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.
>
>>
>> >>
>> >> >>
>> >> >> >>
>> >> >> >> >>
>> >> >> >> >>>
>> >> >> >> >>> >>>
>> >> >> >> >>> >>> 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, 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.

>> > - 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 :wink: (well, not everything...)

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!

> 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.
>
>>
>> >>
>> >> >>
>> >> >> >>
>> >> >> >> >>
>> >> >> >> >>>
>> >> >> >> >>> >>>
>> >> >> >> >>> >>> 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 :wink: (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)

- David

frontend_action_factory_ownership_value.diff (40.7 KB)

frontend_action_factory_ownership_value_extra.diff (21.8 KB)