rsmith added inline comments.
================ Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1223 /// The number of special type IDs. const unsigned NumSpecialTypeIDs = 8; ---------------- This presumably needs to be increased. Are we missing test coverage that would have caught this? ================ Comment at: clang/lib/AST/ASTContext.cpp:9386-9389 +static QualType DecodeFunctionTypeFromStr( + const char *&TypeStr, bool IsNested, bool IsNoReturn, bool IsNoThrow, + bool ForceEmptyTy, const ASTContext &Context, + ASTContext::GetBuiltinTypeError &Error, unsigned *IntegerConstantArgs); ---------------- Our convention for such helper functions is to pass the reference to the associated class (`ASTContext` in this case) as the first parameter. (I appreciate that `DecodeTypeFromStr` violates this convention.) ================ Comment at: clang/lib/AST/ASTContext.cpp:9677-9679 + Type = DecodeFunctionTypeFromStr( + Str, /*IsNested=*/true, /*IsNoReturn=*/false, + /*IsNoThrow=*/false, /*ForceEmptyTy=*/false, Context, Error, nullptr); ---------------- OK. We may eventually want a way to specify noreturn and nothrow here, because they're both part of the type in at least some cases, but this seems fine for the immediate future. ================ Comment at: clang/lib/AST/ASTContext.cpp:9739 + if (TypeStr[0] == ')') { + assert(IsNested && "unmatched ')' found at end of type list"); + Error = ASTContext::GE_Missing_type; ---------------- Maybe "missing return type in builtin function type" or similar would be a more useful assert message here? ================ Comment at: clang/lib/AST/ASTContext.cpp:9756 + while (TypeStr[0] && TypeStr[0] != '.' && TypeStr[0] != ')') { + QualType Ty = DecodeTypeFromStr(TypeStr, Context, Error, RequiresICE, true); + if (Error != ASTContext::GE_None) ---------------- Please `assert(!RequiresICE || !IsNested)` here; we shouldn't require parameters of function pointer parameters to be ICEs. ================ Comment at: clang/lib/AST/ASTContext.cpp:9765-9767 // Do array -> pointer decay. The builtin should use the decayed type. if (Ty->isArrayType()) + Ty = Context.getArrayDecayedType(Ty); ---------------- Should we do function -> pointer decay here too, so you can use `(...)` instead of `(...)*` in `Builtins.def`? ================ Comment at: clang/lib/AST/ASTContext.cpp:9775-9776 + + if (ForceEmptyTy) return {}; ---------------- Do we need this special case and its associated flag? Can we make `GetBuiltinType` directly return `QualType();`for `_GetExceptionInfo` instead? ================ Comment at: clang/lib/AST/ASTContext.cpp:9778-9779 assert((TypeStr[0] != '.' || TypeStr[1] == 0) && "'.' should only occur at end of builtin type list!"); ---------------- This will assert on `(.)`. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:1971-1972 return "ucontext.h"; + case ASTContext::GE_Missing_pthread: + return "pthread.h"; } ---------------- Could we use `GE_Missing_type` here instead? The `LIBBUILTIN` already lists "pthread.h" as the corresponding header, so that seems sufficient. I think we only need the special cases above to handle the non-`LIBBUILTIN` cases such as `__builtin_fprintf` that nonetheless require a header inclusion (in that case to provide `FILE`). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58531/new/ https://reviews.llvm.org/D58531 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits