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

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

jdenny accepted this revision.
jdenny marked an inline comment as done.
jdenny added a comment.
This revision is now accepted and ready to land.

Other than the missing test case, LGTM.



================
Comment at: llvm/lib/Support/FileCheck.cpp:113-116
+  if (!LeftFormat)
+    return LeftFormat;
+  Expected<ExpressionFormat> RightFormat = RightOperand->getImplicitFormat(SM);
+  if (!RightFormat)
----------------
jdenny wrote:
> 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.
Is there a test for this case?


================
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 =
----------------
jdenny wrote:
> thopre wrote:
> > jdenny wrote:
> > > 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?
> > My bad, forgot to submit to send this reply:
> > 
> > NoFormat evaluates to false, hence the final else would have applied instead and set Format to Unsigned. Therefore the behavior is unchanged.
> I don't see where this was addressed in your update.
Ah, got it.  Thanks for explaining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77741