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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
