Improve error message in case of conflict between several implicit
format to mention the operand that conflict.
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
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 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,
Perhaps, I'd inline:
if (!Expr.consumeInteger((AO == AllowedOperand::LegacyLiteral) ? 10 : 0, LiteralValue)) return std::make_unique<ExpressionLiteral>( OperandExpr.drop_back(Expr.size()), LiteralValue);
One more thing: clang-tidy is saying you're violating this:
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.