[cfe-commits] [PATCH] Improved diagnostics for literal operators

[Douglas Gregor]

On Jun 17, 2011, at 2:04 PM, David Majnemer wrote:

> Hi,
> 
> This patch implements improved diagnostics for literal operators. I
> may have gone a little overboard with the number of different
> diagnostics, I can trim it down if it is decided to be excessive. It
> also diagnoses when there is an underscore in a string literal with an
> ExtWarn as [usrlit.suffix] says that this is reserved for future
> standardization.
> 
> Determining if a declaration was a literal operator vs a literal
> operator template was modified. I make the distinction based off of
> the existence or lack of a template-parameter-list instead of the
> number of operands.
> 
> I decided to only include the suffix in the messages as all literal
> operator suffixes all have 'operator ""' which would just add noise to
> the output.

Thanks for working on this!

> Lastly, I only emit a single diagnostic when encountering an error and
> bail. I could diagnose more errors in a single go but I don't know how
> much more helpful it is.

It's generally not helpful to diagnose many errors in the same declaration; you did the right thing.

+def ext_literal_operator_does_not_start_with_underscore : ExtWarn<
+  "literal operator '%0' has a suffix identifier that does not start with an "
+  "underscore, suffix identifiers that do not start with an underscore are "
+  "reserved for future standardization">;

That diagnostic is really long. How about just

	"literal operator '%0' suffix does not start with an underscore"

If you really, really like the long diagnostic, at least change the comma to a semicolon.

def err_literal_operator_template_single_template_parameter : Error<
+  "literal operator template '%0' must have a single template parameter in its template parameter list">;
+def err_literal_operator_template_type_char : Error<
+  "literal operator template '%0' must have a single template parameter of type 'char' in its template parameter list; found %1">;

In both cases, "template parameter" should be "template parameter pack", right?

+def err_literal_operator_too_many_parameters : Error<
+  "literal operator '%0' is invalid because it has unexpected parameters">;

The "is invalid because" part isn't needed. The compiler only produces errors when something is wrong anyway. How about something like:

  "literal operator '%0' has extra parameters"

+def err_literal_operator_expected_valid_first_parameter : Error<
+  "literal operator '%0' must have a first parameter of character type, pointer"
+  " to a character type, 'long double' or 'unsigned long long int'">;
+def err_literal_operator_string_literal_second_parameter_size_t: Error<
+  "string literal operator '%0' requires a second parameter of type 'std::size_t'">;

If the user actually wrote a parameter, then we want to display that parameter's type as part of the diagnostic.

Index: include/clang/AST/ASTContext.h
===================================================================
--- include/clang/AST/ASTContext.h	(revision 132903)
+++ include/clang/AST/ASTContext.h	(working copy)
@@ -1182,7 +1182,7 @@
   CanQualType getCanonicalParamType(QualType T) const;
 
   /// \brief Determine whether the given types are equivalent.
-  bool hasSameType(QualType T1, QualType T2) {
+  bool hasSameType(QualType T1, QualType T2) const {
     return getCanonicalType(T1) == getCanonicalType(T2);
   }
 
@@ -1202,7 +1202,7 @@
 
   /// \brief Determine whether the given types are equivalent after
   /// cvr-qualifiers have been removed.
-  bool hasSameUnqualifiedType(QualType T1, QualType T2) {
+  bool hasSameUnqualifiedType(QualType T1, QualType T2) const {
     return getCanonicalType(T1).getTypePtr() ==
            getCanonicalType(T2).getTypePtr();
   }
@@ -1249,7 +1249,7 @@
 
   /// \brief Determines whether two calling conventions name the same
   /// calling convention.
-  bool isSameCallConv(CallingConv lcc, CallingConv rcc) {
+  bool isSameCallConv(CallingConv lcc, CallingConv rcc) const {
     return (getCanonicalCallConv(lcc) == getCanonicalCallConv(rcc));
   }
 
This is fine, but I'd commit it separately. We generally don't care about 'const' on ASTContext, since it's always considered mutable.

+  // C++0x [usrlit.suffix]p1:
+  //   Literal suffix identifiers that do not start with an underscore are
+  //   reserved for future standardization.
+  if (LiteralSuffixIdentifier.front() != '_')
+    Diag(FnDecl->getLocation(),
+         diag::ext_literal_operator_does_not_start_with_underscore)
+      << LiteralSuffixIdentifier;

I'm not sure if you plan to go further with literal operators or not, but at some point we'll want to give a specific diagnostic here when the the literal suffix is something that would conflict with C99 hexfloat literals (since we'll end up ignoring the operator in that case).

+      // C++0x [over.literal]p5:
+      //   [...] that is a non-type template parameter pack [...]
+      if (!PmDecl->isTemplateParameterPack()) {
+        Diag(Params->getRAngleLoc(),
+             diag::err_literal_operator_template_parameter_pack)
+          << LiteralSuffixIdentifier
+          << FixItHint::CreateInsertion(Params->getRAngleLoc(), "...");
+        return true;
+      }

This Fix-It isn't quite right, for a couple of reasons. First, the insertion location isn't correct. If I write:

	template<char Chars> int operator"" _blarg();

the "…" will be inserted after "Chars", which isn't correct. You want to insert it at the place where the name occurs, to make it

	template<char …Chars>

PmDecl->getLocation() should be the right location.

Second, we shouldn't emit the Fix-It unless the know that the it's the only fix needed to make the template parameter correct. That means we should check that it's a non-type template parameter of type 'char' before emitting the fix.

Finally, when we emit a Fix-It, the compiler should recover as if the user had done the right thing. In this case, that would mean changing the NonTypeTemplateParameterDecl to be a parameter pack  (after emitting the error) and then continuing on as if nothing had gone wrong. That way, if the rest of compilation succeeds, we know it's safe to automagically apply the Fix-It.

   } else {
-    // Check the first parameter
-    FunctionDecl::param_iterator Param = FnDecl->param_begin();
+    // We are assuming this is a literal operator instead of a literal operator
+    // template because we see that this is not a template function
 
FWIW, I don't think you need this comment. If it's not a function template, then it's a function, otherwise we wouldn't have gotten here.

Also, since there's no more checking for literal operator templates after the "if" for them, please just "return false;" there and de-nest this "else" block.

+    } else if (PT && FirstParamPointeeType->isAnyCharacterType()) {
+      // C++0x [over.literal]p4:
+      //   A raw literal operator is a literal operator with a single parameter
+      //   whose type is const char*.
+      if (FnDecl->getNumParams() == 1 &&
+          FirstParamPointeeType->isCharType()) {
+        // bail out with success
+        return false;
+      }

Shouldn't this also be checking that FirstParamPointeeType have the 'const' qualifier (and only the 'const' qualifier)?

+      } else if (!Context.hasSameUnqualifiedType(Context.getSizeType(),
+            FnDecl->getParamDecl(1)->getType())) {

Given that the working paper uses the term "parameter-declaration-clause" rather than "parameter-type-list" (as I would have expected), we should be using hasSameType() rather than hasSameUnqualifiedType() here.

Thanks for working on this!

	- Doug