On 12/3/18 5:10 PM, 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.
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.
If we're adding wrappers, any place that wants to look at particular
tree codes will need to deal with unwrapping. The question then is
whether to just remove location wrappers or do fold_for_warn, which will
remove location wrappers and also try to provide a constant value. In
general, the former is better for things that deal with the forms of the
code, and the latter for things that deal with values.
@@ -1236,6 +1236,8 @@ unsafe_conversion_p (location_t loc, tree type, tree
expr, tree result,
loc = expansion_point_location_if_in_system_header (loc);
+ STRIP_ANY_LOCATION_WRAPPER (expr);
+
if (TREE_CODE (expr) == REAL_CST || TREE_CODE (expr) == INTEGER_CST)
Why do the former here, when this function is interested in constants?
warn_array_subscript_with_type_char (location_t loc, tree index)
{
- if (TYPE_MAIN_VARIANT (TREE_TYPE (index)) == char_type_node
- && TREE_CODE (index) != INTEGER_CST)
- warning_at (loc, OPT_Wchar_subscripts,
- "array subscript has type %<char%>");
+ if (TYPE_MAIN_VARIANT (TREE_TYPE (index)) == char_type_node)
+ {
+ STRIP_ANY_LOCATION_WRAPPER (index);
+ if (TREE_CODE (index) != INTEGER_CST)
+ /* If INDEX has a location, use it; otherwise use LOC (the location
+ of the subscripting expression as a whole). */
+ warning_at (EXPR_LOC_OR_LOC (index, loc), OPT_Wchar_subscripts,
+ "array subscript has type %<char%>");
Here you strip a location wrapper and then check whether the expression
has a location, which will give suboptimal results if the expression is
a variable.
@@ -7375,6 +7375,12 @@ fixed_type_or_null (tree instance, int *nonnull, int
*cdtorp)
}
return NULL_TREE;
+ case VIEW_CONVERT_EXPR:
+ if (location_wrapper_p (instance))
+ return RECUR (TREE_OPERAND (instance, 0));
+ else
+ return NULL_TREE;
I think recursion is probably right for some non-location-wrapper uses
of VIEW_CONVERT_EXPR, as well, but for now let's just add a comment to
that effect.
+ STRIP_ANY_LOCATION_WRAPPER (from);
+ if (TREE_CODE (from) == INTEGER_CST
+ && !integer_zerop (from))
+ {
+ if (flags & tf_error)
+ error_at (loc, "reinterpret_cast from integer to pointer");
This might be another place where folding is the right answer.
ignore_overflows (tree expr, tree orig)
{
- if (TREE_CODE (expr) == INTEGER_CST
- && TREE_CODE (orig) == INTEGER_CST
- && TREE_OVERFLOW (expr) != TREE_OVERFLOW (orig))
+ tree stripped_expr = tree_strip_any_location_wrapper (expr);
+ tree stripped_orig = tree_strip_any_location_wrapper (orig);
+
+ if (TREE_CODE (stripped_expr) == INTEGER_CST
+ && TREE_CODE (stripped_orig) == INTEGER_CST
+ && TREE_OVERFLOW (stripped_expr) != TREE_OVERFLOW (stripped_orig))
{
- gcc_assert (!TREE_OVERFLOW (orig));
+ gcc_assert (!TREE_OVERFLOW (stripped_orig));
/* Ensure constant sharing. */
- expr = wide_int_to_tree (TREE_TYPE (expr), wi::to_wide (expr));
+ stripped_expr = wide_int_to_tree (TREE_TYPE (stripped_expr),
+ wi::to_wide (stripped_expr));
}
- return expr;
+
+ if (location_wrapper_p (expr))
+ return maybe_wrap_with_location (stripped_expr, EXPR_LOCATION (expr));
Maybe use preserve_any_location_wrapper here?
@@ -1058,6 +1058,9 @@ grokbitfield (const cp_declarator *declarator,
return NULL_TREE;
}
+ if (width)
+ STRIP_ANY_LOCATION_WRAPPER (width);
Why is this needed? We should already be reducing width to an unwrapped
constant value somewhere along the line.
@@ -656,6 +656,9 @@ add_capture (tree lambda, tree id, tree orig_init, bool
by_reference_p,
listmem = make_pack_expansion (member);
initializer = orig_init;
}
+
+ STRIP_ANY_LOCATION_WRAPPER (initializer);
Why is this needed? What cares about the tree codes of the capture
initializer?
@@ -14122,6 +14126,7 @@ cp_parser_decltype_expr (cp_parser *parser,
+ auto_suppress_location_wrappers sentinel;
Why do we want to suppress them in decltype?
@@ -21832,6 +21847,9 @@ cp_parser_parameter_declaration (cp_parser *parser,
+ if (default_argument)
+ STRIP_ANY_LOCATION_WRAPPER (default_argument);
...
+ /* Since default args are effectively part of the function type,
+ strip location wrappers here, since otherwise the location of
+ one function's default arguments is arbitrarily chosen for
+ all functions with similar signature (due to canonicalization
+ of function types). */
+ STRIP_ANY_LOCATION_WRAPPER (parsed_arg);
Hmm, I imagine this is already an issue with default arguments that are
location-carrying expressions.
It would be nice to avoid this unification, which I suppose would mean
needing to distinguish better between identical and equivalent in
various places.
But for now I guess this is fine.
+ /* Don't create location wrapper nodes within a template-argument-list. */
+ auto_suppress_location_wrappers sentinel;
You probably also want to remove location wrappers in strip_typedefs_expr.
@@ -6257,6 +6257,7 @@ convert_nontype_argument_function (tree type, tree expr,
+ STRIP_ANY_LOCATION_WRAPPER (fn_no_ptr);
Is this necessary with the above suppression?
+ /* Don't create wrapper nodes within an attribute: the
+ handlers don't know how to handle them. */
+ auto_suppress_location_wrappers sentinel;
Do we still need to strip them in handle_namespace_attrs, then?
@@ -1682,6 +1682,8 @@ cxx_sizeof_expr (tree e, tsubst_flags_t complain)
+ STRIP_ANY_LOCATION_WRAPPER (e);
@@ -1754,6 +1756,8 @@ cxx_alignof_expr (tree e, tsubst_flags_t complain)
+ STRIP_ANY_LOCATION_WRAPPER (e);
We should probably grab the location first, and use it in the later
diagnostics.
@@ -3404,11 +3414,13 @@ cp_build_array_ref (location_t loc, tree array, tree
idx,
pointer arithmetic.) */
idx = cp_perform_integral_promotions (idx, complain);
+ tree stripped_idx = tree_strip_any_location_wrapper (idx);
Perhaps this should be maybe_constant_value, to avoid marking the array
as addressable in more cases.
Jason
