Re: [PATCH] c++, v2: Implement C++26 P2741R3 - user-generated static_assert messages [PR110348]
On Fri, Nov 17, 2023 at 09:18:39AM -0500, Jason Merrill wrote: > You recently pinged this patch, but I haven't seen an update since this > review? Oops, sorry, I've missed this and DR 2406 review posts in my inbox during vacation, will get to that momentarily. Thanks. Jakub
Re: [PATCH] c++, v2: Implement C++26 P2741R3 - user-generated static_assert messages [PR110348]
You recently pinged this patch, but I haven't seen an update since this
review?
On 10/26/23 21:21, Jason Merrill wrote:
On 9/18/23 13:21, Jakub Jelinek wrote:
Here is an updated version of the patch.
Compared to the last version, based on the discussion in the PR, the
patch
1) warns (but only that) if size()/data() methods aren't declared
constexpr/consteval (or implicitly constexpr)
The language requirements also seem to be satisfied by
constexpr const char msg[] = "foo";
struct A { constexpr int operator () () { return sizeof(msg); } };
struct B { constexpr const char * operator()() { return msg; } };
struct C {
A size;
B data;
};
constexpr int i = C().size();
constexpr const char *p = C().data();
static_assert (false, C());
constexpr int size() { return sizeof(msg); }
constexpr const char *data() { return msg; }
struct D {
int (*size)() = ::size;
const char *(*data)() = ::data;
};
constexpr int di = D().size();
constexpr const char *dp = D().data();
static_assert (false, D());
so we shouldn't assume that size/data are methods.
2) as I don't see a function which would determine if some expression
is core constant expression (for the data() case), the patch just
as an
optimization tries to fold_nondependent_expr msg.data() expression
quietly, if it is a constant expression, passes it to c_getstr if
len > 0
and if successful, only tries to constant expression evaluate
msg.data()[0] and msg.data()[len - 1], otherwise it will constant
expression evaluate the characters one by one;
for the len == 0 case, it will fold_nondependent_expr + check
result is
integer_zero_node for (msg.data(), 0) which I think should fail if
msg.data() is not a core constant expression, but succeed if it is
even if it is not constant expression
Sounds good.
3) already the earlier version of the patch was passing
manifestly_const_eval=true argument, you said in the PR you've raised
it in CWG, I've newly added testsuite coverage for that
CWG agreed with this direction.
--- gcc/cp/semantics.cc.jj 2023-09-05 17:26:51.849921954 +0200
+++ gcc/cp/semantics.cc 2023-09-18 14:31:55.269431759 +0200
@@ -11388,11 +11389,77 @@ finish_static_assert (tree condition, tr
if (check_for_bare_parameter_packs (condition))
condition = error_mark_node;
+ if (check_for_bare_parameter_packs (message))
+ return;
+
+ if (TREE_CODE (message) != STRING_CST
+ && !type_dependent_expression_p (message))
+ {
+ message_sz
+ = finish_class_member_access_expr (message,
+ get_identifier ("size"),
+ false, tf_none);
+ if (TREE_CODE (message_sz) != COMPONENT_REF)
+ message_sz = error_mark_node;
+ if (message_sz != error_mark_node)
+ message_sz = build_new_method_call (message,
+ TREE_OPERAND (message_sz, 1),
+ NULL, NULL_TREE, LOOKUP_NORMAL,
+ NULL, tf_none);
This should probably use finish_call_expr instead of
build_new_method_call because of my example above, and also so you don't
need to pull out the TREE_OPERAND or check the result of name lookup.
+ message_data
+ = finish_class_member_access_expr (message,
+ get_identifier ("data"),
+ false, tf_none);
+ if (TREE_CODE (message_data) != COMPONENT_REF)
+ message_data = error_mark_node;
+ if (message_data != error_mark_node)
+ message_data = build_new_method_call (message,
+ TREE_OPERAND (message_data, 1),
+ NULL, NULL_TREE, LOOKUP_NORMAL,
+ NULL, tf_none);
Likewise.
+ if (message_sz == error_mark_node
+ || message_data == error_mark_node)
+ {
+ error_at (location, "% message must be a string "
+ "literal or object with % and "
+ "% members");
This diagnostic should be just if the calls to
finish_class_member_access_expr fail; better to get the normal
diagnostics from finish_call_expr if the calls fail for whatever reason.
+ return;
+ }
+ if (tree s
+ = cp_get_callee_fndecl_nofold (extract_call_expr (message_sz)))
+ if (!DECL_DECLARED_CONSTEXPR_P (s))
+ warning_at (location, 0, "% message %qs "
+ "member not %", "size()");
+ message_sz = perform_implicit_conversion (size_type_node,
message_sz,
+ tf_none);
This should probably use build_converted_constant_expr?
+ if (message_sz == error_mark_node)
+ {
+ error_at (location, "% message % member "
+ "function must be implicitly convertible to "
+ "%");
+ return;
+ }
+ if (tree d
+ = cp_get_callee_fndecl_nofold (extract_call_expr (message_data)))
+ if (!DECL_DECLARED_CONSTEXPR_P (d))
+ warning_at (location, 0, "% message %qs "
+ "member not %",
Re: [PATCH] c++, v2: Implement C++26 P2741R3 - user-generated static_assert messages [PR110348]
On 9/18/23 13:21, Jakub Jelinek wrote:
Here is an updated version of the patch.
Compared to the last version, based on the discussion in the PR, the patch
1) warns (but only that) if size()/data() methods aren't declared
constexpr/consteval (or implicitly constexpr)
The language requirements also seem to be satisfied by
constexpr const char msg[] = "foo";
struct A { constexpr int operator () () { return sizeof(msg); } };
struct B { constexpr const char * operator()() { return msg; } };
struct C {
A size;
B data;
};
constexpr int i = C().size();
constexpr const char *p = C().data();
static_assert (false, C());
constexpr int size() { return sizeof(msg); }
constexpr const char *data() { return msg; }
struct D {
int (*size)() = ::size;
const char *(*data)() = ::data;
};
constexpr int di = D().size();
constexpr const char *dp = D().data();
static_assert (false, D());
so we shouldn't assume that size/data are methods.
2) as I don't see a function which would determine if some expression
is core constant expression (for the data() case), the patch just as an
optimization tries to fold_nondependent_expr msg.data() expression
quietly, if it is a constant expression, passes it to c_getstr if len > 0
and if successful, only tries to constant expression evaluate
msg.data()[0] and msg.data()[len - 1], otherwise it will constant
expression evaluate the characters one by one;
for the len == 0 case, it will fold_nondependent_expr + check result is
integer_zero_node for (msg.data(), 0) which I think should fail if
msg.data() is not a core constant expression, but succeed if it is
even if it is not constant expression
Sounds good.
3) already the earlier version of the patch was passing
manifestly_const_eval=true argument, you said in the PR you've raised
it in CWG, I've newly added testsuite coverage for that
CWG agreed with this direction.
--- gcc/cp/semantics.cc.jj 2023-09-05 17:26:51.849921954 +0200
+++ gcc/cp/semantics.cc 2023-09-18 14:31:55.269431759 +0200
@@ -11388,11 +11389,77 @@ finish_static_assert (tree condition, tr
if (check_for_bare_parameter_packs (condition))
condition = error_mark_node;
+ if (check_for_bare_parameter_packs (message))
+return;
+
+ if (TREE_CODE (message) != STRING_CST
+ && !type_dependent_expression_p (message))
+{
+ message_sz
+ = finish_class_member_access_expr (message,
+ get_identifier ("size"),
+ false, tf_none);
+ if (TREE_CODE (message_sz) != COMPONENT_REF)
+ message_sz = error_mark_node;
+ if (message_sz != error_mark_node)
+ message_sz = build_new_method_call (message,
+ TREE_OPERAND (message_sz, 1),
+ NULL, NULL_TREE, LOOKUP_NORMAL,
+ NULL, tf_none);
This should probably use finish_call_expr instead of
build_new_method_call because of my example above, and also so you don't
need to pull out the TREE_OPERAND or check the result of name lookup.
+ message_data
+ = finish_class_member_access_expr (message,
+ get_identifier ("data"),
+ false, tf_none);
+ if (TREE_CODE (message_data) != COMPONENT_REF)
+ message_data = error_mark_node;
+ if (message_data != error_mark_node)
+ message_data = build_new_method_call (message,
+ TREE_OPERAND (message_data, 1),
+ NULL, NULL_TREE, LOOKUP_NORMAL,
+ NULL, tf_none);
Likewise.
+ if (message_sz == error_mark_node
+ || message_data == error_mark_node)
+ {
+ error_at (location, "% message must be a string "
+ "literal or object with % and "
+ "% members");
This diagnostic should be just if the calls to
finish_class_member_access_expr fail; better to get the normal
diagnostics from finish_call_expr if the calls fail for whatever reason.
+ return;
+ }
+ if (tree s
+ = cp_get_callee_fndecl_nofold (extract_call_expr (message_sz)))
+ if (!DECL_DECLARED_CONSTEXPR_P (s))
+ warning_at (location, 0, "% message %qs "
+ "member not %", "size()");
+ message_sz = perform_implicit_conversion (size_type_node, message_sz,
+ tf_none);
This should probably use build_converted_constant_expr?
+ if (message_sz == error_mark_node)
+ {
+ error_at (location, "% message % member "
+ "function must be implicitly convertible to "
+ "%");
+ return;
+ }
+ if (tree d
+
[PATCH] c++, v2: Implement C++26 P2741R3 - user-generated static_assert messages [PR110348]
On Thu, Aug 24, 2023 at 04:30:51PM +0200, Jakub Jelinek via Gcc-patches wrote:
> The following patch on top of PR110349 patch (weak dependency,
> only for -Wc++26-extensions, I could split that part into an independent
> patch) and PR110342 patch (again weak dependency, this time mainly
> because it touches the same code in cp_parser_static_assert and
> nearby spot in udlit-error1.C testcase) implements the user generated
> static_assert messages next to string literals.
>
> As I wrote already in the PR, in addition to looking through the paper
> I looked at the clang++ testcase for this feature implemented there from
> paper's author and on godbolt played with various parts of the testcase
> coverage below, and there are 4 differences between what the patch
> implements and what clang++ implements.
>
> The first is that clang++ diagnoses if M.size () or M.data () methods
> are present, but aren't constexpr; while the paper introduction talks about
> that, the standard wording changes don't seem to require that, all they say
> is that those methods need to exist (assuming accessible and the like)
> and be implicitly convertible to std::size_t or const char *, but rest is
> only if the static assertion fails. If there is intent to change that
> wording, the question is how far to go, e.g. while M.size () could be
> constexpr, they could e.g. return some class object which wouldn't have
> constexpr conversion operator to size_t/const char * and tons of other
> reasons why the constant evaluation could fail. Without actually evaluating
> it I don't see how we could guarantee anything for non-failed static_assert.
>
> The second and most important is that clang++ has a couple of tests (and the
> testcase below as well) where M.data () is not a core constant expression
> but M.data ()[0] ... M.data ()[M.size () - 1] is integer constant
> expression. From my reading of http://eel.is/c++draft/dcl.pre#11.2.2
> that means those should be rejected (examples of these are e.g.
> static_assert (false, T{});
> in the testcase, where T{}.data () returns pointer returned from new
> expression, but T{}'s destructor then deletes it, making it point to
> no longer live object. Or
> static_assert (false, a);
> where a.data () returns &a.a but because a is constexpr automatic variable,
> that isn't valid core constant expression, while a.data ()[0] is.
> There are a couple of others. Now, it seems allowing that is quite useful
> in real-world, but the question is with what standard changes to achieve
> that. One possibility would be s/a core constant/an/; from implementation
> POV that would mean that if M.size () is 0, then M.data () doesn't have
> to be constexpr at all. Otherwise, implementation could try to evaluate
> silently M.data () as constant expression, if it would be one, it could
> just use c_getstr in the GCC case as the patch does + optionally the 2
> M.data ()[0] and M.data ()[M.size () - 1] tests to verify boundary cases
> more carefully. And if it wouldn't be one, it would need to evaluate
> M.data ()[i] for i in [0, M.size () - 1] to get all the characters one by
> one. Another possibility would be to require that say ((void) (M.data ()), 0)
> is a constant expression, that doesn't help much with the optimized way
> to get at the message characters, but would require that data () is
> constexpr even for the 0 case etc.
>
> The third difference is that
> static_assert (false, "foo"_myd);
> in the testcase is normal failed static assertion and
> static_assert (true, "foo"_myd);
> would be accepted, while clang++ rejects it. IMHO
> "foo"_myd doesn't match the syntactic requirements of unevaluated-string
> as mentioned in http://eel.is/c++draft/dcl.pre#10 , and because
> a constexpr udlit operator can return something which is valid, it shouldn't
> be rejected just in case.
>
> Last is clang++ ICEs on non-static data members size/data.
>
> The patch implements what I see in the paper, because it is unclear what
> further changes will be voted in (and the changes can be done at that
> point).
> The patch uses tf_none in 6 spots so that just the static_assert specific
> errors are emitted and not others, but it would certainly be possible to
> use complain instead of tf_none there, get more errors in some cases, but
> perhaps help users figure out what exactly is wrong in detail.
Here is an updated version of the patch.
Compared to the last version, based on the discussion in the PR, the patch
1) warns (but only that) if size()/data() methods aren't declared
constexpr/consteval (or implicitly constexpr)
2) as I don't see a function which would determine if some expression
is core constant expression (for the data() case), the patch just as an
optimization tries to fold_nondependent_expr msg.data() expression
quietly, if it is a constant expression, passes it to c_getstr if len > 0
and if successful, only tries to constant expression evaluate
msg.data()[0] and msg.data()[len - 1]
