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
----------------
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.
> 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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135557/new/
https://reviews.llvm.org/D135557
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
