Page MenuHomePhabricator

[FileCheck] Better diagnostic for format conflict
ClosedPublic

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

Details

Summary

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.

llvm/lib/Support/FileCheck.cpp
112–115

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.

118–120

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.

407

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

435–442

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?

llvm/lib/Support/FileCheckImpl.h
238–239

Is Name now being stored in both ExpressionAST and NumericVariableUse?

675–683

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
llvm/lib/Support/FileCheck.cpp
435–442

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.
llvm/lib/Support/FileCheck.cpp
435–442

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.

llvm/lib/Support/FileCheck.cpp
30

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.

297

Perhaps, I'd inline:

if (!Expr.consumeInteger((AO == AllowedOperand::LegacyLiteral) ? 10 : 0,
                         LiteralValue))
  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.

llvm/lib/Support/FileCheck.cpp
112–115

Is there a test for this case?

435–442

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
llvm/lib/Support/FileCheck.cpp
39
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.
llvm/unittests/Support/FileCheckTest.cpp
45

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
llvm/unittests/Support/FileCheckTest.cpp
45

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.

llvm/unittests/Support/FileCheckTest.cpp
45

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

This revision was automatically updated to reflect the committed changes.