erichkeane added inline comments.
================
Comment at: clang/docs/LanguageExtensions.rst:2442
+
+This builtin does not return a value.
+
----------------
rsmith wrote:
> erichkeane wrote:
> > I don't know if anyone would be using this value, but I wonder if there is
> > value to making this a 'sum' of the results of `f`? Less useful for the
> > purposes of printf, but perhaps to a sprintf-type-function?
> >
> > Not sure if we have the use cases to support this however.
> I don't think you can use this directly with `sprintf` -- you'd need a
> wrapper to update the write position as part of each call to `f`, and that
> wrapper can handle accumulating the value returned from `sprintf`. In order
> to do that, you'd want to provide an additional input argument that can be
> modified as the dump progresses, and that can be used to return the overall
> (desired) size too. So I don't think even the `sprintf` case provides
> motivation for this, and it would add complexity (as well as making the
> builtin less usable in the case where the printing function doesn't return
> something addable).
>
> I could see this being at least a little useful for a formatting function
> that returns `std::string` or similar, but ... I think it's probably better
> to let the formatting function deal with that itself through argument passing
> instead of assuming that we can apply `+` to the results. WDYT?
I agree that we're probably better off just making the user write a wrapper
function/functor to do so. I was thinking about C folks, but you're right,
they could pass a state object into their extended args.
I think we probably should just hold off on any attempts to do this until we
find someone with a bonafide need.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:595
+ PseudoObjectExpr::NoResult);
+ TheCall->setType(Wrapper->getType());
+ TheCall->setValueKind(Wrapper->getValueKind());
----------------
rsmith wrote:
> erichkeane wrote:
> > What is happening here? I would expect the opposite to need to happen here
> > (wrapper be set to match the call, not the other way around?).
> This is the normal way we handle builtins with custom type checking: the
> `SemaChecking` logic analyzes the builtin and sets the result type of the
> call as appropriate, because the type we get from `Builtins.def` is
> meaningless. As it happens, this is a degenerate case (compared to
> `__builtin_reflect_struct` where this did something more interesting),
> because the type of the `PseudoObjectExpr` is always `void` here; I can just
> set that directly if you think that's clearer. It shouldn't matter much
> anyway, given that the call is only used as the syntactic form of the
> `PseudoObjectExpr`, so its type doesn't really affect anything. but following
> the type and value kind of the `PseudoObjectExpr` seems "right" to me.
Ah, I see some other examples now, thanks for the explanation. I don't think
setting to `void` is a good idea, in case we find a good reason to change the
type; this ends up being just one more place we need to mess with it.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:614
+ if (!PtrArgType->isPointerType() ||
+ !PtrArgType->getPointeeType()->isRecordType()) {
+ S.Diag(PtrArgResult.get()->getBeginLoc(),
----------------
rsmith wrote:
> erichkeane wrote:
> > Is this sufficient? Couldn't this be a pointer to a union? Do you mean
> > the suggestion?
> Allowing this is what the existing builtin did, and there were tests for it.
>
> I'm not sure what the best thing to do with unions is -- especially when a
> union appears as a struct member. Perhaps the least bad approach is to print
> out all the union members and hope that nothing goes wrong. That'll crash in
> some cases (eg, `union { const char *p; int n; };` when the `int` is the
> active member), but that seems to be the nature of this builtin.
>
> I've added a FIXME to `dumpRecordValue`.
SGTM! We DO document a few times IIRC that we accept a 'struct' param(in fact,
the comment on line 637 n ow says we do!), so I wanted to make sure we are
consistent there.
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
