aaron.ballman added a comment. Thanks for working on this, I like the direction it's going!
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8470-8473 +def err_expected_struct_pointer_argument : Error< + "expected pointer to struct as %ordinal0 argument to %1, found %2">; +def err_expected_callable_argument : Error< + "expected a callable expression as %ordinal0 argument to %1, found %2">; ---------------- Should we combine these into: ``` def err_builtin_dump_struct_expects : Error< "expected %select{pointer to struct|a callable expression}0 as %ordinal1 argument to %2, found %3">; ``` to reduce the number of diagnostics we have to tablegen? ================ Comment at: clang/lib/AST/Expr.cpp:2742 + // Otherwise, warn if the result expression would warn. + const Expr *Result = POE->getResultExpr(); + return Result && Result->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx); ---------------- I don't think the changes here are incorrect, but I'm wondering if they should be split into a different patch as they seem to be largely unrelated to the builtin? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:390 + Expr *Lit = S.Context.getPredefinedStringLiteralFromCache(Str); + // Wrap the literal in parentheses to attach a source location. + return new (S.Context) ParenExpr(Loc, Loc, Lit); ---------------- Haha, neat trick. Perhaps someday we should allow `getPredefinedStringLiteralFromCache()` to specify a `SourceLocation`? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:397 + SmallVector<Expr *, 8> Args; + Args.reserve((TheCall->getNumArgs() - 2) + /*Format*/ 1 + Exprs.size()); + Args.assign(TheCall->arg_begin() + 2, TheCall->arg_end()); ---------------- Can you add an assertion that the call has at least two args? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { + if (auto *BT = T->getAs<BuiltinType>()) { ---------------- yihanaa wrote: > I think this is better maintained in "clang/AST/FormatString.h". For example > analyze_printf::PrintfSpecifier can get format specifier, what do you all > think about? +1 to this suggestion -- my hope is that we could generalize it more then as I notice there are missing specifiers for things like intmax_t, size_t, ptrdiff_t, _Decimal types, etc. Plus, that will hopefully make it easier for __builtin_dump_struct to benefit when new format specifiers are added, such as ones for printing a _BitInt. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:500 + // triggering re-evaluation. + auto *RecordArg = makeOpaqueValueExpr(E); + bool RecordArgIsPtr = RecordArg->getType()->isPointerType(); ---------------- Can you spell out the type since it's not in the initialization? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:507 + // Dump each base class, regardless of whether they're aggregates. + if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { + for (const auto &Base : CXXRD->bases()) { ---------------- ================ Comment at: clang/lib/Sema/SemaChecking.cpp:572-574 + // We don't know how to print this field. Print out its address + // with a format specifier that a smart tool will be able to + // recognize and treat specially. ---------------- Should we handle array types specifically, or should we continue to punt on those for now? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:625-626 + QualType FnArgType = TheCall->getArg(1)->getType(); + if (!FnArgType->isFunctionType() && !FnArgType->isFunctionPointerType() && + !(S.getLangOpts().CPlusPlus && FnArgType->isRecordType())) { + auto *BT = FnArgType->getAs<BuiltinType>(); ---------------- ObjC block? 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