Improve literal operator parameter diagnostics.

[erik.pilkington]

Summary

Previously, invalid parameters in a literal operator function were diagnosed with an uninformative catch all. This commit breaks the catch all into a couple of more informative cases.

[rsmith]

Thanks for tackling this!

include/clang/Basic/DiagnosticSemaKinds.td

6889

Maybe "parameter of literal operator must have type 'unsigned long long', 'long double', 'char', 'wchar_t', 'char16_t', or 'char32_t'"?

6895

on -> for ?

6897

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?

lib/Sema/SemaDeclCXX.cpp

11798–11825

Factor out an isValidLiteralOperatorTemplateParameterList function to avoid the somewhat-unclear control flow via goto here.

11827–11831

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.

11846–11847

Replace these two with ParamType->isSpecificBuiltinType(BuiltinType::ULongLong) and ParamType->isSpecificBuiltinType(BuiltinType::LongDouble).

11871–11874

I don't think this special case is worthwhile. Merge it with the _invalid_param case.

[erik.pilkington]

Thanks! This update fixes everything you brought up.

[rsmith]

Thanks, this looks good. I think we can make the control flow a little more obvious by moving the return true;s right after we emit each diagnostic, and that also lets us remove the goto.

lib/Sema/SemaDeclCXX.cpp

11769

If you're going to issue diagnostics from here, it'd be clearer to name this checkLiteralOperator[...].

11846

Drop this goto, we fall through to the same place.

11886

Move the return true; up into the individual cases that bail out, and remove the goto from the success cases.

[erik.pilkington]

Remove the FinishedParams label, clean up the control flow.

[rsmith]

Looks great, thanks, do you need someone to commit this for you?

[erik.pilkington]

Yes I do, if you don't mind. Thanks for your help!

[rsmith]

Committed as r261034.

include/clang/Basic/DiagnosticSemaKinds.td

 
 // C++ literal operators
 def err_literal_operator_outside_namespace : Error<
   "literal operator %0 must be in a namespace or global scope">;
 def err_literal_operator_id_outside_namespace : Error<
   "non-namespace scope '%0' cannot have a literal operator member">;
 def err_literal_operator_default_argument : Error<
   "literal operator cannot have a default argument">;
-// FIXME: This diagnostic sucks
-def err_literal_operator_params : Error<
-  "parameter declaration for literal operator %0 is not valid">;
+def err_literal_operator_bad_param_count : Error<
+  "non-template literal operator must have one or two parameters">;
+def err_literal_operator_invalid_param : Error<
+  "invalid literal operator parameter type %0">;
+def err_literal_operator_param : Error<
+  "invalid literal operator parameter type %0, did you mean %1?">;
+def err_literal_operator_template_with_params : Error<
+  "literal operator template cannot have any parameters">;
+def err_literal_operator_defined_type : Error<
+  "cannot define a literal operator on user-defined type %0">;
+def err_literal_operator_template : Error<
+  "invalid template parameters for literal operator">;
 def err_literal_operator_extern_c : Error<
   "literal operator must have C++ linkage">;
 def ext_string_literal_operator_template : ExtWarn<
   "string literal operator templates are a GNU extension">,
   InGroup<GNUStringLiteralOperatorTemplate>;
 def warn_user_literal_reserved : Warning<
   "user-defined literal suffixes not starting with '_' are reserved"
   "%select{; no literal will invoke this operator|}0">,

lib/Sema/SemaDeclCXX.cpp

                   diag::err_operator_overload_post_incdec_must_be_int)
         << LastParam->getType() << (Op == OO_MinusMinus);
   }
 
   return false;
 }
 
 /// CheckLiteralOperatorDeclaration - Check whether the declaration
 /// of this literal operator function is well-formed. If so, returns
 /// false; otherwise, emits appropriate diagnostics and returns true.
 bool Sema::CheckLiteralOperatorDeclaration(FunctionDecl *FnDecl) {
   if (isa<CXXMethodDecl>(FnDecl)) {
     Diag(FnDecl->getLocation(), diag::err_literal_operator_outside_namespace)
       << FnDecl->getDeclName();
     return true;
   }
 
   if (FnDecl->isExternC()) {
     Diag(FnDecl->getLocation(), diag::err_literal_operator_extern_c);
     return true;
   }
 
-  bool Valid = false;
-
   // This might be the definition of a literal operator template.
   FunctionTemplateDecl *TpDecl = FnDecl->getDescribedFunctionTemplate();
+
   // This might be a specialization of a literal operator template.
   if (!TpDecl)
     TpDecl = FnDecl->getPrimaryTemplate();
 
   // template <char...> type operator "" name() and
   // template <class T, T...> type operator "" name() are the only valid
   // template signatures, and the only valid signatures with no parameters.
   if (TpDecl) {
     if (FnDecl->param_size() == 0) {
-      // Must have one or two template parameters
       TemplateParameterList *Params = TpDecl->getTemplateParameters();
+
+      // 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;
           }
         }
+      } else {
+        Diag(TpDecl->getLocation(),
+             diag::err_literal_operator_template_with_params);
+        return true;
       }
     }
-  } else if (FnDecl->param_size()) {
-    // Check the first parameter
-    FunctionDecl::param_iterator Param = FnDecl->param_begin();
 
-    QualType T = (*Param)->getType().getUnqualifiedType();
-
-    // unsigned long long int, long double, and any character type are allowed
-    // as the only parameters.
-    if (Context.hasSameType(T, Context.UnsignedLongLongTy) ||
-        Context.hasSameType(T, Context.LongDoubleTy) ||
-        Context.hasSameType(T, Context.CharTy) ||
-        Context.hasSameType(T, Context.WideCharTy) ||
-        Context.hasSameType(T, Context.Char16Ty) ||
-        Context.hasSameType(T, Context.Char32Ty)) {
-      if (++Param == FnDecl->param_end())
-        Valid = true;
+    Diag(TpDecl->getTemplateParameters()->getSourceRange().getBegin(),
+         diag::err_literal_operator_template)
+        << TpDecl->getTemplateParameters()->getSourceRange();
+    return true;
+
+  } else if (FnDecl->param_size() == 1) {
+    const ParmVarDecl *Param = FnDecl->getParamDecl(0);
+
+    QualType ParamType = Param->getType().getUnqualifiedType();
+
+    // Only unsigned long long int, long double, and any character type are
+    // allowed as the only parameters.
+    if (Context.hasSameType(ParamType, Context.UnsignedLongLongTy) ||
+        Context.hasSameType(ParamType, Context.LongDoubleTy) ||
+        Context.hasSameType(ParamType, Context.CharTy) ||
+        Context.hasSameType(ParamType, Context.WideCharTy) ||
+        Context.hasSameType(ParamType, Context.Char16Ty) ||
+        Context.hasSameType(ParamType, Context.Char32Ty)) {
       goto FinishedParams;
+    } else if (const PointerType *Ptr = ParamType->getAs<PointerType>()) {
+      QualType InnerType = Ptr->getPointeeType();
+
+      if (Context.hasSameType(InnerType.getUnqualifiedType(), Context.CharTy) &&
+          InnerType.isConstQualified() && !InnerType.isVolatileQualified())
+        goto FinishedParams;
+
+      Diag(Param->getSourceRange().getBegin(), diag::err_literal_operator_param)
+          << ParamType << "'const char *'" << Param->getSourceRange();
+
+    } else if (ParamType->isRealFloatingType()) {
+      Diag(Param->getSourceRange().getBegin(), diag::err_literal_operator_param)
+          << ParamType << Context.LongDoubleTy << Param->getSourceRange();
+
+    } else if (ParamType->isIntegerType()) {
+      Diag(Param->getSourceRange().getBegin(), diag::err_literal_operator_param)
+          << ParamType << Context.UnsignedLongLongTy << Param->getSourceRange();
+
+    } else if (!ParamType->isBuiltinType()) {
+      Diag(Param->getSourceRange().getBegin(),
+           diag::err_literal_operator_defined_type)
+          << ParamType << Param->getSourceRange();
+
+    } else {
+      Diag(Param->getSourceRange().getBegin(),
+           diag::err_literal_operator_invalid_param)
+          << ParamType << Param->getSourceRange();
     }
 
-    // Otherwise it must be a pointer to const; let's strip those qualifiers.
-    const PointerType *PT = T->getAs<PointerType>();
-    if (!PT)
-      goto FinishedParams;
-    T = PT->getPointeeType();
-    if (!T.isConstQualified() || T.isVolatileQualified())
-      goto FinishedParams;
-    T = T.getUnqualifiedType();
+    return true;
 
-    // Move on to the second parameter;
-    ++Param;
+  } else if (FnDecl->param_size() == 2) {
+    FunctionDecl::param_iterator Param = FnDecl->param_begin();
 
-    // If there is no second parameter, the first must be a const char *
-    if (Param == FnDecl->param_end()) {
-      if (Context.hasSameType(T, Context.CharTy))
-        Valid = true;
-      goto FinishedParams;
+    // First, verify that the first parameter is correct.
+
+    QualType FirstParamType = (*Param)->getType().getUnqualifiedType();
+
+    // Two parameter function must have a pointer to const as a
+    // first parameter; let's strip those qualifiers.
+    const PointerType *PT = FirstParamType->getAs<PointerType>();
+
+    if (!PT) {
+      Diag((*Param)->getSourceRange().getBegin(),
+           diag::err_literal_operator_param)
+          << FirstParamType << "'const char *'" << (*Param)->getSourceRange();
+      return true;
+    }
+
+    QualType PointeeType = PT->getPointeeType();
+    // First parameter must be const
+    if (!PointeeType.isConstQualified() || PointeeType.isVolatileQualified()) {
+      Diag((*Param)->getSourceRange().getBegin(),
+           diag::err_literal_operator_param)
+          << FirstParamType << "'const char *'" << (*Param)->getSourceRange();
+      return true;
     }
 
-    // const char *, const wchar_t*, const char16_t*, and const char32_t*
+    QualType InnerType = PointeeType.getUnqualifiedType();
+    // Only const char *, const wchar_t*, const char16_t*, and const char32_t*
     // are allowed as the first parameter to a two-parameter function
-    if (!(Context.hasSameType(T, Context.CharTy) ||
-          Context.hasSameType(T, Context.WideCharTy) ||
-          Context.hasSameType(T, Context.Char16Ty) ||
-          Context.hasSameType(T, Context.Char32Ty)))
-      goto FinishedParams;
+    if (!(Context.hasSameType(InnerType, Context.CharTy) ||
+          Context.hasSameType(InnerType, Context.WideCharTy) ||
+          Context.hasSameType(InnerType, Context.Char16Ty) ||
+          Context.hasSameType(InnerType, Context.Char32Ty))) {
+      Diag((*Param)->getSourceRange().getBegin(),
+           diag::err_literal_operator_param)
+          << FirstParamType << "'const char *'" << (*Param)->getSourceRange();
+      return true;
+    }
 
-    // The second and final parameter must be an std::size_t
-    T = (*Param)->getType().getUnqualifiedType();
-    if (Context.hasSameType(T, Context.getSizeType()) &&
-        ++Param == FnDecl->param_end())
-      Valid = true;
-  }
+    // Move on to the second and final parameter.
+    ++Param;
 
-  // FIXME: This diagnostic is absolutely terrible.
-FinishedParams:
-  if (!Valid) {
-    Diag(FnDecl->getLocation(), diag::err_literal_operator_params)
-      << FnDecl->getDeclName();
+    // The second parameter must be a std::size_t.
+    QualType SecondParamType = (*Param)->getType().getUnqualifiedType();
+    if (Context.hasSameType(SecondParamType, Context.getSizeType()))
+      goto FinishedParams;
+    else {
+      Diag((*Param)->getSourceRange().getBegin(),
+           diag::err_literal_operator_param)
+          << SecondParamType << Context.getSizeType()
+          << (*Param)->getSourceRange();
+      return true;
+    }
+  } else {
+    Diag(FnDecl->getLocation(), diag::err_literal_operator_bad_param_count);
     return true;
   }
 
+FinishedParams:
   // A parameter-declaration-clause containing a default argument is not
   // equivalent to any of the permitted forms.
   for (auto Param : FnDecl->params()) {
     if (Param->hasDefaultArg()) {
       Diag(Param->getDefaultArgRange().getBegin(),
            diag::err_literal_operator_default_argument)
         << Param->getDefaultArgRange();
       break;

test/CXX/over/over.oper/over.literal/p5.cpp

 // RUN: %clang_cc1 -std=c++11 %s -verify
 
 using size_t = decltype(sizeof(int));
 template<char...> struct S {};
 
 template<char...> void operator "" _a();
 template<char... C> S<C...> operator "" _a();
 
 template<typename T> struct U {
   friend int operator "" _a(const char *, size_t);
   // FIXME: It's not entirely clear whether this is intended to be legal.
   friend U operator "" _a(const T *, size_t); // expected-error {{parameter}}
 };
 template<char...> struct V {
-  friend void operator "" _b(); // expected-error {{parameter}}
+  friend void operator "" _b(); // expected-error {{template}}
 };
 
-template<char... C, int N = 0> void operator "" _b(); // expected-error {{parameter}}
-template<char... C> void operator "" _b(int N = 0); // expected-error {{parameter}}
-template<char, char...> void operator "" _b(); // expected-error {{parameter}}
-template<typename T> T operator "" _b(const char *); // expected-error {{parameter}}
-template<typename T> int operator "" _b(const T *, size_t); // expected-error {{parameter}}
+template<char... C, int N = 0> void operator "" _b(); // expected-error {{template}}
+template<char... C> void operator "" _b(int N = 0); // expected-error {{template}}
+template<char, char...> void operator "" _b(); // expected-error {{template}}
+template<typename T> T operator "" _b(const char *); // expected-error {{template}}
+template<typename T> int operator "" _b(const T *, size_t); // expected-error {{template}}

test/CXX/over/over.oper/over.literal/p8.cpp

 // RUN: %clang_cc1 -std=c++11 %s -verify
 
 struct string;
 namespace std {
   using size_t = decltype(sizeof(int));
 }
 
 void operator "" _km(long double); // ok
 string operator "" _i18n(const char*, std::size_t); // ok
 template<char...> int operator "" \u03C0(); // ok, UCN for lowercase pi // expected-warning {{reserved}}
 float operator ""E(const char *); // expected-error {{invalid suffix on literal}} expected-warning {{reserved}}
 float operator " " B(const char *); // expected-error {{must be '""'}} expected-warning {{reserved}}
 string operator "" 5X(const char *, std::size_t); // expected-error {{expected identifier}}
 double operator "" _miles(double); // expected-error {{parameter}}
-template<char...> int operator "" j(const char*); // expected-error {{parameter}}
+template<char...> int operator "" j(const char*); // expected-error {{template}}
 
 float operator ""_E(const char *);

test/SemaCXX/literal-operators.cpp

 void operator "" _good (const char32_t *, size_t);
 
 // Check typedef and array equivalences
 void operator "" _good (const char[]);
 typedef const char c;
 void operator "" _good (c*);
 
 // Check extra cv-qualifiers
-void operator "" _cv_good (volatile const char *, const size_t); // expected-error {{parameter declaration for literal operator 'operator""_cv_good' is not valid}}
+void operator "" _cv_good (volatile const char *, const size_t); // expected-error {{invalid literal operator parameter type 'const volatile char *', did you mean 'const char *'?}}
 
 // Template declaration
 template <char...> void operator "" _good ();
 
-// FIXME: Test some invalid decls that might crop up.
-template <typename...> void operator "" _invalid(); // expected-error {{parameter declaration for literal operator 'operator""_invalid' is not valid}}
+template <typename...> void operator "" _invalid(); // expected-error {{invalid template parameters for literal operator}}
+template <wchar_t...> void operator "" _invalid(); // expected-error {{invalid template parameters for literal operator}}
+template <unsigned long long...> void operator "" _invalid(); // expected-error {{invalid template parameters for literal operator}}
 
 _Complex float operator""if(long double); // expected-warning {{reserved}}
 _Complex float test_if_1() { return 2.0f + 1.5if; };
 void test_if_2() { "foo"if; } // expected-error {{no matching literal operator for call to 'operator""if'}}