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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
