| Message ID | 20180314155956.GU3522@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH to fix an ICE on invalid with OVERLOADs (PR c++/84854) | expand |
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
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
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
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
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
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
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. 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)
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" } +}