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
