[Mesa-dev] [PATCH 1/2] glsl: add is_lhs bool on ast_expression
Alejandro Piñeiro
apinheiro at igalia.com
Mon Feb 29 06:58:49 UTC 2016
On 26/02/16 22:15, Ian Romanick wrote:
> On 02/26/2016 07:09 AM, Alejandro Piñeiro wrote:
>> Useful to know if a expression is the recipient of an assignment
>> or not, that would be used to (for example) raise warnings of
>> "use of uninitialized variable" without getting a false positive
>> when assigning first a variable.
>>
>> By default the value is false, and it is assigned to true on
>> the following cases:
>> * The lhs assignments subexpression
>> * At ast_array_index, on the array itself.
>> * While handling the method on an array, to avoid the warning
>> calling array.length
>> * When computed the cached test expression at test_to_hir, to
>> avoid a duplicate warning on the test expression of a switch.
>>
>> set_is_lhs setter is added, because in some cases (like ast_field_selection)
>> the value need to be propagated on the expression tree. To avoid doing the
>> propatagion if not needed, it skips if no primary_expression.identifier is
>> available.
>>
>> v2: use a new bool on ast_expression, instead of a new parameter
>> on ast_expression::hir (Timothy Arceri)
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94129
>> ---
>> src/compiler/glsl/ast.h | 5 +++++
>> src/compiler/glsl/ast_function.cpp | 3 +++
>> src/compiler/glsl/ast_to_hir.cpp | 30 ++++++++++++++++++++++++++++++
>> 3 files changed, 38 insertions(+)
>>
>> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
>> index 9aa5bb9..d07b595 100644
>> --- a/src/compiler/glsl/ast.h
>> +++ b/src/compiler/glsl/ast.h
>> @@ -263,6 +263,11 @@ public:
>> * This pointer may be \c NULL.
>> */
>> const char *non_lvalue_description;
>> +
>> + void set_is_lhs(bool new_value);
>> +
>> +private:
>> + bool is_lhs = false;
> This is valid C++? I thought you could only initialize static members
> in this way, and everything else had to be initialized by the constructor.
Yes, it is valid C++, but looking a little, it seems that is valid since
C++11
http://en.cppreference.com/w/cpp/language/data_members
I remember some patches sent to the list related to get some source
files more C++11 friendly, but Im not sure if that applies to
mesa/compiler/glsl. Do you prefer to do the initialization at the
constructor?
>> };
>>
>> class ast_expression_bin : public ast_expression {
>> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
>> index 1a44020..49afccc 100644
>> --- a/src/compiler/glsl/ast_function.cpp
>> +++ b/src/compiler/glsl/ast_function.cpp
>> @@ -1727,6 +1727,9 @@ ast_function_expression::handle_method(exec_list *instructions,
>> const char *method;
>> method = field->primary_expression.identifier;
>>
>> + /* This would prevent to raise "unitialized variable" warnings when calling
> uninitialized
>
>> + * array.length. */
> Here and elsewhere, the closing */ of a multiline comment goes on its
> own line.
Ok.
>> + field->subexpressions[0]->set_is_lhs(true);
>> op = field->subexpressions[0]->hir(instructions, state);
>> if (strcmp(method, "length") == 0) {
>> if (!this->expressions.is_empty()) {
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
>> index db5ec9a..49e4858 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -1248,6 +1248,23 @@ ast_expression::hir_no_rvalue(exec_list *instructions,
>> do_hir(instructions, state, false);
>> }
>>
>> +void
>> +ast_expression::set_is_lhs(bool new_value)
>> +{
>> + /* is_lhs is tracked only to print "variable used unitialized warnings", if
> uninitialized
Ok.
>
>> + * we lack a identifier we can just skip, so also skipping going through
>> + * subexpressions (see below) */
>> + if (this->primary_expression.identifier == NULL)
>> + return;
>> +
>> + this->is_lhs = new_value;
>> +
>> + /* We need to go through the subexpressions tree to cover cases like
>> + * ast_field_selection */
>> + if (this->subexpressions[0] != NULL)
>> + this->subexpressions[0]->set_is_lhs(new_value);
>> +}
>> +
>> ir_rvalue *
>> ast_expression::do_hir(exec_list *instructions,
>> struct _mesa_glsl_parse_state *state,
>> @@ -1323,6 +1340,7 @@ ast_expression::do_hir(exec_list *instructions,
>> break;
>>
>> case ast_assign: {
>> + this->subexpressions[0]->set_is_lhs(true);
>> op[0] = this->subexpressions[0]->hir(instructions, state);
>> op[1] = this->subexpressions[1]->hir(instructions, state);
>>
>> @@ -1592,6 +1610,7 @@ ast_expression::do_hir(exec_list *instructions,
>> case ast_div_assign:
>> case ast_add_assign:
>> case ast_sub_assign: {
>> + this->subexpressions[0]->set_is_lhs(true);
>> op[0] = this->subexpressions[0]->hir(instructions, state);
>> op[1] = this->subexpressions[1]->hir(instructions, state);
>>
>> @@ -1618,6 +1637,7 @@ ast_expression::do_hir(exec_list *instructions,
>> }
>>
>> case ast_mod_assign: {
>> + this->subexpressions[0]->set_is_lhs(true);
>> op[0] = this->subexpressions[0]->hir(instructions, state);
>> op[1] = this->subexpressions[1]->hir(instructions, state);
>>
>> @@ -1640,6 +1660,7 @@ ast_expression::do_hir(exec_list *instructions,
>>
>> case ast_ls_assign:
>> case ast_rs_assign: {
>> + this->subexpressions[0]->set_is_lhs(true);
>> op[0] = this->subexpressions[0]->hir(instructions, state);
>> op[1] = this->subexpressions[1]->hir(instructions, state);
>> type = shift_result_type(op[0]->type, op[1]->type, this->oper, state,
>> @@ -1658,6 +1679,7 @@ ast_expression::do_hir(exec_list *instructions,
>> case ast_and_assign:
>> case ast_xor_assign:
>> case ast_or_assign: {
>> + this->subexpressions[0]->set_is_lhs(true);
>> op[0] = this->subexpressions[0]->hir(instructions, state);
>> op[1] = this->subexpressions[1]->hir(instructions, state);
>> type = bit_logic_result_type(op[0], op[1], this->oper, state, &loc);
>> @@ -1839,6 +1861,10 @@ ast_expression::do_hir(exec_list *instructions,
>> case ast_array_index: {
>> YYLTYPE index_loc = subexpressions[1]->get_location();
>>
>> + /* Getting if an array is being used unintialized is beyond what we get
> uninitialized
Ok.
>
>> + * from ir_value.data.assigned. Setting is_lhs as true would force to
>> + * not raise a unitialized warning when using an array*/
> uninitialized
Ok.
>
>> + subexpressions[0]->set_is_lhs(true);
>> op[0] = subexpressions[0]->hir(instructions, state);
>> op[1] = subexpressions[1]->hir(instructions, state);
>>
>> @@ -5732,6 +5758,10 @@ ast_switch_statement::test_to_hir(exec_list *instructions,
>> {
>> void *ctx = state;
>>
>> + /* set to true to avoid a duplicate "use of unitialized variable" warning
> uninitialized
Ok.
>
>> + * on the switch test case. The first one would be already raised when
>> + * getting the test_expression at ast_switch_statement::hir */
>> + test_expression->set_is_lhs(true);
>> /* Cache value of test expression. */
>> ir_rvalue *const test_val =
>> test_expression->hir(instructions,
>>
>
Thanks for the quick review.
BR
More information about the mesa-dev
mailing list