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

[John McCall via cfe-commits]

rjmccall added a comment.

Thank you, that's a lot better.  Just a few suggestions to promote exhaustive checking.


================
Comment at: lib/AST/ExprConstant.cpp:6246
@@ +6245,3 @@
+
+    default:
+      break;
----------------
Again, try to avoid adding default cases.  There's an x-macro file for BuiltinTypes, too.

================
Comment at: lib/AST/ExprConstant.cpp:6262
@@ +6261,3 @@
+    else if (CanTy->isMemberFunctionPointerType())
+      return method_type_class;
+    break;
----------------
Please turn the else-if into an else with an assert.

================
Comment at: lib/AST/ExprConstant.cpp:6277
@@ +6276,3 @@
+      return union_type_class;
+    break;
+
----------------
You can switch over the tag kind here.

================
Comment at: lib/AST/ExprConstant.cpp:6285
@@ +6284,3 @@
+
+  default:
+    break;
----------------
Instead of having a default case, you should use an x-macro to exhaustively fill in all the cases that you're assuming can't happen.  That way, if somebody adds a new canonical type kind, they'll automatically get a warning here if they don't add it.

In this case, since you're working with a canonical type and it can't be dependent, this would look like:

  #define TYPE(ID, BASE)
  #define NON_CANONICAL_TYPE(ID, BASE) case Type::ID:
  #define NON_CANONICAL_UNLESS_DEPENDENT_TYPE(ID, BASE) case Type::ID:
  #define DEPENDENT_TYPE(ID, BASE) case Type::ID:
  #include "clang/AST/TypeNodes.def"


http://reviews.llvm.org/D16846