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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This is a pretty nice seeming interface that I think does a fairly good job at maintaining backwards compat while improving the functionality. A few questions/comments.



================
Comment at: clang/docs/LanguageExtensions.rst:2442
+
+This builtin does not return a value.
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:558
+      auto *InnerRD = FD->getType()->getAsRecordDecl();
+      auto *InnerCXXRD = InnerRD ? dyn_cast<CXXRecordDecl>(InnerRD) : nullptr;
+      if (InnerRD && (!InnerCXXRD || InnerCXXRD->isAggregate())) {
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:595
+                                             PseudoObjectExpr::NoResult);
+    TheCall->setType(Wrapper->getType());
+    TheCall->setValueKind(Wrapper->getValueKind());
----------------
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?).  


================
Comment at: clang/lib/Sema/SemaChecking.cpp:603
+static ExprResult SemaBuiltinDumpStruct(Sema &S, CallExpr *TheCall) {
+  if (TheCall->getNumArgs() < 2 && checkArgCount(S, TheCall, 2))
+    return ExprError();
----------------
This is unfortunate.  Do we really not have a `checkAtLeastArgCount` type function here?  I know we have something similar for attributes.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:614
+  if (!PtrArgType->isPointerType() ||
+      !PtrArgType->getPointeeType()->isRecordType()) {
+    S.Diag(PtrArgResult.get()->getBeginLoc(),
----------------
Is this sufficient?  Couldn't this be a pointer to a union?  Do you mean the suggestion?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221