Review feeback from 106420

Change-Id: I9c1ec7f26b0fda25bcedc86fec66d174fe81ed5f
Bug: tint:1633, tint:1451
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/106843
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Auto-Submit: Dan Sinclair <dsinclair@chromium.org>
This commit is contained in:
dan sinclair 2022-10-24 17:49:20 +00:00 committed by Dawn LUCI CQ
parent 29f61747bf
commit 3a1b799585
13 changed files with 33 additions and 27 deletions

View File

@ -22,7 +22,7 @@
namespace tint::ast {
/// A break if statement
/// A break-if statement
class BreakIfStatement final : public Castable<BreakIfStatement, Statement> {
public:
/// Constructor

View File

@ -2456,10 +2456,10 @@ Maybe<const ast::Statement*> ParserImpl::break_if_statement() {
return Failure::kErrored;
}
if (!expr.matched) {
return add_error(t1, "expected expression for `break if`");
return add_error(t1, "expected expression for `break-if`");
}
if (!match(Token::Type::kSemicolon)) {
return add_error(peek(), "expected ';' for `break if` statement");
if (!expect("`break-if` statement", Token::Type::kSemicolon)) {
return Failure::kErrored;
}
return create<ast::BreakIfStatement>(t1.source(), expr.value);

View File

@ -129,7 +129,7 @@ TEST_F(ParserImplTest, LoopStmt_Continuing_BreakIf_MissingExpr) {
EXPECT_TRUE(e.errored);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(e.value, nullptr);
EXPECT_EQ(p->error(), "1:21: expected expression for `break if`");
EXPECT_EQ(p->error(), "1:21: expected expression for `break-if`");
}
TEST_F(ParserImplTest, LoopStmt_Continuing_BreakIf_InvalidExpr) {
@ -139,7 +139,7 @@ TEST_F(ParserImplTest, LoopStmt_Continuing_BreakIf_InvalidExpr) {
EXPECT_TRUE(e.errored);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(e.value, nullptr);
EXPECT_EQ(p->error(), "1:21: expected expression for `break if`");
EXPECT_EQ(p->error(), "1:21: expected expression for `break-if`");
}
TEST_F(ParserImplTest, LoopStmt_NoContinuing_BreakIf) {
@ -159,7 +159,7 @@ TEST_F(ParserImplTest, LoopStmt_Continuing_BreakIf_MissingSemicolon) {
EXPECT_TRUE(e.errored);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(e.value, nullptr);
EXPECT_EQ(p->error(), "1:40: expected ';' for `break if` statement");
EXPECT_EQ(p->error(), "1:40: expected ';' for `break-if` statement");
}
} // namespace

View File

@ -1126,7 +1126,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfTrueInContinuing) {
EXPECT_TRUE(r()->Resolve()) << r()->error();
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.");
"from a continuing block. Use `break-if` instead.");
}
TEST_F(ResolverValidationTest, Stmt_BreakInIfElseInContinuing) {
@ -1140,7 +1140,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseInContinuing) {
EXPECT_TRUE(r()->Resolve()) << r()->error();
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.");
"from a continuing block. Use `break-if` instead.");
}
TEST_F(ResolverValidationTest, Stmt_BreakInContinuing) {
@ -1151,7 +1151,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1170,7 +1170,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfInIfInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1189,7 +1189,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfTrueMultipleStmtsInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1208,7 +1208,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseMultipleStmtsInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1226,7 +1226,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseIfInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1245,7 +1245,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfNonEmptyElseInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1264,7 +1264,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfElseNonEmptyTrueInContinuing) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"
@ -1282,7 +1282,7 @@ TEST_F(ResolverValidationTest, Stmt_BreakInIfInContinuingNotLast) {
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
"12:34 warning: use of deprecated language feature: `break` must not be used to exit "
"from a continuing block. Use break-if instead.\n"
"from a continuing block. Use `break-if` instead.\n"
"12:34 error: break statement in a continuing block must be the single "
"statement of an if statement's true or false block, and that if "
"statement must be the last statement of the continuing block\n"

View File

@ -1465,7 +1465,7 @@ bool Validator::BreakStatement(const sem::Statement* stmt,
if (auto* continuing = ClosestContinuing(/*stop_at_loop*/ true, current_statement)) {
AddWarning(
"use of deprecated language feature: `break` must not be used to exit from "
"a continuing block. Use break-if instead.",
"a continuing block. Use `break-if` instead.",
stmt->Declaration()->source);
auto fail = [&](const char* note_msg, const Source& note_src) {
@ -1651,9 +1651,8 @@ bool Validator::BreakIfStatement(const sem::BreakIfStatement* stmt,
if (s->Is<sem::LoopStatement>()) {
break;
}
if (s->Is<sem::LoopContinuingBlockStatement>()) {
if (s->Declaration()->As<ast::BlockStatement>()->statements.Back() !=
stmt->Declaration()) {
if (auto* continuing = s->As<sem::LoopContinuingBlockStatement>()) {
if (continuing->Declaration()->statements.Back() != stmt->Declaration()) {
AddError("break-if must be last statement in a continuing block",
stmt->Declaration()->source);
AddNote("see continuing block here", s->Declaration()->source);

View File

@ -40,4 +40,8 @@ LoopContinuingBlockStatement::LoopContinuingBlockStatement(const ast::BlockState
}
LoopContinuingBlockStatement::~LoopContinuingBlockStatement() = default;
const ast::BlockStatement* LoopContinuingBlockStatement::Declaration() const {
return static_cast<const ast::BlockStatement*>(Base::Declaration());
}
} // namespace tint::sem

View File

@ -53,6 +53,9 @@ class LoopContinuingBlockStatement final
/// Destructor
~LoopContinuingBlockStatement() override;
/// @returns the AST node
const ast::BlockStatement* Declaration() const;
};
} // namespace tint::sem

View File

@ -1,4 +1,4 @@
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use break-if instead.
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use `break-if` instead.
break;
^^^^^

View File

@ -1,4 +1,4 @@
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use break-if instead.
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use `break-if` instead.
break;
^^^^^

View File

@ -1,4 +1,4 @@
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use break-if instead.
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use `break-if` instead.
break;
^^^^^

View File

@ -1,4 +1,4 @@
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use break-if instead.
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use `break-if` instead.
break;
^^^^^

View File

@ -1,4 +1,4 @@
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use break-if instead.
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use `break-if` instead.
break;
^^^^^

View File

@ -1,4 +1,4 @@
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use break-if instead.
bug/tint/1064.wgsl:12:9 warning: use of deprecated language feature: `break` must not be used to exit from a continuing block. Use `break-if` instead.
break;
^^^^^