Re: [Mesa-dev] [PATCH 1/2] glsl: add is_lhs bool on ast_expression
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 @@
Re: [Mesa-dev] [PATCH 1/2] glsl: add is_lhs bool on ast_expression
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] =
Re: [Mesa-dev] [PATCH 1/2] glsl: add is_lhs bool on ast_expression
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.
> };
>
> 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.
> + 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
> +* 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: {
> +
[Mesa-dev] [PATCH 1/2] glsl: add is_lhs bool on ast_expression
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;
};
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
+* array.length. */
+ 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
+* 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, );
@@ -1839,6 +1861,10 @@
