[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Erich Keane via Phabricator via cfe-commits]

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