[PATCH] D77741: [FileCheck] Better diagnostic for format conflict

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

I like the idea of the change, but there are some parts I don't understand yet.  I've added inline comments.



================
Comment at: llvm/lib/Support/FileCheck.cpp:113-116
+  if (!LeftFormat)
+    return LeftFormat;
+  Expected<ExpressionFormat> RightFormat = RightOperand->getImplicitFormat(SM);
+  if (!RightFormat)
----------------
Is a diagnostic lost when both `!LeftFormat` and `!RightFormat`?

If that's not possible, I think an assert would help.

If it is possible, I think a comment explaining why we don't care would help.


================
Comment at: llvm/lib/Support/FileCheck.cpp:119-121
+  ExpressionFormat Format = *LeftFormat != ExpressionFormat::Kind::NoFormat
+                                ? *LeftFormat
+                                : *RightFormat;
----------------
`Format` is unused if the following `if` condition is true.  I think the logic would be clearer if you move the assigned expression to the final return statement.


================
Comment at: llvm/lib/Support/FileCheck.cpp:406
+    Expr = Expr.rtrim(SpaceChars);
+    StringRef UseExpr = Expr;
     // The first operand in a legacy @LINE expression is always the @LINE
----------------
I'm not sure what UseExpr means.  Does OuterBinOpExpr make sense?


================
Comment at: llvm/lib/Support/FileCheck.cpp:434
     Format = ExplicitFormat;
-  else if (ImplicitFormat == ExpressionFormat::Kind::Conflict)
-    return ErrorDiagnostic::get(
-        SM, UseExpr,
-        "variables with conflicting format specifier: need an explicit one");
-  else if (bool(ImplicitFormat))
-    Format = ImplicitFormat;
-  else
+  else if (ExpressionASTPointer) {
+    Expected<ExpressionFormat> ImplicitFormat =
----------------
Is there a logic change here for the case of !ExpressionASTPointer?

That used to mean ImplicitFormat = NoFormat and then Format = ImplicitFormat.

Now it means Format = Unsigned.

Did I read that correctly?  Why the change?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:241
   NumericVariableUse(StringRef Name, NumericVariable *Variable)
-      : Name(Name), Variable(Variable) {}
+      : ExpressionAST(Name), Name(Name), Variable(Variable) {}
 
----------------
Is Name now being stored in both ExpressionAST and NumericVariableUse?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:678
                       FileCheckPatternContext *Context, const SourceMgr &SM);
-  /// Parses \p Expr for a binary operation at line \p LineNumber, or before
-  /// input is parsed if \p LineNumber is None. The left operand of this binary
-  /// operation is given in \p LeftOp and \p IsLegacyLineExpr indicates whether
-  /// we are parsing a legacy @LINE expression. Parameter \p Context points to
-  /// the class instance holding the live string and numeric variables.
-  /// \returns the class representing the binary operation in the AST of the
-  /// expression, or an error holding a diagnostic against \p SM otherwise.
+  /// Parses and update \p RemainingExpr for a binary operation at line
+  /// \p LineNumber, or before input is parsed if \p LineNumber is None. The
----------------
update -> updates


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77741/new/

https://reviews.llvm.org/D77741