[PATCH] D16846: PR26449: Fixes for bugs in __builtin_classify_type implementation

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: lib/AST/ExprConstant.cpp:6213
@@ -6211,3 +6212,3 @@
   else if (ArgTy->isEnumeralType())
-    return enumeral_type_class;
+    return LangOpts.CPlusPlus ? enumeral_type_class : integer_type_class;
   else if (ArgTy->isBooleanType())
----------------
I mean, matching GCC exactly on this weird little extension seems reasonable to me, but this is very strange behavior.  Sure, there's an implicit conversion to enum types in C, but there are implicit conversions between a lot of types that are distinguished.

================
Comment at: lib/AST/ExprConstant.cpp:6217
@@ -6215,3 +6216,3 @@
   else if (ArgTy->isCharType())
-    return string_type_class; // gcc doesn't appear to use char_type_class
+    return integer_type_class; // gcc doesn't appear to use char_type_class
   else if (ArgTy->isIntegerType())
----------------
You can just eliminate this case completely; it's caught by the next line.  The comment is good, please add it there.

================
Comment at: lib/AST/ExprConstant.cpp:6223
@@ -6221,3 +6222,3 @@
   else if (ArgTy->isReferenceType())
     return reference_type_class;
   else if (ArgTy->isRealType())
----------------
Note that this case is not actually possible; expressions never have reference type, they're just l-values or x-values.  It's vaguely possible that GCC actually does something like the C++11 decltype feature, which IIRC recognizes direct decl references and reports their declared type instead of the formal type of the reference expression.

================
Comment at: lib/AST/ExprConstant.cpp:6231
@@ -6229,1 +6230,3 @@
+  else if (ArgTy->isMemberFunctionPointerType())
+    return method_type_class;
   else if (ArgTy->isStructureOrClassType())
----------------
I assume member data pointers are supposed to be offset_type_class.

================
Comment at: lib/AST/ExprConstant.cpp:6241
@@ -6238,2 +6240,3 @@
+  else  // FIXME: offset_type_class & lang_type_class?
     llvm_unreachable("CallExpr::isBuiltinClassifyType(): unimplemented type");
 }
----------------
We should be able to turn this into an exhaustive switch over the canonical types.


http://reviews.llvm.org/D16846