dblaikie 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
----------------
aaron.ballman wrote:
> dblaikie wrote:
> > anderslanglands wrote:
> > > 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`
> > @royjacobson it's also going to be pretty problematic to instantiate those
> > templates as members too.
> >
> > The AST is pretty accurately reflective of the compiler's understanding of
> > the type at the time - adding extra instantiations isn't necessarily an
> > improvement in fidelity. I'd argue it's a loss in fidelity because these
> > things weren't instantiated by the original code.
> >
> > (types won't be entirely consistent/stable - member function templates are
> > a great example - you can add code that'll cause new instantiations, and
> > there's no way to fully enumerate all possible instantiations. So it's not
> > a goal for a type description to be stable regardless of the code that uses
> > the type)
> To my thinking, there's a difference between "is this copy constructible" and
> "does this have a copy constructor (even implicitly)". My understanding of
> what @anderslanglands is trying to do is to find classes that have a special
> member function so that the wrapper can expose the same functionality. That's
> not "is this copy constructible", that's "does this have a copy constructor
> (even implicitly)."
>
> The C++ standard specifies when classes get implicit special member functions
> and our AST does not reflect that accurately unless the class is being used:
> https://godbolt.org/z/13h3T3dPq. Both have definition data that accurately
> reflects whether the class could get those special members (and that
> definition data is an internal implementation detail), but only one of those
> classes have AST nodes for the special members, which means trying to run an
> AST matcher query for every class with a constructor gives unexpected
> results: https://godbolt.org/z/PhMY57anM
Perhaps a simple test: @anderslanglands, if the class had a ctor template that
could be used for copying the type - would that meet your requirements? Would
you want to write a wrapper that invoked/instantiated that template to perform
a copy? Or would you want your wrapper not to expose copy construction?
If it does, then the property of interest is "is this type copy constructible"
and not "does this type have a copy constructor", I think?
All that said, I don't outright object to the implicit special members being
eagerly generated if it doesn't cost much compile time/memory usage/etc, but
I'm certainly a little hesitant. Totally your call, @aaron.ballman
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
