anderslanglands added inline comments.
================
Comment at: clang/bindings/python/clang/cindex.py:1530
+
+ def record_needs_implicit_default_constructor(self):
+ """Returns True if the cursor refers to a C++ record declaration
----------------
dblaikie wrote:
> royjacobson wrote:
> > anderslanglands wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > dblaikie wrote:
> > > > > > anderslanglands wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > royjacobson wrote:
> > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > anderslanglands wrote:
> > > > > > > > > > > > > royjacobson wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > I don't think we should expose any of the
> > > > > > > > > > > > > > > > "needs" functions like this -- those are
> > > > > > > > > > > > > > > > internal implementation details of the class
> > > > > > > > > > > > > > > > and I don't think we want to calcify that into
> > > > > > > > > > > > > > > > something we have to support forever. As we add
> > > > > > > > > > > > > > > > members to a class, we recalculate whether the
> > > > > > > > > > > > > > > > added member causes us to delete defaulted
> > > > > > > > > > > > > > > > special members (among other things), and the
> > > > > > > > > > > > > > > > "needs" functions are basically used when the
> > > > > > > > > > > > > > > > class is completed to handle lazily created
> > > > > > > > > > > > > > > > special members. I'm pretty sure that lazy
> > > > > > > > > > > > > > > > creation is not mandated by the standard, which
> > > > > > > > > > > > > > > > is why I think the "needs" functions are more
> > > > > > > > > > > > > > > > of an implementation detail.
> > > > > > > > > > > > > > > CC @erichkeane and @royjacobson as folks who have
> > > > > > > > > > > > > > > been in this same area of the compiler to see if
> > > > > > > > > > > > > > > they agree or disagree with my assessment there.
> > > > > > > > > > > > > > I think so. The 'needs_*' functions query
> > > > > > > > > > > > > > `DeclaredSpecialMembers` and I'm pretty sure it's
> > > > > > > > > > > > > > modified when we add the implicit definitions in
> > > > > > > > > > > > > > the class completion code. So this looks a bit
> > > > > > > > > > > > > > suspicious. Is this API //meant// to be used with
> > > > > > > > > > > > > > incomplete classes?
> > > > > > > > > > > > > > For complete classes I think looking up the
> > > > > > > > > > > > > > default/move/copy constructor and calling
> > > > > > > > > > > > > > `isImplicit()` is the way to do it.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > About the 'is deleted' API - can't the same be done
> > > > > > > > > > > > > > for those functions as well so we have a smaller
> > > > > > > > > > > > > > API?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > If this //is// meant to be used with incomplete
> > > > > > > > > > > > > > classes for efficiency that would be another thing,
> > > > > > > > > > > > > > I guess.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > So the intended use case here is I'm using libclang
> > > > > > > > > > > > > to parse an existing C++ libray's headers and
> > > > > > > > > > > > > generate a C interface to it. To do that I need to
> > > > > > > > > > > > > know if I need to generate default constructors etc,
> > > > > > > > > > > > > which the needs* methods do for me (I believe). The
> > > > > > > > > > > > > alternative is I have to check manually whether all
> > > > > > > > > > > > > the constructors/assignment operators exist, then
> > > > > > > > > > > > > implement the implicit declaration rules myself
> > > > > > > > > > > > > correctly for each version of the standard, which I'd
> > > > > > > > > > > > > rather avoid.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Would putting a note in the doc comment about the
> > > > > > > > > > > > > behaviour differing when the class is being
> > > > > > > > > > > > > constructed as originally suggested work for everyone?
> > > > > > > > > > > > Why is the `__is_default_constructible` builtin type
> > > > > > > > > > > > trait not enough? Do you have different behavior for
> > > > > > > > > > > > user provided and implicit default constructors?
> > > > > > > > > > > >
> > > > > > > > > > > Can I evaluate that from libclang somewhow? I can't
> > > > > > > > > > > modify the C++ libraries I'm wrapping.
> > > > > > > > > > >
> > > > > > > > > > > Basically, given:
> > > > > > > > > > > ```
> > > > > > > > > > > struct Foo { /* ... */ };
> > > > > > > > > > > ```
> > > > > > > > > > >
> > > > > > > > > > > I want to generate:
> > > > > > > > > > >
> > > > > > > > > > > ```
> > > > > > > > > > > typedef struct Foo_t;
> > > > > > > > > > >
> > > > > > > > > > > Foo_t* Foo_ctor();
> > > > > > > > > > > Foo_t* Foo_copy_ctor(Foo_t*);
> > > > > > > > > > > /* etc... */
> > > > > > > > > > > Foo_dtor(Foo_t*);
> > > > > > > > > > > ```
> > > > > > > > > > >
> > > > > > > > > > > In order to know which ones to generate for an arbitrary
> > > > > > > > > > > struct that may or may not have any combination of
> > > > > > > > > > > ctor/assignments defined, I need to know which ones exist
> > > > > > > > > > > and follow the implicit generation rules for the ones
> > > > > > > > > > > that don't. I can do this myself with a whole bunch of
> > > > > > > > > > > version-dependent logic, but I'd rather just rely on
> > > > > > > > > > > libclang since it already knows all this much better than
> > > > > > > > > > > I do.
> > > > > > > > > > I looked a bit, and it seems they aren't, and that
> > > > > > > > > > generally libclang doesn't really know about Sema, so
> > > > > > > > > > exporting the type traits is not that easy :/
> > > > > > > > > >
> > > > > > > > > > I'm not sure what's the best way forward here, but I don't
> > > > > > > > > > like the idea of exporting those half baked internal API
> > > > > > > > > > calls when there are actual standardized and implemented
> > > > > > > > > > type traits that perform the same goal.
> > > > > > > > > CCing folks who may have more historical memory of the C APIs
> > > > > > > > > and whether they're expected to operate on a completed AST or
> > > > > > > > > are expected to work on an AST as it is under construction.
> > > > > > > > > My unverified belief is that these APIs are expected to work
> > > > > > > > > on a completed AST.
> > > > > > > > >
> > > > > > > > > @echristo @dblaikie @rjmccall @rsmith
> > > > > > > > >
> > > > > > > > > I'm also not certain of what the best path forward is here.
> > > > > > > > > I'm not comfortable exposing the needs* functions because
> > > > > > > > > they really are implementation details and I don't want to
> > > > > > > > > promise we'll support that API forever. But at the same time,
> > > > > > > > > the use case is reasonably compelling on the assumption you
> > > > > > > > > need to inspect the AST nodes as they're still under
> > > > > > > > > construction instead of inspecting them once the AST is
> > > > > > > > > completed. If the AST is fully constructed, then we should
> > > > > > > > > have already added the AST nodes for any special member
> > > > > > > > > functions that needed to be generated implicitly, so as Roy
> > > > > > > > > mentioned, you should be able to find the special member
> > > > > > > > > function you're after and check `isImplicit()` on it.
> > > > > > > > Not sure I'm quite following - it doesn't look (admittedly,
> > > > > > > > sorry, at a somewhat superficial look at the discussion here)
> > > > > > > > like this is necessarily about incomplete AST - could parse the
> > > > > > > > header and stop. That's a complete AST, yeah? And then it might
> > > > > > > > be OK/reasonable to ask "could this type be default
> > > > > > > > constructed" (even if the implicit ctor has been implicitly
> > > > > > > > instantiated/there was no use in the source code that's been
> > > > > > > > parsed)
> > > > > > > >
> > > > > > > > Is that correct?
> > > > > > > I am just parsing the headers of a library using
> > > > > > > `clang_parseTranslationUnit()` then using `clang_visitChildren()`
> > > > > > > to inspect the AST. Doing this I do NOT see any implicitly
> > > > > > > generated methods, hence why I need these functions. It sounds
> > > > > > > like you expect those methods to be in the AST already? Which
> > > > > > > suggests I'm doing something wrong in my parsing (missing another
> > > > > > > function call/option or something)?
> > > > > > Ah, no, I wouldn't expect them to be in the AST unless there was
> > > > > > code that used them in the header.
> > > > > >
> > > > > > But I'm saying/trying to say is this isn't a "AST nodes still under
> > > > > > construction" that @aaron.ballman is describing, so far as I can
> > > > > > see - you can completely parse the header, have a complete AST then
> > > > > > reasonably want to ask "could I default construct an object like
> > > > > > this" - even if the implicit default ctor hasn't been instantiated
> > > > > > because none of the parsed code asked that question.
> > > > > >
> > > > > > Not sure what the API to do this should look like, but it seems
> > > > > > like a pretty reasonable use case.
> > > > > >
> > > > > > Not sure about whether they cross some threshold where they're too
> > > > > > complex/nuanced to go in the C API or not - maybe that's a question.
> > > > > >> I am just parsing the headers of a library using
> > > > > >> clang_parseTranslationUnit() then using clang_visitChildren() to
> > > > > >> inspect the AST. Doing this I do NOT see any implicitly generated
> > > > > >> methods, hence why I need these functions. It sounds like you
> > > > > >> expect those methods to be in the AST already? Which suggests I'm
> > > > > >> doing something wrong in my parsing (missing another function
> > > > > >> call/option or something)?
> > > > > > Ah, no, I wouldn't expect them to be in the AST unless there was
> > > > > > code that used them in the header.
> > > > >
> > > > > That's part of the "this is an implementation detail" I was talking
> > > > > about. *Today* we don't generate the AST nodes for those functions
> > > > > unless we have to. Nothing says we won't find a reason we need to
> > > > > always generate those AST nodes, which makes the `needs*` functions
> > > > > useless. I suppose in that situation, the breakage for the C APIs is
> > > > > mostly that the exposed `needs*` functions start trivially returning
> > > > > `false` though, so maybe it's not as bad as it could be...
> > > > >
> > > > > > But I'm saying/trying to say is this isn't a "AST nodes still under
> > > > > > construction" that @aaron.ballman is describing, so far as I can
> > > > > > see - you can completely parse the header, have a complete AST then
> > > > > > reasonably want to ask "could I default construct an object like
> > > > > > this" - even if the implicit default ctor hasn't been instantiated
> > > > > > because none of the parsed code asked that question.
> > > > >
> > > > > Yeah, the situation I mentioned earlier was the validity of the calls
> > > > > when the class has not been fully constructed in the AST yet. That's
> > > > > not the case here, which is great.
> > > > >
> > > > > > Not sure what the API to do this should look like, but it seems
> > > > > > like a pretty reasonable use case.
> > > > >
> > > > > Agreed that the use case is reasonable.
> > > > >
> > > > > > Not sure about whether they cross some threshold where they're too
> > > > > > complex/nuanced to go in the C API or not - maybe that's a question.
> > > > >
> > > > > Mostly, I think we try to expose APIs that we think we can support
> > > > > long-term based on what needs folks have. Given that there's a need
> > > > > here, and the use case seems reasonable, it seems to be something we
> > > > > should consider supporting.
> > > > >
> > > > > I suppose there's another way we could view this need though -- some
> > > > > folks need those special member functions even if Clang doesn't think
> > > > > they're necessary to generate. Not only is this use case one such
> > > > > time, but running AST matchers over the AST (like in clang-query or
> > > > > clang-tidy) may also have a similar expectation of finding all the
> > > > > special members. So maybe what we need is some flag to tell Clang
> > > > > "force the generation of those special member functions" so that we
> > > > > don't have to expose a `needs` function for them (which helps for the
> > > > > C API users but doesn't help folks like consumers of AST matchers).
> > > > > (Note, I don't yet know how good or bad of an idea this is.)
> > > > Yeah - if someone is interested in doing the work, I'd be curious how
> > > > some equivalent operations work in the AST matchers? I'd assume there's
> > > > some way to query if something is copy constructible - and maybe that's
> > > > more likely to be the query the user wants, rather than the "needs"
> > > > operations?
> > > >
> > > > (like, if we did add the implicit copy constructors into the AST
> > > > proactively, I don't think I'd want these queries to return "false" - I
> > > > think likely the intended query is "is this thing copy constructible"
> > > > (or similar) less about whether the operation is or isn't present in
> > > > the AST)
> > > In my case it's "do I need to generate a copy ctor for this type?".
> > > @aaron.ballman 's suggestion of a way to force the implicits to be
> > > generated in the AST would work just fine for me.
> > > That's part of the "this is an implementation detail" I was talking
> > > about. *Today* we don't generate the AST nodes for those functions unless
> > > we have to. Nothing says we won't find a reason we need to always
> > > generate those AST nodes, which makes the `needs*` functions useless. I
> > > suppose in that situation, the breakage for the C APIs is mostly that the
> > > exposed `needs*` functions start trivially returning `false` though, so
> > > maybe it's not as bad as it could be...
> > >
> > > ...
> > >
> > > Yeah, the situation I mentioned earlier was the validity of the calls
> > > when the class has not been fully constructed in the AST yet. That's not
> > > the case here, which is great.
> > >
> >
> > This is not even about future proofing - this is already bad API. Simply
> > adding
> >
> > ```
> > void f() {
> > Test t;
> > }
> > ```
> >
> > in the test in this PR is changing the printed line from
> > `ClassDecl=Test:3:7 (Definition) (needs ctor) (needs cctor) (needs mctor)
> > (needs cassign) (needs massign) (needs dtor) Extent=[3:1 - 17:2]`
> > to
> > `ClassDecl=Test:3:7 (Definition) (needs cassign) (needs massign) (needs
> > dtor) Extent=[3:1 - 17:2]`
> >
> > I don't think making functions in libclang conditional on whether somewhere
> > in the headers types are actually used or not is likely to provide value.
> > It's impossible to enforce non-use of a type if it's definition is
> > available and it's very unnatural to C++ to rely on it.
> >
> > I'm now also pessimistic about the possibility of implementing **correct**
> > versions of those `std::is...` type traits without Sema. Default
> > constructors might be template functions that are SFINAE-disabled, for
> > example. This isn't very exotic - the default constructors of pair,
> > optional, etc.. are all implemented like this. The other type traits that
> > we'd want to expose are also pretty similar.
> >
> > A solution might be useful even if it doesn't handle all cases correctly,
> > of course. But IMHO in this case an approach with only AST would be too
> > partial to justify its shortcomings.
> >
> > In my case it's "do I need to generate a copy ctor for this type?".
> > @aaron.ballman 's suggestion of a way to force the implicits to be
> > generated in the AST would work just fine for me.
>
> But by "generate" you mean "generate a wrapper for this operation", yeah?
>
> If you could query the type for "is this type copy constructible", "is this
> type copy assignable", etc, be adequate for your needs?
> > In my case it's "do I need to generate a copy ctor for this type?".
> > @aaron.ballman 's suggestion of a way to force the implicits to be
> > generated in the AST would work just fine for me.
>
> But by "generate" you mean "generate a wrapper for this operation", yeah?
>
> If you could query the type for "is this type copy constructible", "is this
> type copy assignable", etc, be adequate for your needs?
Thinking about it some more, yes I think it probably would. I would have to do
some minor book-keeping to track whether there was one already declared on the
class or not, but that's a lot simpler than reimplementing the implicit rules.
I guess I would need `isDefaultConstructible`, `isCopyConstructible`,
`isMoveConstructible`, `isCopyAssignable`, and `isMoveAssignable`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135557/new/
https://reviews.llvm.org/D135557
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
