[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

Fri Oct 14 12:24:44 PDT 2022

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:
> > 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)?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135557/new/

https://reviews.llvm.org/D135557