Page MenuHomePhabricator

[FileCheck] Better diagnostic for format conflict

Authored by thopre on Apr 8 2020, 9:43 AM.



Improve error message in case of conflict between several implicit
format to mention the operand that conflict.

Diff Detail

Event Timeline

thopre created this revision.Apr 8 2020, 9:43 AM

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


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.


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.


I'm not sure what UseExpr means. Does OuterBinOpExpr make sense?


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?


Is Name now being stored in both ExpressionAST and NumericVariableUse?


update -> updates

thopre updated this revision to Diff 256137.Apr 8 2020, 4:08 PM
thopre marked 7 inline comments as done.

Address all feedback.

jdenny added inline comments.Apr 8 2020, 5:21 PM

I don't see where this was addressed in your update.

thopre marked an inline comment as done.Apr 9 2020, 1:01 AM
thopre added inline comments.

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.

grimar added a comment.Apr 9 2020, 2:31 AM

I am not well familar with the code of FileCheck,
the code looks good to me (I've found no issues with how errors are handled either),
Two observations/suggestions are below.


I'd put it to the end, after other cases. It is more consistent with the resto of the code and also,
default is the most unlikely case here.


Perhaps, I'd inline:

if (!Expr.consumeInteger((AO == AllowedOperand::LegacyLiteral) ? 10 : 0,
  return std::make_unique<ExpressionLiteral>(
      OperandExpr.drop_back(Expr.size()), LiteralValue);
thopre updated this revision to Diff 256239.Apr 9 2020, 4:01 AM
thopre marked 2 inline comments as done.

Address review feedback

jdenny accepted this revision.Apr 9 2020, 5:03 PM
jdenny marked an inline comment as done.

Other than the missing test case, LGTM.


Is there a test for this case?


Ah, got it. Thanks for explaining.

This revision is now accepted and ready to land.Apr 9 2020, 5:03 PM
jdenny added inline comments.Apr 9 2020, 5:09 PM
thopre updated this revision to Diff 257128.Apr 13 2020, 3:28 PM
thopre marked 2 inline comments as done.
  • Remove default case in ExplicitFormat::toString() and move llvm_unreachable outside the switch block
  • Add testcase for getImplicitFormat with errors on both operands
thopre marked an inline comment as done.Apr 13 2020, 4:25 PM
thopre added inline comments.

clang-tidy complains about the first letter of this variable being lowercase but since it's a lambda I think it's justified.

jhenderson added inline comments.Apr 14 2020, 2:17 AM

I believe the general consensus is that lamdbas are variables, not functions (akin to a variable storing a function pointer or a class instance with an operator()), and therefore should be labelled as such with an upper-case first letter. I can dig up conversations on the topic from elsewhere if you want.

thopre updated this revision to Diff 257297.Apr 14 2020, 5:38 AM
thopre marked 2 inline comments as done.

Fix code style for lambda variable.


No need, I'm happy to take your word for it. :-)

This revision was automatically updated to reflect the committed changes.