erichkeane 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) {
----------------
rsmith wrote:
> 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.
Ah, I see! Sure, I can see how those are useful, though again, mostly to the C
implementation (as those should all be part of the functor's state), and this
doesn't work particularly well in C.
================
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:
> > 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.)
Yep, thats definitely true. There are ways to DO that (either a 2-pass at this
to get the sizes, allocate something, then go through again filling in your
struct OR a constant 'append' thing with a `vector` like type), but I see what
you mean. It seems that either case we are limiting a couple of useful use
cases.
================
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:
> > 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.
>>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
THAT is an interesting use case, I see value to that too.
>>The fact that the dump built-in gets us into this area but can't solve these
>>problems is frustrating.
I definitely see that. FWIW, I don't think I was the one to approve/review the
initial version of `__builtin_dump_struct` (and perhaps would have been equally
frustrated if I thought that through), but was looking into the patches after
the fact to help out.
I guess I find myself equally/additionally/etc frustrated at THIS patch because
it gets us so close to everything SG7 promises they can invent.
================
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:
> > 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.
They are lexically part of the type and are part of what makes up the 'size' of
the thing. I would expect something dumping the type to be as consistent
lexically as possible.
================
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:
> > rsmith wrote:
> > > 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.
> > They are lexically part of the type and are part of what makes up the
> > 'size' of the thing. I would expect something dumping the type to be as
> > consistent lexically as possible.
> Looks like `__builtin_dump_struct` currently includes them *and* prints a
> value (whatever garbage happens to be in those padding bits). That's
> obviously a bug.
O.o! Yeah, printing the value is nonsense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124221/new/
https://reviews.llvm.org/D124221
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
