[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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