Improve DSL error reporting with invalid array sizes
This eliminates an extra "expected int literal" error when the
array size is an invalid expression.
Change-Id: Iaf5d15316df3ec5200d51d73c14d7e428ce17be9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/443236
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
diff --git a/include/sksl/DSLExpression.h b/include/sksl/DSLExpression.h
index d7ed8ea..6f7142e 100644
--- a/include/sksl/DSLExpression.h
+++ b/include/sksl/DSLExpression.h
@@ -124,13 +124,18 @@
/**
* Returns true if this object contains an expression. DSLExpressions which were created with
- * the empty constructor or which have already been release()ed are not valid. DSLExpressions
- * created with errors are still considered valid (but contain a poison value).
+ * the empty constructor or which have already been release()ed do not have a value.
+ * DSLExpressions created with errors are still considered to have a value (but contain poison).
*/
- bool valid() const {
+ bool hasValue() const {
return fExpression != nullptr;
}
+ /**
+ * Returns true if this object contains an expression which is not poison.
+ */
+ bool isValid() const;
+
void swap(DSLExpression& other);
/**
@@ -141,9 +146,9 @@
private:
/**
- * Calls release if this expression is valid, otherwise returns null.
+ * Calls release if this expression has a value, otherwise returns null.
*/
- std::unique_ptr<SkSL::Expression> releaseIfValid();
+ std::unique_ptr<SkSL::Expression> releaseIfPossible();
/**
* Invalidates this object and returns the SkSL expression it represents coerced to the
diff --git a/include/sksl/DSLStatement.h b/include/sksl/DSLStatement.h
index 4f4f80d..3abc92d 100644
--- a/include/sksl/DSLStatement.h
+++ b/include/sksl/DSLStatement.h
@@ -48,10 +48,10 @@
DSLStatement& operator=(DSLStatement&& other) = default;
- bool valid() { return fStatement != nullptr; }
+ bool hasValue() { return fStatement != nullptr; }
std::unique_ptr<SkSL::Statement> release() {
- SkASSERT(this->valid());
+ SkASSERT(this->hasValue());
return std::move(fStatement);
}
@@ -60,7 +60,7 @@
DSLStatement(std::unique_ptr<SkSL::Expression> expr);
- std::unique_ptr<SkSL::Statement> releaseIfValid() {
+ std::unique_ptr<SkSL::Statement> releaseIfPossible() {
return std::move(fStatement);
}
@@ -90,7 +90,7 @@
~DSLPossibleStatement();
- bool valid() { return fStatement != nullptr; }
+ bool hasValue() { return fStatement != nullptr; }
std::unique_ptr<SkSL::Statement> release() {
return DSLStatement(std::move(*this)).release();
diff --git a/src/sksl/SkSLDSLParser.cpp b/src/sksl/SkSLDSLParser.cpp
index 3115d70..3546b8b 100644
--- a/src/sksl/SkSLDSLParser.cpp
+++ b/src/sksl/SkSLDSLParser.cpp
@@ -336,6 +336,10 @@
return Declare(vars);
}
+static bool is_valid(const skstd::optional<DSLWrapper<DSLExpression>>& expr) {
+ return expr && expr->get().isValid();
+}
+
SKSL_INT DSLParser::arraySize() {
Token next = this->peek();
if (next.fKind == Token::Kind::TK_INT_LITERAL) {
@@ -357,8 +361,10 @@
this->error(next, "array size must be positive");
return 1;
} else {
- this->expression();
- this->error(next, "expected int literal");
+ skstd::optional<DSLWrapper<DSLExpression>> expr = this->expression();
+ if (is_valid(expr)) {
+ this->error(next, "expected int literal");
+ }
return 1;
}
}
diff --git a/src/sksl/dsl/DSLCore.cpp b/src/sksl/dsl/DSLCore.cpp
index 0d24a51..f32a58c 100644
--- a/src/sksl/dsl/DSLCore.cpp
+++ b/src/sksl/dsl/DSLCore.cpp
@@ -185,15 +185,15 @@
static DSLPossibleStatement For(DSLStatement initializer, DSLExpression test,
DSLExpression next, DSLStatement stmt, PositionInfo pos) {
return ForStatement::Convert(DSLWriter::Context(), /*offset=*/-1,
- initializer.releaseIfValid(), test.releaseIfValid(),
- next.releaseIfValid(), stmt.release(),
+ initializer.releaseIfPossible(), test.releaseIfPossible(),
+ next.releaseIfPossible(), stmt.release(),
DSLWriter::SymbolTable());
}
static DSLPossibleStatement If(DSLExpression test, DSLStatement ifTrue, DSLStatement ifFalse,
bool isStatic) {
return IfStatement::Convert(DSLWriter::Context(), /*offset=*/-1, isStatic, test.release(),
- ifTrue.release(), ifFalse.releaseIfValid());
+ ifTrue.release(), ifFalse.releaseIfPossible());
}
static DSLGlobalVar InterfaceBlock(const DSLModifiers& modifiers, skstd::string_view typeName,
@@ -248,7 +248,7 @@
// this point we do not know the function's return type. We therefore do not check for
// errors, or coerce the value to the correct type, until the return statement is actually
// added to a function. (This is done in FunctionDefinition::Convert.)
- return SkSL::ReturnStatement::Make(/*offset=*/-1, value.releaseIfValid());
+ return SkSL::ReturnStatement::Make(/*offset=*/-1, value.releaseIfPossible());
}
static DSLExpression Swizzle(DSLExpression base, SkSL::SwizzleComponent::Type a,
@@ -301,7 +301,7 @@
SkTArray<StatementArray> statements;
statements.reserve_back(cases.count());
for (DSLCase& c : cases) {
- values.push_back(c.fValue.releaseIfValid());
+ values.push_back(c.fValue.releaseIfPossible());
statements.push_back(std::move(c.fStatements));
}
return DSLWriter::ConvertSwitch(value.release(), std::move(values), std::move(statements),
diff --git a/src/sksl/dsl/DSLExpression.cpp b/src/sksl/dsl/DSLExpression.cpp
index 134318b..6ebd00e 100644
--- a/src/sksl/dsl/DSLExpression.cpp
+++ b/src/sksl/dsl/DSLExpression.cpp
@@ -35,7 +35,7 @@
DSLExpression::DSLExpression(std::unique_ptr<SkSL::Expression> expression)
: fExpression(std::move(expression)) {
- SkASSERT(this->valid());
+ SkASSERT(this->hasValue());
DSLWriter::ReportErrors();
}
@@ -106,21 +106,25 @@
"ProgramSettings::fAssertDSLObjectsReleased)");
}
+bool DSLExpression::isValid() const {
+ return this->hasValue() && !fExpression->is<SkSL::Poison>();
+}
+
void DSLExpression::swap(DSLExpression& other) {
std::swap(fExpression, other.fExpression);
}
std::unique_ptr<SkSL::Expression> DSLExpression::release() {
- SkASSERT(this->valid());
+ SkASSERT(this->hasValue());
return std::move(fExpression);
}
-std::unique_ptr<SkSL::Expression> DSLExpression::releaseIfValid() {
+std::unique_ptr<SkSL::Expression> DSLExpression::releaseIfPossible() {
return std::move(fExpression);
}
DSLType DSLExpression::type() {
- if (!this->valid()) {
+ if (!this->hasValue()) {
return kVoid_Type;
}
return &fExpression->type();
diff --git a/src/sksl/dsl/DSLFunction.cpp b/src/sksl/dsl/DSLFunction.cpp
index 12484f2..8eff3e0 100644
--- a/src/sksl/dsl/DSLFunction.cpp
+++ b/src/sksl/dsl/DSLFunction.cpp
@@ -39,7 +39,7 @@
if (param->fDeclared) {
DSLWriter::ReportError("parameter has already been used in another function");
}
- SkASSERT(!param->fInitialValue.valid());
+ SkASSERT(!param->fInitialValue.hasValue());
SkASSERT(!param->fDeclaration);
param->fDeclared = true;
std::unique_ptr<SkSL::Variable> paramVar = DSLWriter::CreateParameterVar(*param);
diff --git a/src/sksl/dsl/DSLStatement.cpp b/src/sksl/dsl/DSLStatement.cpp
index 35b488f..0ce2fd1 100644
--- a/src/sksl/dsl/DSLStatement.cpp
+++ b/src/sksl/dsl/DSLStatement.cpp
@@ -37,12 +37,12 @@
DSLStatement::DSLStatement(std::unique_ptr<SkSL::Expression> expr)
: fStatement(SkSL::ExpressionStatement::Make(DSLWriter::Context(), std::move(expr))) {
- SkASSERT(this->valid());
+ SkASSERT(this->hasValue());
}
DSLStatement::DSLStatement(std::unique_ptr<SkSL::Statement> stmt)
: fStatement(std::move(stmt)) {
- SkASSERT(this->valid());
+ SkASSERT(this->hasValue());
}
DSLStatement::DSLStatement(DSLPossibleExpression expr, PositionInfo pos)
@@ -50,7 +50,7 @@
DSLStatement::DSLStatement(DSLPossibleStatement stmt, PositionInfo pos) {
DSLWriter::ReportErrors(pos);
- if (stmt.valid()) {
+ if (stmt.hasValue()) {
fStatement = std::move(stmt.fStatement);
} else {
fStatement = SkSL::Nop::Make();
diff --git a/src/sksl/dsl/priv/DSLFPs.cpp b/src/sksl/dsl/priv/DSLFPs.cpp
index c5c5540..eb23e94 100644
--- a/src/sksl/dsl/priv/DSLFPs.cpp
+++ b/src/sksl/dsl/priv/DSLFPs.cpp
@@ -30,7 +30,7 @@
}
DSLExpression SampleChild(int index, DSLExpression sampleExpr) {
- std::unique_ptr<SkSL::Expression> expr = sampleExpr.releaseIfValid();
+ std::unique_ptr<SkSL::Expression> expr = sampleExpr.releaseIfPossible();
if (expr) {
SkASSERT(expr->type().isVector());
SkASSERT(expr->type().componentType().isFloat());
diff --git a/src/sksl/dsl/priv/DSLWriter.cpp b/src/sksl/dsl/priv/DSLWriter.cpp
index ecd4f4b..48c2f82 100644
--- a/src/sksl/dsl/priv/DSLWriter.cpp
+++ b/src/sksl/dsl/priv/DSLWriter.cpp
@@ -160,7 +160,7 @@
args.reserve_back(rawArgs.size());
for (DSLExpression& arg : rawArgs) {
- if (!arg.valid()) {
+ if (!arg.hasValue()) {
return DSLPossibleExpression(nullptr);
}
args.push_back(arg.release());
@@ -268,7 +268,7 @@
// of DSLParser we don't even need DSL variables to show up in the symbol table in the
// first place.
var.fDeclaration = DSLWriter::IRGenerator().convertVarDeclaration(
- std::move(skslvar), var.fInitialValue.releaseIfValid(),
+ std::move(skslvar), var.fInitialValue.releaseIfPossible(),
/*addToSymbolTable=*/false);
if (var.fDeclaration) {
var.fVar = varPtr;
@@ -294,7 +294,7 @@
if (!var.fDeclaration) {
// We should have already reported an error before ending up here, just clean up the
// initial value so it doesn't assert and return a nop.
- var.fInitialValue.releaseIfValid();
+ var.fInitialValue.releaseIfPossible();
return SkSL::Nop::Make();
}
return std::move(var.fDeclaration);