[PATCH] D16930: Improve literal operator parameter diagnostics.

[Richard Smith via cfe-commits]

rsmith added a comment.

Thanks for tackling this!


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6889
@@ +6888,3 @@
+def err_literal_operator_invalid_param : Error<
+  "invalid literal operator parameter type %0">;
+def err_literal_operator_param : Error<
----------------
Maybe "parameter of literal operator must have type 'unsigned long long', 'long double', 'char', 'wchar_t', 'char16_t', or 'char32_t'"?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6895
@@ +6894,3 @@
+def err_literal_operator_defined_type : Error<
+  "cannot define a literal operator on user-defined type %0">;
+def err_literal_operator_template : Error<
----------------
on -> for ?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6897
@@ -6889,1 +6896,3 @@
+def err_literal_operator_template : Error<
+  "invalid template parameters for literal operator">;
 def err_literal_operator_extern_c : Error<
----------------
It'd be nice to say what the valid ones are here. "template parameter list for literal operator must be either 'char...' or 'typename T, T...'" maybe?

================
Comment at: lib/Sema/SemaDeclCXX.cpp:11798-11825
@@ -11798,29 +11797,30 @@
+      // Must have one or two template parameters
       if (Params->size() == 1) {
         NonTypeTemplateParmDecl *PmDecl =
           dyn_cast<NonTypeTemplateParmDecl>(Params->getParam(0));
 
         // The template parameter must be a char parameter pack.
         if (PmDecl && PmDecl->isTemplateParameterPack() &&
             Context.hasSameType(PmDecl->getType(), Context.CharTy))
-          Valid = true;
+          goto FinishedParams;
+
       } else if (Params->size() == 2) {
         TemplateTypeParmDecl *PmType =
           dyn_cast<TemplateTypeParmDecl>(Params->getParam(0));
         NonTypeTemplateParmDecl *PmArgs =
           dyn_cast<NonTypeTemplateParmDecl>(Params->getParam(1));
 
         // The second template parameter must be a parameter pack with the
         // first template parameter as its type.
-        if (PmType && PmArgs &&
-            !PmType->isTemplateParameterPack() &&
-            PmArgs->isTemplateParameterPack()) {
+        if (PmType && PmArgs && !PmType->isTemplateParameterPack() &&
+          PmArgs->isTemplateParameterPack()) {
           const TemplateTypeParmType *TArgs =
             PmArgs->getType()->getAs<TemplateTypeParmType>();
           if (TArgs && TArgs->getDepth() == PmType->getDepth() &&
               TArgs->getIndex() == PmType->getIndex()) {
-            Valid = true;
             if (ActiveTemplateInstantiations.empty())
               Diag(FnDecl->getLocation(),
                    diag::ext_string_literal_operator_template);
+            goto FinishedParams;
           }
         }
----------------
Factor out an `isValidLiteralOperatorTemplateParameterList` function to avoid the somewhat-unclear control flow via `goto` here.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:11827-11831
@@ -11826,3 +11826,7 @@
         }
+      } else {
+        Diag(TpDecl->getLocation(),
+             diag::err_literal_operator_template_with_params);
+        return true;
       }
     }
----------------
Please invert the sense of the `param_size() == 0` test and move this up to the top, so it's right next to the condition that the diagnostic is diagnosing.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:11846-11847
@@ +11845,4 @@
+    // allowed as the only parameters.
+    if (Context.hasSameType(ParamType, Context.UnsignedLongLongTy) ||
+        Context.hasSameType(ParamType, Context.LongDoubleTy) ||
+        Context.hasSameType(ParamType, Context.CharTy) ||
----------------
Replace these two with `ParamType->isSpecificBuiltinType(BuiltinType::ULongLong)` and `ParamType->isSpecificBuiltinType(BuiltinType::LongDouble)`.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:11871-11874
@@ +11870,6 @@
+
+    } else if (!ParamType->isBuiltinType()) {
+      Diag(Param->getSourceRange().getBegin(),
+           diag::err_literal_operator_defined_type)
+          << ParamType << Param->getSourceRange();
+
----------------
I don't think this special case is worthwhile. Merge it with the `_invalid_param` case.


http://reviews.llvm.org/D16930