* C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
@ 2018-03-14 16:35 Marek Polacek
2018-03-14 18:48 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Marek Polacek @ 2018-03-14 16:35 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
cxx_constant_value doesn't understand template codes, and neither it
understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
instantiate_non_dependent_expr got an OVERLOAD, but since it calls
is_nondependent_constant_expression which checks type_unknown_p, it left the
expression as it was. We can't use is_nondependent_constant_expression in
finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
not suitable here; we'd miss diagnostics. So I did the following; I think we
should reject the testcase with an error.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2018-03-14 Marek Polacek <polacek@redhat.com>
PR c++/84854
* semantics.c (finish_if_stmt_cond): Give error if the condition
is an overloaded function with no contextual type information.
* g++.dg/cpp1z/constexpr-if15.C: New test.
diff --git gcc/cp/semantics.c gcc/cp/semantics.c
index fdf37bea770..a056e9445e9 100644
--- gcc/cp/semantics.c
+++ gcc/cp/semantics.c
@@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
&& require_constant_expression (cond)
&& !value_dependent_expression_p (cond))
{
- cond = instantiate_non_dependent_expr (cond);
- cond = cxx_constant_value (cond, NULL_TREE);
+ if (type_unknown_p (cond))
+ {
+ cxx_incomplete_type_error (cond, TREE_TYPE (cond));
+ cond = error_mark_node;
+ }
+ else
+ {
+ cond = instantiate_non_dependent_expr (cond);
+ cond = cxx_constant_value (cond, NULL_TREE);
+ }
}
finish_cond (&IF_COND (if_stmt), cond);
add_stmt (if_stmt);
diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
index e69de29bb2d..c819b3e3a07 100644
--- gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
+++ gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
@@ -0,0 +1,11 @@
+// PR c++/84854
+// { dg-options -std=c++17 }
+
+constexpr int foo () { return 1; }
+constexpr int foo (int) { return 2; }
+
+template <typename>
+void a()
+{
+ if constexpr(foo) { }; // { dg-error "overloaded function" }
+}
Marek
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-14 16:35 C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854) Marek Polacek
@ 2018-03-14 18:48 ` Jason Merrill
2018-03-15 12:11 ` Marek Polacek
0 siblings, 1 reply; 8+ messages in thread
From: Jason Merrill @ 2018-03-14 18:48 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
> cxx_constant_value doesn't understand template codes, and neither it
> understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
> instantiate_non_dependent_expr got an OVERLOAD, but since it calls
> is_nondependent_constant_expression which checks type_unknown_p, it left the
> expression as it was. We can't use is_nondependent_constant_expression in
> finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
> not suitable here; we'd miss diagnostics. So I did the following; I think we
> should reject the testcase with an error.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-03-14 Marek Polacek <polacek@redhat.com>
>
> PR c++/84854
> * semantics.c (finish_if_stmt_cond): Give error if the condition
> is an overloaded function with no contextual type information.
>
> * g++.dg/cpp1z/constexpr-if15.C: New test.
>
> diff --git gcc/cp/semantics.c gcc/cp/semantics.c
> index fdf37bea770..a056e9445e9 100644
> --- gcc/cp/semantics.c
> +++ gcc/cp/semantics.c
> @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
> && require_constant_expression (cond)
> && !value_dependent_expression_p (cond))
> {
> - cond = instantiate_non_dependent_expr (cond);
> - cond = cxx_constant_value (cond, NULL_TREE);
> + if (type_unknown_p (cond))
> + {
> + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
> + cond = error_mark_node;
I think I'd prefer to skip this block when type_unknown_p, and leave
error handling up to the code shared with regular if.
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-14 18:48 ` Jason Merrill
@ 2018-03-15 12:11 ` Marek Polacek
2018-03-15 14:07 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Marek Polacek @ 2018-03-15 12:11 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
> > cxx_constant_value doesn't understand template codes, and neither it
> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
> > is_nondependent_constant_expression which checks type_unknown_p, it left the
> > expression as it was. We can't use is_nondependent_constant_expression in
> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
> > not suitable here; we'd miss diagnostics. So I did the following; I think we
> > should reject the testcase with an error.
> >
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >
> > 2018-03-14 Marek Polacek <polacek@redhat.com>
> >
> > PR c++/84854
> > * semantics.c (finish_if_stmt_cond): Give error if the condition
> > is an overloaded function with no contextual type information.
> >
> > * g++.dg/cpp1z/constexpr-if15.C: New test.
> >
> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
> > index fdf37bea770..a056e9445e9 100644
> > --- gcc/cp/semantics.c
> > +++ gcc/cp/semantics.c
> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
> > && require_constant_expression (cond)
> > && !value_dependent_expression_p (cond))
> > {
> > - cond = instantiate_non_dependent_expr (cond);
> > - cond = cxx_constant_value (cond, NULL_TREE);
> > + if (type_unknown_p (cond))
> > + {
> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
> > + cond = error_mark_node;
>
> I think I'd prefer to skip this block when type_unknown_p, and leave
> error handling up to the code shared with regular if.
Like this? It was my first version, but I thought we wanted the error;
the following rejects the testcase only when instantiating the template
function. Which is fine by me, too.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2018-03-15 Marek Polacek <polacek@redhat.com>
PR c++/84854
* semantics.c (finish_if_stmt_cond): Check type_unknown_p.
* g++.dg/cpp1z/constexpr-if15.C: New test.
diff --git gcc/cp/semantics.c gcc/cp/semantics.c
index fdf37bea770..753a5f4d4f8 100644
--- gcc/cp/semantics.c
+++ gcc/cp/semantics.c
@@ -733,7 +733,8 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
if (IF_STMT_CONSTEXPR_P (if_stmt)
&& !type_dependent_expression_p (cond)
&& require_constant_expression (cond)
- && !value_dependent_expression_p (cond))
+ && !value_dependent_expression_p (cond)
+ && !type_unknown_p (cond))
{
cond = instantiate_non_dependent_expr (cond);
cond = cxx_constant_value (cond, NULL_TREE);
diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
index e69de29bb2d..c819b3e3a07 100644
--- gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
+++ gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
@@ -0,0 +1,11 @@
+// PR c++/84854
+// { dg-options -std=c++17 }
+
+constexpr int foo () { return 1; }
+constexpr int foo (int) { return 2; }
+
+template <typename>
+void a()
+{
+ if constexpr(foo) { };
+}
Marek
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-15 12:11 ` Marek Polacek
@ 2018-03-15 14:07 ` Jason Merrill
2018-03-21 18:38 ` Marek Polacek
0 siblings, 1 reply; 8+ messages in thread
From: Jason Merrill @ 2018-03-15 14:07 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
>> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
>> > cxx_constant_value doesn't understand template codes, and neither it
>> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
>> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
>> > is_nondependent_constant_expression which checks type_unknown_p, it left the
>> > expression as it was. We can't use is_nondependent_constant_expression in
>> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
>> > not suitable here; we'd miss diagnostics. So I did the following; I think we
>> > should reject the testcase with an error.
>> >
>> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>> >
>> > 2018-03-14 Marek Polacek <polacek@redhat.com>
>> >
>> > PR c++/84854
>> > * semantics.c (finish_if_stmt_cond): Give error if the condition
>> > is an overloaded function with no contextual type information.
>> >
>> > * g++.dg/cpp1z/constexpr-if15.C: New test.
>> >
>> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
>> > index fdf37bea770..a056e9445e9 100644
>> > --- gcc/cp/semantics.c
>> > +++ gcc/cp/semantics.c
>> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
>> > && require_constant_expression (cond)
>> > && !value_dependent_expression_p (cond))
>> > {
>> > - cond = instantiate_non_dependent_expr (cond);
>> > - cond = cxx_constant_value (cond, NULL_TREE);
>> > + if (type_unknown_p (cond))
>> > + {
>> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
>> > + cond = error_mark_node;
>>
>> I think I'd prefer to skip this block when type_unknown_p, and leave
>> error handling up to the code shared with regular if.
>
> Like this?
Yes, thanks.
> It was my first version, but I thought we wanted the error;
Getting an error is an improvement, but it should apply to
non-constexpr if as well, so checking in maybe_convert_cond would be
better. Actually, if we deal with unknown type there, we shouldn't
need this patch at all.
...except I also notice that since maybe_convert_cond doesn't do any
conversion in a template, we're trying to extract the constant value
without first converting to bool, which breaks this testcase:
struct A
{
constexpr operator bool () { return true; }
int i;
};
A a;
template <class T> void f()
{
constexpr bool b = a; // works
if constexpr (a) { } // breaks
}
int main()
{
f<int>();
}
A simple fix would be to replace your type_unknown_p check here with a
comparison to boolean_type_node, so we wait until instantiation time
to require a constant value.
Better would be to actually do the conversion. Perhaps this could
share code (for converting and getting a constant value) with
finish_static_assert.
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-15 14:07 ` Jason Merrill
@ 2018-03-21 18:38 ` Marek Polacek
2018-03-21 19:19 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Marek Polacek @ 2018-03-21 18:38 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote:
> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> > cxx_constant_value doesn't understand template codes, and neither it
> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
> >> > is_nondependent_constant_expression which checks type_unknown_p, it left the
> >> > expression as it was. We can't use is_nondependent_constant_expression in
> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
> >> > not suitable here; we'd miss diagnostics. So I did the following; I think we
> >> > should reject the testcase with an error.
> >> >
> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >> >
> >> > 2018-03-14 Marek Polacek <polacek@redhat.com>
> >> >
> >> > PR c++/84854
> >> > * semantics.c (finish_if_stmt_cond): Give error if the condition
> >> > is an overloaded function with no contextual type information.
> >> >
> >> > * g++.dg/cpp1z/constexpr-if15.C: New test.
> >> >
> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
> >> > index fdf37bea770..a056e9445e9 100644
> >> > --- gcc/cp/semantics.c
> >> > +++ gcc/cp/semantics.c
> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
> >> > && require_constant_expression (cond)
> >> > && !value_dependent_expression_p (cond))
> >> > {
> >> > - cond = instantiate_non_dependent_expr (cond);
> >> > - cond = cxx_constant_value (cond, NULL_TREE);
> >> > + if (type_unknown_p (cond))
> >> > + {
> >> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
> >> > + cond = error_mark_node;
> >>
> >> I think I'd prefer to skip this block when type_unknown_p, and leave
> >> error handling up to the code shared with regular if.
> >
> > Like this?
>
> Yes, thanks.
>
> > It was my first version, but I thought we wanted the error;
>
> Getting an error is an improvement, but it should apply to
> non-constexpr if as well, so checking in maybe_convert_cond would be
> better. Actually, if we deal with unknown type there, we shouldn't
> need this patch at all.
>
> ...except I also notice that since maybe_convert_cond doesn't do any
> conversion in a template, we're trying to extract the constant value
> without first converting to bool, which breaks this testcase:
>
> struct A
> {
> constexpr operator bool () { return true; }
> int i;
> };
>
> A a;
>
> template <class T> void f()
> {
> constexpr bool b = a; // works
> if constexpr (a) { } // breaks
> }
>
> int main()
> {
> f<int>();
> }
>
> A simple fix would be to replace your type_unknown_p check here with a
> comparison to boolean_type_node, so we wait until instantiation time
> to require a constant value.
Ok, that works.
We should also make g++ accept the testcase with "static_assert(a)" instead of
"if constexpr (a) { }" probably. I guess the cxx_constant_value call in
finish_static_assert should happen earlier?
> Better would be to actually do the conversion. Perhaps this could
> share code (for converting and getting a constant value) with
> finish_static_assert.
But this I didn't manage to make to work. If I call perform_implicit_conversion_flags
in maybe_convert_cond, I get
error: cannot resolve overloaded function âfooâ based on conversion to type âboolâ
so I'm not sure how the conversion would help.
Anyway, here's at least the boolean_type_node version.
Bootstrapped/regtested on x86_64-linux.
2018-03-21 Marek Polacek <polacek@redhat.com>
PR c++/84854
* semantics.c (finish_if_stmt_cond): Check if the type of the condition
is boolean.
(finish_static_assert): Remove redundant variable.
* g++.dg/cpp1z/constexpr-if15.C: New test.
* g++.dg/cpp1z/constexpr-if16.C: New test.
diff --git gcc/cp/semantics.c gcc/cp/semantics.c
index fdf37bea770..39455c0168d 100644
--- gcc/cp/semantics.c
+++ gcc/cp/semantics.c
@@ -733,7 +733,10 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
if (IF_STMT_CONSTEXPR_P (if_stmt)
&& !type_dependent_expression_p (cond)
&& require_constant_expression (cond)
- && !value_dependent_expression_p (cond))
+ && !value_dependent_expression_p (cond)
+ /* Wait until instantiation time, since only then COND has been
+ converted to bool. */
+ && TREE_TYPE (cond) == boolean_type_node)
{
cond = instantiate_non_dependent_expr (cond);
cond = cxx_constant_value (cond, NULL_TREE);
@@ -8619,8 +8622,6 @@ void
finish_static_assert (tree condition, tree message, location_t location,
bool member_p)
{
- tsubst_flags_t complain = tf_warning_or_error;
-
if (message == NULL_TREE
|| message == error_mark_node
|| condition == NULL_TREE
@@ -8653,7 +8654,8 @@ finish_static_assert (tree condition, tree message, location_t location,
/* Fold the expression and convert it to a boolean value. */
condition = perform_implicit_conversion_flags (boolean_type_node, condition,
- complain, LOOKUP_NORMAL);
+ tf_warning_or_error,
+ LOOKUP_NORMAL);
condition = fold_non_dependent_expr (condition);
if (TREE_CODE (condition) == INTEGER_CST && !integer_zerop (condition))
diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
index e69de29bb2d..9a9053c3305 100644
--- gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
+++ gcc/testsuite/g++.dg/cpp1z/constexpr-if15.C
@@ -0,0 +1,11 @@
+// PR c++/84854
+// { dg-options -std=c++17 }
+
+constexpr int foo () { return 1; }
+constexpr int foo (int) { return 2; }
+
+template <typename>
+void a()
+{
+ if constexpr(foo) { };
+}
diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-if16.C gcc/testsuite/g++.dg/cpp1z/constexpr-if16.C
index e69de29bb2d..31a149702fd 100644
--- gcc/testsuite/g++.dg/cpp1z/constexpr-if16.C
+++ gcc/testsuite/g++.dg/cpp1z/constexpr-if16.C
@@ -0,0 +1,20 @@
+// { dg-options -std=c++17 }
+
+struct A
+{
+ constexpr operator bool () { return true; }
+ int i;
+};
+
+A a;
+
+template <class T> void f()
+{
+ constexpr bool b = a;
+ if constexpr (a) { }
+}
+
+int main()
+{
+ f<int>();
+}
Marek
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-21 18:38 ` Marek Polacek
@ 2018-03-21 19:19 ` Jason Merrill
2018-03-21 20:26 ` Marek Polacek
0 siblings, 1 reply; 8+ messages in thread
From: Jason Merrill @ 2018-03-21 19:19 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Wed, Mar 21, 2018 at 2:37 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote:
>> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
>> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
>> >> > cxx_constant_value doesn't understand template codes, and neither it
>> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
>> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
>> >> > is_nondependent_constant_expression which checks type_unknown_p, it left the
>> >> > expression as it was. We can't use is_nondependent_constant_expression in
>> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
>> >> > not suitable here; we'd miss diagnostics. So I did the following; I think we
>> >> > should reject the testcase with an error.
>> >> >
>> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>> >> >
>> >> > 2018-03-14 Marek Polacek <polacek@redhat.com>
>> >> >
>> >> > PR c++/84854
>> >> > * semantics.c (finish_if_stmt_cond): Give error if the condition
>> >> > is an overloaded function with no contextual type information.
>> >> >
>> >> > * g++.dg/cpp1z/constexpr-if15.C: New test.
>> >> >
>> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
>> >> > index fdf37bea770..a056e9445e9 100644
>> >> > --- gcc/cp/semantics.c
>> >> > +++ gcc/cp/semantics.c
>> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
>> >> > && require_constant_expression (cond)
>> >> > && !value_dependent_expression_p (cond))
>> >> > {
>> >> > - cond = instantiate_non_dependent_expr (cond);
>> >> > - cond = cxx_constant_value (cond, NULL_TREE);
>> >> > + if (type_unknown_p (cond))
>> >> > + {
>> >> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
>> >> > + cond = error_mark_node;
>> >>
>> >> I think I'd prefer to skip this block when type_unknown_p, and leave
>> >> error handling up to the code shared with regular if.
>> >
>> > Like this?
>>
>> Yes, thanks.
>>
>> > It was my first version, but I thought we wanted the error;
>>
>> Getting an error is an improvement, but it should apply to
>> non-constexpr if as well, so checking in maybe_convert_cond would be
>> better. Actually, if we deal with unknown type there, we shouldn't
>> need this patch at all.
>>
>> ...except I also notice that since maybe_convert_cond doesn't do any
>> conversion in a template, we're trying to extract the constant value
>> without first converting to bool, which breaks this testcase:
>>
>> struct A
>> {
>> constexpr operator bool () { return true; }
>> int i;
>> };
>>
>> A a;
>>
>> template <class T> void f()
>> {
>> constexpr bool b = a; // works
>> if constexpr (a) { } // breaks
>> }
>>
>> int main()
>> {
>> f<int>();
>> }
>>
>> A simple fix would be to replace your type_unknown_p check here with a
>> comparison to boolean_type_node, so we wait until instantiation time
>> to require a constant value.
>
> Ok, that works.
>
> We should also make g++ accept the testcase with "static_assert(a)" instead of
> "if constexpr (a) { }" probably.
> I guess the cxx_constant_value call in
> finish_static_assert should happen earlier?
fold_non_dependent_expr should already have gotten the constant value,
the call to cxx_constant_value is just for giving an error.
The bug seems to be that is_nondependent_constant_expression doesn't
realize that the conversion to bool is OK because it uses a constexpr
member function.
>> Better would be to actually do the conversion. Perhaps this could
>> share code (for converting and getting a constant value) with
>> finish_static_assert.
>
> But this I didn't manage to make to work. If I call perform_implicit_conversion_flags
> in maybe_convert_cond, I get
> error: cannot resolve overloaded function ‘foo’ based on conversion to type ‘bool’
> so I'm not sure how the conversion would help.
That looks like a good diagnostic to me, what's the problem?
> Anyway, here's at least the boolean_type_node version.
>
> Bootstrapped/regtested on x86_64-linux.
>
> 2018-03-21 Marek Polacek <polacek@redhat.com>
>
> PR c++/84854
> * semantics.c (finish_if_stmt_cond): Check if the type of the condition
> is boolean.
OK.
> (finish_static_assert): Remove redundant variable.
But not this hunk; I like to be able to use the name "complain" even
when it isn't a parameter.
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-21 19:19 ` Jason Merrill
@ 2018-03-21 20:26 ` Marek Polacek
2018-04-03 17:33 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Marek Polacek @ 2018-03-21 20:26 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Wed, Mar 21, 2018 at 02:55:36PM -0400, Jason Merrill wrote:
> On Wed, Mar 21, 2018 at 2:37 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote:
> >> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
> >> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> >> > cxx_constant_value doesn't understand template codes, and neither it
> >> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
> >> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
> >> >> > is_nondependent_constant_expression which checks type_unknown_p, it left the
> >> >> > expression as it was. We can't use is_nondependent_constant_expression in
> >> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
> >> >> > not suitable here; we'd miss diagnostics. So I did the following; I think we
> >> >> > should reject the testcase with an error.
> >> >> >
> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >> >> >
> >> >> > 2018-03-14 Marek Polacek <polacek@redhat.com>
> >> >> >
> >> >> > PR c++/84854
> >> >> > * semantics.c (finish_if_stmt_cond): Give error if the condition
> >> >> > is an overloaded function with no contextual type information.
> >> >> >
> >> >> > * g++.dg/cpp1z/constexpr-if15.C: New test.
> >> >> >
> >> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
> >> >> > index fdf37bea770..a056e9445e9 100644
> >> >> > --- gcc/cp/semantics.c
> >> >> > +++ gcc/cp/semantics.c
> >> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
> >> >> > && require_constant_expression (cond)
> >> >> > && !value_dependent_expression_p (cond))
> >> >> > {
> >> >> > - cond = instantiate_non_dependent_expr (cond);
> >> >> > - cond = cxx_constant_value (cond, NULL_TREE);
> >> >> > + if (type_unknown_p (cond))
> >> >> > + {
> >> >> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
> >> >> > + cond = error_mark_node;
> >> >>
> >> >> I think I'd prefer to skip this block when type_unknown_p, and leave
> >> >> error handling up to the code shared with regular if.
> >> >
> >> > Like this?
> >>
> >> Yes, thanks.
> >>
> >> > It was my first version, but I thought we wanted the error;
> >>
> >> Getting an error is an improvement, but it should apply to
> >> non-constexpr if as well, so checking in maybe_convert_cond would be
> >> better. Actually, if we deal with unknown type there, we shouldn't
> >> need this patch at all.
> >>
> >> ...except I also notice that since maybe_convert_cond doesn't do any
> >> conversion in a template, we're trying to extract the constant value
> >> without first converting to bool, which breaks this testcase:
> >>
> >> struct A
> >> {
> >> constexpr operator bool () { return true; }
> >> int i;
> >> };
> >>
> >> A a;
> >>
> >> template <class T> void f()
> >> {
> >> constexpr bool b = a; // works
> >> if constexpr (a) { } // breaks
> >> }
> >>
> >> int main()
> >> {
> >> f<int>();
> >> }
> >>
> >> A simple fix would be to replace your type_unknown_p check here with a
> >> comparison to boolean_type_node, so we wait until instantiation time
> >> to require a constant value.
> >
> > Ok, that works.
> >
> > We should also make g++ accept the testcase with "static_assert(a)" instead of
> > "if constexpr (a) { }" probably.
>
> > I guess the cxx_constant_value call in
> > finish_static_assert should happen earlier?
>
> fold_non_dependent_expr should already have gotten the constant value,
> the call to cxx_constant_value is just for giving an error.
Oop, right.
> The bug seems to be that is_nondependent_constant_expression doesn't
> realize that the conversion to bool is OK because it uses a constexpr
> member function.
OK, I can look into this separately.
> >> Better would be to actually do the conversion. Perhaps this could
> >> share code (for converting and getting a constant value) with
> >> finish_static_assert.
> >
> > But this I didn't manage to make to work. If I call perform_implicit_conversion_flags
> > in maybe_convert_cond, I get
> > error: cannot resolve overloaded function âfooâ based on conversion to type âboolâ
> > so I'm not sure how the conversion would help.
>
> That looks like a good diagnostic to me, what's the problem?
Ugh, I got this wrong. That diagnostic is fine, because we can reject
constexpr-if15.C, but with perform_implicit_conversion_flags we then
can't evaluate constexpr-if16.C:
error: the value of âaâ is not usable in a constant expression
> > Anyway, here's at least the boolean_type_node version.
> >
> > Bootstrapped/regtested on x86_64-linux.
> >
> > 2018-03-21 Marek Polacek <polacek@redhat.com>
> >
> > PR c++/84854
> > * semantics.c (finish_if_stmt_cond): Check if the type of the condition
> > is boolean.
>
> OK.
Thanks, will commit this part along with the testcases. Incrementally we should
then a) add constexpr-if15.C diagnostic, b) accept constexpr-if16.C with
static_assert, too.
> > (finish_static_assert): Remove redundant variable.
>
> But not this hunk; I like to be able to use the name "complain" even
> when it isn't a parameter.
I see, dropped.
Thanks,
Marek
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854)
2018-03-21 20:26 ` Marek Polacek
@ 2018-04-03 17:33 ` Jason Merrill
0 siblings, 0 replies; 8+ messages in thread
From: Jason Merrill @ 2018-04-03 17:33 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 5666 bytes --]
On Wed, Mar 21, 2018 at 4:14 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Mar 21, 2018 at 02:55:36PM -0400, Jason Merrill wrote:
>> On Wed, Mar 21, 2018 at 2:37 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote:
>> >> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <polacek@redhat.com> wrote:
>> >> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
>> >> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <polacek@redhat.com> wrote:
>> >> >> > cxx_constant_value doesn't understand template codes, and neither it
>> >> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash. Here
>> >> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
>> >> >> > is_nondependent_constant_expression which checks type_unknown_p, it left the
>> >> >> > expression as it was. We can't use is_nondependent_constant_expression in
>> >> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression and that is
>> >> >> > not suitable here; we'd miss diagnostics. So I did the following; I think we
>> >> >> > should reject the testcase with an error.
>> >> >> >
>> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>> >> >> >
>> >> >> > 2018-03-14 Marek Polacek <polacek@redhat.com>
>> >> >> >
>> >> >> > PR c++/84854
>> >> >> > * semantics.c (finish_if_stmt_cond): Give error if the condition
>> >> >> > is an overloaded function with no contextual type information.
>> >> >> >
>> >> >> > * g++.dg/cpp1z/constexpr-if15.C: New test.
>> >> >> >
>> >> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
>> >> >> > index fdf37bea770..a056e9445e9 100644
>> >> >> > --- gcc/cp/semantics.c
>> >> >> > +++ gcc/cp/semantics.c
>> >> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
>> >> >> > && require_constant_expression (cond)
>> >> >> > && !value_dependent_expression_p (cond))
>> >> >> > {
>> >> >> > - cond = instantiate_non_dependent_expr (cond);
>> >> >> > - cond = cxx_constant_value (cond, NULL_TREE);
>> >> >> > + if (type_unknown_p (cond))
>> >> >> > + {
>> >> >> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
>> >> >> > + cond = error_mark_node;
>> >> >>
>> >> >> I think I'd prefer to skip this block when type_unknown_p, and leave
>> >> >> error handling up to the code shared with regular if.
>> >> >
>> >> > Like this?
>> >>
>> >> Yes, thanks.
>> >>
>> >> > It was my first version, but I thought we wanted the error;
>> >>
>> >> Getting an error is an improvement, but it should apply to
>> >> non-constexpr if as well, so checking in maybe_convert_cond would be
>> >> better. Actually, if we deal with unknown type there, we shouldn't
>> >> need this patch at all.
>> >>
>> >> ...except I also notice that since maybe_convert_cond doesn't do any
>> >> conversion in a template, we're trying to extract the constant value
>> >> without first converting to bool, which breaks this testcase:
>> >>
>> >> struct A
>> >> {
>> >> constexpr operator bool () { return true; }
>> >> int i;
>> >> };
>> >>
>> >> A a;
>> >>
>> >> template <class T> void f()
>> >> {
>> >> constexpr bool b = a; // works
>> >> if constexpr (a) { } // breaks
>> >> }
>> >>
>> >> int main()
>> >> {
>> >> f<int>();
>> >> }
>> >>
>> >> A simple fix would be to replace your type_unknown_p check here with a
>> >> comparison to boolean_type_node, so we wait until instantiation time
>> >> to require a constant value.
>> >
>> > Ok, that works.
>> >
>> > We should also make g++ accept the testcase with "static_assert(a)" instead of
>> > "if constexpr (a) { }" probably.
>>
>> > I guess the cxx_constant_value call in
>> > finish_static_assert should happen earlier?
>>
>> fold_non_dependent_expr should already have gotten the constant value,
>> the call to cxx_constant_value is just for giving an error.
>
> Oop, right.
>
>> The bug seems to be that is_nondependent_constant_expression doesn't
>> realize that the conversion to bool is OK because it uses a constexpr
>> member function.
>
> OK, I can look into this separately.
>
>> >> Better would be to actually do the conversion. Perhaps this could
>> >> share code (for converting and getting a constant value) with
>> >> finish_static_assert.
>> >
>> > But this I didn't manage to make to work. If I call perform_implicit_conversion_flags
>> > in maybe_convert_cond, I get
>> > error: cannot resolve overloaded function ‘foo’ based on conversion to type ‘bool’
>> > so I'm not sure how the conversion would help.
>>
>> That looks like a good diagnostic to me, what's the problem?
>
> Ugh, I got this wrong. That diagnostic is fine, because we can reject
> constexpr-if15.C, but with perform_implicit_conversion_flags we then
> can't evaluate constexpr-if16.C:
> error: the value of ‘a’ is not usable in a constant expression
>
>> > Anyway, here's at least the boolean_type_node version.
>> >
>> > Bootstrapped/regtested on x86_64-linux.
>> >
>> > 2018-03-21 Marek Polacek <polacek@redhat.com>
>> >
>> > PR c++/84854
>> > * semantics.c (finish_if_stmt_cond): Check if the type of the condition
>> > is boolean.
>>
>> OK.
>
> Thanks, will commit this part along with the testcases. Incrementally we should
> then a) add constexpr-if15.C diagnostic, b) accept constexpr-if16.C with
> static_assert, too.
I think we want to use instantiation_dependent_expression_p here, too.
Applying.
[-- Attachment #2: if-inst-dep.diff --]
[-- Type: text/x-patch, Size: 795 bytes --]
commit 255d33f7e10bd13e99b270e7ac34946e3a03dba1
Author: Jason Merrill <jason@redhat.com>
Date: Mon Apr 2 17:58:41 2018 -0400
* semantics.c (finish_if_stmt_cond): Use instantiation_dependent_expression_p.
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index eef9e2f645d..ef243f6bf0a 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -733,7 +733,7 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
if (IF_STMT_CONSTEXPR_P (if_stmt)
&& !type_dependent_expression_p (cond)
&& require_constant_expression (cond)
- && !value_dependent_expression_p (cond)
+ && !instantiation_dependent_expression_p (cond)
/* Wait until instantiation time, since only then COND has been
converted to bool. */
&& TREE_TYPE (cond) == boolean_type_node)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-03 17:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-14 16:35 C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854) Marek Polacek
2018-03-14 18:48 ` Jason Merrill
2018-03-15 12:11 ` Marek Polacek
2018-03-15 14:07 ` Jason Merrill
2018-03-21 18:38 ` Marek Polacek
2018-03-21 19:19 ` Jason Merrill
2018-03-21 20:26 ` Marek Polacek
2018-04-03 17:33 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).