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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits