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

[Aaron Ballman via Phabricator via cfe-commits]

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