Re: [PING] Re: [PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR c++/43486)

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2018-12-03 at 15:10 -0700, Jeff Law wrote:
> On 11/19/18 9:51 AM, David Malcolm wrote:
> > Ping, for these patches:
> > 
> > [PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR
> > c++/43486)
> >   https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00304.html
> > 
> > [PATCH 2/2] C++: improvements to binary operator diagnostics (PR
> > c++/87504)
> >   https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00303.html
> > 
> > 
> > Thanks
> > Dave
> > 
> > > 
> > > [1] I've split them up for ease of review; they could be reworked
> > > to
> > > be
> > > fully independent, but there's some churn in the results for
> > > -Wtautological-compare introduced by the 1st patch which the 2nd
> > > patch addresses.
> > > 
> > > gcc/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* convert.c: Include "selftest.h".
> > > 	(preserve_any_location_wrapper): New function.
> > > 	(convert_to_pointer_maybe_fold): Update to handle location
> > > 	wrappers.
> > > 	(convert_to_real_maybe_fold): Likewise.
> > > 	(convert_to_integer_1): Handle location wrappers when checking
> > > for
> > > 	INTEGER_CST.
> > > 	(convert_to_integer_maybe_fold): Update to handle location
> > > 	wrappers.
> > > 	(convert_to_complex_maybe_fold): Likewise.
> > > 	(selftest::test_convert_to_integer_maybe_fold): New functions.
> > > 	(selftest::convert_c_tests): New function.
> > > 	* fold-const.c (operand_equal_p): Strip any location wrappers.
> > > 	* selftest-run-tests.c (selftest::run_tests): Call
> > > 	selftest::convert_c_tests.
> > > 	* selftest.h (selftest::convert_c_tests): New decl.
> > > 	* tree.c (tree_int_cst_equal): Strip any location wrappers.
> > > 	(maybe_wrap_with_location): Don't create wrappers if any
> > > 	auto_suppress_location_wrappers are active.
> > > 	(suppress_location_wrappers): New variable.
> > > 	* tree.h (CONSTANT_CLASS_OR_WRAPPER_P): New macro.
> > > 	(suppress_location_wrappers): New decl.
> > > 	(class auto_suppress_location_wrappers): New class.
> > > 
> > > gcc/c-family/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* c-common.c (unsafe_conversion_p): Strip any location wrapper.
> > > 	(verify_tree): Handle location wrappers.
> > > 	(c_common_truthvalue_conversion): Strip any location wrapper.
> > > 	Handle CONST_DECL.
> > > 	(fold_offsetof): Strip any location wrapper.
> > > 	(complete_array_type): Likewise for initial_value.
> > > 	(convert_vector_to_array_for_subscript): Call fold_for_warn on
> > > the
> > > 	index before checking for INTEGER_CST.
> > > 	* c-pretty-print.c (c_pretty_printer::primary_expression):
> > > Don't
> > > 	print parentheses around location wrappers.
> > > 	* c-warn.c (warn_logical_operator): Call fold_for_warn on
> > > op_right
> > > 	before checking for INTEGER_CST.
> > > 	(warn_tautological_bitwise_comparison): Call
> > > 	tree_strip_any_location_wrapper on lhs, rhs, and bitop's
> > > operand
> > > 	before checking for INTEGER_CST.
> > > 	(readonly_error): Strip any location wrapper.
> > > 	(warn_array_subscript_with_type_char): Strip location wrappers
> > > 	before checking for INTEGER_CST.  Use the location of the index
> > > if
> > > 	available.
> > > 
> > > gcc/cp/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* call.c (build_conditional_expr_1): Strip location wrappers
> > > when
> > > 	checking for CONST_DECL.
> > > 	(conversion_null_warnings): Use location of "expr" if
> > > available.
> > > 	* class.c (fixed_type_or_null): Handle location wrappers.
> > > 	* constexpr.c (potential_constant_expression_1): Likewise.
> > > 	* cvt.c (ignore_overflows): Strip location wrappers when
> > > 	checking for INTEGER_CST, and re-wrap the result if present.
> > > 	(ocp_convert): Call fold_for_warn before checking for
> > > INTEGER_CST.
> > > 	* decl.c (reshape_init_r): Strip any location wrapper.
> > > 	(undeduced_auto_decl): Likewise.
> > > 	* decl2.c (grokbitfield): Likewise for width.
> > > 	* expr.c (mark_discarded_use): Likewise for expr.
> > > 	* init.c (build_aggr_init): Likewise before checking init for
> > > 	DECL_P.
> > > 	(warn_placement_new_too_small): Call fold_for_warn on adj
> > > before
> > > 	checking for CONSTANT_CLASS_P, and on nelts.  Strip any
> > > location
> > > 	wrapper from op0 and on oper before checking for VAR_P.
> > > 	* lambda.c (add_capture): Strip any location from initializer.
> > > 	* name-lookup.c (handle_namespace_attrs): Strip any location
> > > from
> > > 	x before checking for STRING_CST.
> > > 	* parser.c (cp_parser_primary_expression): Call
> > > 	maybe_add_location_wrapper on numeric and string literals.
> > > 	(cp_parser_postfix_expression): Strip any location wrapper when
> > > 	checking for DECL_IS_BUILTIN_CONSTANT_P.
> > > 	(cp_parser_binary_expression): Strip any location wrapper when
> > > 	checking for DECL_P on the lhs.
> > > 	(cp_parser_decltype_expr): Suppress location wrappers in the
> > > 	id-expression.
> > > 	(cp_parser_mem_initializer): Add location wrappers to the
> > > 	parenthesized expression list.
> > > 	(cp_parser_template_parameter_list): Don't create wrapper nodes
> > > 	within a template-parameter-list.
> > > 	(cp_parser_template_argument_list): Don't create wrapper nodes
> > > 	within a template-argument-list.
> > > 	(cp_parser_parameter_declaration): Strip location wrappers from
> > > 	default arguments.
> > > 	(cp_parser_gnu_attribute_list): Don't create wrapper nodes
> > > within
> > > 	an attribute.
> > > 	(cp_parser_late_parsing_default_args): Strip location wrappers
> > > 	from default arguments.
> > > 	(cp_parser_omp_all_clauses): Don't create wrapper nodes within
> > > 	OpenMP clauses.
> > > 	(cp_parser_omp_for_loop): Likewise.
> > > 	(cp_parser_omp_declare_reduction_exprs): Likewise.
> > > 	* pt.c (convert_nontype_argument_function): Strip location
> > > 	wrappers from fn_no_ptr before checking for FUNCTION_DECL.
> > > 	(do_auto_deduction): Likewise from init before checking for
> > > 	DECL_P.
> > > 	* semantics.c (force_paren_expr): Likewise from expr before
> > > 	checking for DECL_P.
> > > 	(finish_parenthesized_expr): Likewise from expr before
> > > 	checking for STRING_CST.
> > > 	(perform_koenig_lookup): Likewise from fn.
> > > 	(finish_call_expr): Likewise.
> > > 	(finish_id_expression): Rename to...
> > > 	(finish_id_expression_1): ...this, calling
> > > 	maybe_add_location_wrapper on the result.
> > > 	* tree.c (cp_stabilize_reference): Strip any location wrapper.
> > > 	(builtin_valid_in_constant_expr_p): Likewise.
> > > 	(is_overloaded_fn): Likewise.
> > > 	(maybe_get_fns): Likewise.
> > > 	(selftest::test_lvalue_kind): Verify lvalue_p.
> > > 	* typeck.c (cxx_sizeof_expr): Strip any location wrapper.
> > > 	(cxx_alignof_expr): Likewise.
> > > 	(is_bitfield_expr_with_lowered_type): Handle location wrappers.
> > > 	(cp_build_array_ref): Strip location wrappers from idx before
> > > 	checking for INTEGER_CST.
> > > 	(cp_build_binary_op): Strip location wrapper from first_arg
> > > before
> > > 	checking for PARM_DECL.  Likewise for op1 before checking for
> > > 	INTEGER_CST in two places.  Likewise for orig_op0 and orig_op1
> > > 	when checking for STRING_CST.
> > > 	(cp_build_addr_expr_1): Likewise for arg when checking for
> > > 	FUNCTION_DECL.
> > > 	(cp_build_modify_expr): Likewise for newrhs when checking for
> > > 	STRING_CST.
> > > 	(convert_for_assignment): Don't strip location wrappers when
> > > 	stripping NON_LVALUE_EXPR.
> > > 	(maybe_warn_about_returning_address_of_local): Strip location
> > > 	wrapper from whats_returned before checking for DECL_P.
> > > 	(can_do_nrvo_p): Strip location wrapper from retval.
> > > 	(treat_lvalue_as_rvalue_p): Likewise.
> > > 	(check_return_expr): Likewise.
> > > 	* typeck2.c (cxx_incomplete_type_diagnostic): Strip location
> > > 	wrapper from value before checking for VAR_P or PARM_DECL.
> > > 	(digest_init_r): Strip location wrapper from init.  When
> > > 	copying "init", also copy the wrapped node.
> > > 
> > > gcc/objc/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* objc-act.c (objc_maybe_build_component_ref): Strip any
> > > location
> > > 	wrapper before checking for UOBJC_SUPER_decl and self_decl.
> > > 	(objc_finish_message_expr): Strip any location wrapper.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* c-c++-common/pr51712.c (valid2): Mark xfail as passing on
> > > C++.
> > > 	* g++.dg/cpp1z/decomp48.C: Update expected location of warning
> > > 	for named local variables to use that of the local variable.
> > > 	* g++.dg/init/array43.C: Update expected column to be that of
> > > the
> > > 	initializer.
> > > 	* g++.dg/init/initializer-string-too-long.C: New test.
> > > 	* g++.dg/init/pr43064-1.C: New test.
> > > 	* g++.dg/init/pr43064-2.C: New test.
> > > 	* g++.dg/init/pr43064-3.C: New test.
> > > 	* g++.dg/wrappers/Wparentheses.C: New test.
> 
> Well, you probably know my biggest worry with this -- the unwrappign
> wack-a-mole problem.  It looks like additional memory usage is
> measurable, but tiny.
> 
> Have you received any feedback from Jason on the C++ side?  That's
> the
> bulk of this patch AFAICT.

Not yet.

> I don't see anything objectionable in the c-family/ or generic bits.
> But again I worry more about the need to sprinkle unwrapping all over
> the place.

Thanks for looking at this.

FWIW, the patches have bit-rotted somewhat over the last month; I'm
working on fixing them.

Dave