[Mesa-dev] [PATCH 1/2] glsl: add is_lhs bool on ast_expression
Alejandro Piñeiro
apinheiro at igalia.com
Thu Mar 3 09:18:20 UTC 2016
All the points made were somewhat small (afaiu). So just in case my
answer were missed. Ping.
On 29/02/16 07:58, Alejandro Piñeiro wrote:
>
> 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