rsmith added inline comments.
================
Comment at: clang/docs/LanguageExtensions.rst:2434
+
+ bool print(int arg1, int arg2, std::initializer_list<Field> fields) {
+ for (auto f : fields) {
----------------
erichkeane wrote:
> 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.
Suppose you want to dump a struct to a log file, or you want the dump in a
string. Or you want to carry along an indentation level. My first attempt at
using `__builtin_dump_struct` essentially failed because it doesn't have
anything like this; even if we don't want this patch, this is functionality we
should add to that builtin.
================
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
----------------
erichkeane wrote:
> 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.
>
>
I think that would work. I'm not sure it would be better, though, given that it
loses the ability to build a type whose shape depends on the number of bases
and fields, because you're not given them all at once. (Eg, if you want to
build a static array of field names.)
================
Comment at: clang/docs/LanguageExtensions.rst:2473
+
+* For each direct base class, the address of the base class, in declaration
+ order.
----------------
erichkeane wrote:
> 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.
The use cases are the things that `__builtin_dump_struct` gets close to but
fails at, such as: I have an `EXPECT_EQ` macro that I'd like to log the
difference in input and output values where possible, or I want to build a
simple print-to-string utility for structs. Or I want a dump to stout but I
want it formatted differently from what `__builtin_dump_struct` thinks is best.
The fact that the dump built-in gets us into this area but can't solve these
problems is frustrating.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:420
+ auto *FD = IFD ? IFD->getAnonField() : dyn_cast<FieldDecl>(D);
+ if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion()))
+ continue;
----------------
erichkeane wrote:
> 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.
>
>
Why? They're not part of the value, they're just padding.
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
