erichkeane added a comment.
>> I think you've misunderstood; that is not required. Though with the design
>> as-is, it might make sense to restrict this to being C++-only, given that
>> there's not really a way to effectively use this from C.
Ah, I see the example in SemaCXX. its sad we can't do this from C, I would
consider the use case of `__builtin_dump_struct` to be very handy in C.
I guess I think I would want to start off with, "what do we see the 'usecase'
of these builtins" being. `__builtin_dump_struct` to me seems to have a pretty
clear and semantically enforced one: its useful to printf it for debugging
purposes.
This seems to be more useful-enough that it leaves me feeling like users would
very quickly fall into, "man, I wish this did more" because of it. Its just a
shame we can't get SG7 to work quicker :)
================
Comment at: clang/docs/LanguageExtensions.rst:2434
+
+ bool print(int arg1, int arg2, std::initializer_list<Field> fields) {
+ for (auto f : fields) {
----------------
rsmith wrote:
> erichkeane wrote:
> > I'm curious as to the need for the 'arg1' and 'arg2'? Also, what is the
> > type of the 'fields' array that works in C?
> This is aimed to fix a major deficiency in `__builtin_dump_struct`, that
> there's no way to pass information into its callback (other than stashing it
> in TLS).
>
> As noted in the patch description, this builtin is not especially usable in
> C. I'm not sure whether that's fixable, given the very limited
> metaprogramming support in C.
Presumably one could also put it in the functor for the callback, or even as a
capture in the lambda. Though, I'm not positive I see the VALUE to passing
this information, but was just curious as to the intent.
================
Comment at: clang/docs/LanguageExtensions.rst:2437
+ std::cout << f.name;
+ if (f.bitwidth)
+ std::cout << " : " << f.bitwidth;
----------------
rsmith wrote:
> erichkeane wrote:
> > Hmm... what is the type of f.bitwidth? A zero-length bitwidth is a valid
> > thing, so having it just be an 'int' doesn't seem workable.
> It's a `size_t`. There are no zero-size bitfield members. (Unnamed bitfield
> aren't members, so are excluded here.) In any case, you can tell the
> difference between a bitfield and a regular member by the length of the
> initialiser list; we don't pass a bit width for non-bit-field members.
>>There are no zero-size bitfield members. (Unnamed bitfield aren't members, so
>>are excluded here.)
Hrm... Semantically YES, lexically I'm not sure folks make that
differentiation. One issue with skipping 'unnamed bitfields' is it makes
something like:
```
struct Foo {
int : 31; // Take up some space so something else could have been put there.
int field : 1;
int field2 : 2;
};
```
and comparing `sizeof` the type with the output to be... jarring at least.
>> you can tell the difference between a bitfield and a regular member by the
>> length of the initialiser list;
I see now how you've done that with ctors in C++. It feels that the way of
doing this in C is unfortunately quite clunky (in addition, not being able to
use arbitrary types).
================
Comment at: clang/docs/LanguageExtensions.rst:2462
+to the type to reflect. The second argument ``f`` should be some callable
+expression, and can be a function object or an overload set. The builtin calls
+``f``, passing any further arguments ``args...`` followed by an initializer
----------------
rsmith wrote:
> erichkeane wrote:
> > Is the purpose of the overload set/functor so you can support multiple
> > types (seeing how the 'base' below works)? How does this all work in C?
> > (or do we just do fields in C, since they cannot have the rest?
> >
> > I wonder if we would be better if we treated this function as a 'callback'
> > or a 'callback set' that returned a discriminated union of a pre-defined
> > type that describes what is being returned. That way it could handle
> > member functions, static variables, etc.
> This doesn't really work well in C, as noted in the patch description. A set
> of callbacks might work a bit better across both languages, but I don't think
> it solves the larger problem that there's no good way to pass type
> information into a callback in C.
Well, if the callback itself took the discriminated union for each thing, and
we did 1 callback per base/field/etc, perhaps that would be useful? I just am
having a hard time seeing this being all that useful in C, which is a shame.
================
Comment at: clang/docs/LanguageExtensions.rst:2471
+
+The initializer list contains the following components:
+
----------------
rsmith wrote:
> erichkeane wrote:
> > I find myself wishing this was a more complete list.
> What else do you want? My goal here was to be able to do what
> `__builtin_dump_struct` does but without its many arbitrary limitations. From
> C++, this is enough for that.
I think this is again falling down the "this is close enough to reflection, I
want reflection" hole :) I find myself wanting to be able to introspect member
functions, static variables/functions/etc.
================
Comment at: clang/docs/LanguageExtensions.rst:2473
+
+* For each direct base class, the address of the base class, in declaration
+ order.
----------------
rsmith wrote:
> erichkeane wrote:
> > I wonder if bases should include their virtualness or access specifier.
> I mean, maybe, but I don't have a use case for either. For access in general
> my leaning is to say that the built-in doesn't get special access rights --
> either it can access everything or you'll get an access error if you use it
> -- in which case including the access for bases and fields doesn't seem
> likely to be super useful.
I guess it depends on the use case for these builtins. I saw the 'dump-struct'
builtin's purpose was for quick & dirty printf-debugging. At which point the
format/features were less important than the action. So from that perspective,
I would see this having 'special access rights' as sort of a necessity.
Though again, this is a significantly more general builtin, which comes with
additional potential use cases.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:420
+ auto *FD = IFD ? IFD->getAnonField() : dyn_cast<FieldDecl>(D);
+ if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion()))
+ continue;
----------------
rsmith wrote:
> erichkeane wrote:
> > Losing the unnamed bitfield is unfortunate, and I the 'dump struct'
> > handles. As is losing the anonymous struct/union.
> >
> > Also, how does this handle 'record type' members? Does the user have to
> > know the inner type already? If they already know that much information
> > about the type, they wouldn't really need this, right?
> If `__builtin_dump_struct` includes unnamed bitfields, that's a bug in that
> builtin.
>
> In general, the user function gets given the bases and members with the right
> types, so they can use an overload set or a template to dispatch based on
> that type. See the SemaCXX test for a basic example of that.
I thought it did? For the use case I see `__builtin_dump_struct` having, I
would think printing the existence of unnamed bitfields to be quite useful.
================
Comment at: clang/test/CodeGenCXX/builtin-reflect-struct.cpp:24
+
+int consume(int, Expected);
+
----------------
rsmith wrote:
> erichkeane wrote:
> > OH! I see now. This is unfortunately quite limiting by doing it this way.
> > At this point the user of this builtin is required to know the layout of
> > the struct before calling the builtin, which seems unfortunate. Despite the
> > downsides of being just a 'dump function', the other builtin had the
> > benefit of working on an arbitrary type.
> Well, no, this is set up this way to test code generation. See the SemaCXX
> test for an example of using this to dump an arbitrary type.
Yep, thanks for that. I see now how it can be used that way.
================
Comment at: clang/test/SemaCXX/builtin-reflect-struct.cpp:52
+
+struct FieldOrBase {
+ template <typename Base>
----------------
Ah, this is perhaps the example I was looking for.
================
Comment at: clang/test/SemaCXX/builtin-reflect-struct.cpp:82
+ // Assume it's an integer.
+ T value = t;
+ ConstexprString s = "";
----------------
What would this look like if you needed to look into a union? Perhaps it would
be handled similarly to "__is_class"? In both builtins, its a shame we don't
have a way to properly handle the 'active member'.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124221/new/
https://reviews.llvm.org/D124221
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
