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