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

Reply via email to