On 1/19/26 10:57 PM, Iain Sandoe wrote:
Hi Jason,
Thanks for the review.
The attached patch will be part of the V3 series (which needs work to
patch 8 to complete).
The current series (including all review changes, completed to date) is
here:
https://forge.sourceware.org/iains/gcc-TEST/commits/branch/C%2B%2B26-contracts-base
The individual commits for fixes to this specific review are here:
https://forge.sourceware.org/iains/gcc-TEST/commits/branch/C%2B%2B26-contracts-base
(the branches are also on GH if that is easier to access)
https://github.com/iains/gcc-git/tree/C%2B%2B26-contracts-base
https://github.com/iains/gcc-git/tree/contracts_v2_patch_3_review
I believe that I’ve addressed the merge blockers here and also many of the
other points
including the typographical and comment ones.
This work has been rebased onto r16-6881-gddda478641ffcc, which includes the
reflection commits and the annual changes to (c).
how does it look now?
thanks
Iain
PS. We expect to post the complete V3 series in the next few days, after
addressing
recent review comments.
On 16 Jan 2026, at 16:16, Jason Merrill <[email protected]> wrote:
On 12/1/25 8:50 PM, Iain Sandoe wrote:
above. A future improvement planned here is to store the specifiers using a
tree vec instead of the attribute list.
So you removed the attributes from the decls, but kept the TREE_LIST format?
Interesting.
Until the discussions in Porto, we were expecting to continue to have cxx2a
contracts
still active; however having contract specifiers in the attributes was very
unhelpful since
the semantics of merging the attributes and the contracts duplication checks
are quite
different. So we had split the specifiers out, but since all the existing
code was expecting
an attribute list, we kept that.
Similarly, as we also discussed, we have re-used code from the cxx2a
implementation
that handled the various re-mappings of contract conditions (some of which we
all agree
is probably overly-complex and possibly obtuse for what it is doing) relevant
to a couple
of replies to later parts of this review.
Between Porto, things to prepare for WG21 and the end of stage #1 there was not
time to update all the interfaces or to replace the inherited code.
However, these elements are self-contained and local to the contracts
implementation
so it seems to me they could be improved once the code is in trunk (obviously
the
inherited code is already in trunk).
Agreed.
+ { "contract_assert", RID_CONTASSERT, D_CXXONLY | D_CXXWARN }, // removed
D_CXX20 in order for contracts to work out of the box
This breaks valid C++98-23 code that uses contract_assert as an identifier.
wa.C:3:8: error: expected unqualified-id before ‘contract_assert’
3 | auto contract_assert = 42;
This must be fixed before merge.
Done, also added logic for the Wc++26-compat and fixed up with
warn_keyword_macro.
Perhaps (at some point) we should add a ‘__contract_assert’ that is generally
available?
I would see no reason for contracts not to work back to at least C++17 (we might
want to consider if there are any implications to postconditions in the absence
of
guaranteed copy elision if we wanted to go earlier).
Makes sense.
/* These builtins shall be ignored during constant expression
evaluation. */
return void_node;
+ case BUILT_IN_OBSERVABLE_CHKPT:
+ if (ctx->manifestly_const_eval == mce_true)
Why only when manifestly constant-evaluated? Shouldn't we be able to
speculatively constant-evaluate a call to function that contains a checkpoint?
Fixed (i.e. observable checkpoints are now also ignored).
@@ -10175,7 +10180,18 @@ cxx_eval_constant_expression (const constexpr_ctx
*ctx, tree t,
case ASSERTION_STMT:
case PRECONDITION_STMT:
case POSTCONDITION_STMT:
- gcc_checking_assert (false && "hoo...");
+ {
+ if (contract_ignored_p (t))
+ break;
+
+ if (!cxx_eval_assert (ctx, CONTRACT_CONDITION (t),
+ G_("contract predicate is false in "
+ "constant expression"),
+ EXPR_LOCATION (t), contract_evaluated_p (t),
+ non_constant_p, overflow_p))
+ *non_constant_p = true;
+ r = void_node;
I thought the agreement was that contracts don't affect whether something is a
constant-expression, that if evaluating a contract ends up being non-constant,
that's a hard error if the evaluation is otherwise constant.
yes… I see - that would be:
https://eel.is/c++draft/basic.contract#eval-7.3
So we need to track non-constancy of this evaluation separately from the
normal non_constant_p (perhaps in constexpr_global_ctx) and check it in
_outermost_ to give an error before returning a constant value.
I see what you mean …
We also have
https://eel.is/c++draft/basic.contract#eval-8
https://eel.is/c++draft/intro.compliance#general-2.3.4
Which means I think we need to stash
- if it failed 7.3
- if it passed 7.3 but evaluated false
- the semantic in force
- the location.
(or maybe some higher-level thing that allows us to retrieve these).
Done.
Great.
+ }
break;
case TEMPLATE_ID_EXPR:
@@ -12519,7 +12524,9 @@ potential_constant_expression_1 (tree t, bool
want_rval, bool strict, bool now,
case ASSERTION_STMT:
case PRECONDITION_STMT:
case POSTCONDITION_STMT:
- gcc_checking_assert (false && "hmm...");
+ if (!contract_evaluated_p (t))
+ return true;
+ return RECUR (CONTRACT_CONDITION (t), rval);
Similarly, I would expect this to always return true.
Yes, we must assume that it would not affect the outcome at this point.
+ insert_decl_map (&id, sp, dp);
+ do_remap = true;
+
+ /* First artificial arg is *this. We want to remap that. However, we
+ want to skip _in_charge param and __vtt_parm. Do so now. */
Why do we want to skip them?
This is cxx2a inherited code, as noted in Porto some of it seems to be overly
sophisticated for what we need to do.
+/* Returns an invented parameter declaration of the form 'TYPE ID' for the
+ purpose of parsing the postcondition.
+
+ We use a PARM_DECL instead of a VAR_DECL so that tsubst forces a lookup
+ in local specializations when we instantiate these things later. */
??? tsubst_expr does retrieve_local_specialization for VAR_DECL, too. The
difference is that if that returns null, we then build a new PARM_DECL, but
only in unevaluated context, which a contract specifier shouldn't be...except
that you also add processing_postcondition, which seems like a big kludge. And
seems like it shouldn't be necessary since you register_local_specialization in
rebuild_postconditions. If we need to register_local_specialization elsewhere
(say, tsubst_contract), let's do that and lose processing_postcondition.
Umm .. inherited code ...
This comment is inherited, but processing_postcondition is not.
+ /* Always update the context of the result variable so that it can
+ be remapped by remap_contracts. */
+ DECL_CONTEXT (oldvar) = fndecl;
When would it not have the right context?
Much of the time it will be empty, since we do not have a function decl
when we are parsing the contract specifiers. We set the context here in
‘maybe_rebuild_postconditions` which is called after deferred parsing
or when we come to use the contracts (for free functions). It is also set
in tsubst for instantiations.
I added:
gcc_checking_assert (!DECL_CONTEXT (oldvar)
|| DECL_CONTEXT (oldvar) == fndecl);
But by the time we get to rebuild_postconditions in grokfndecl we've
already set DECL_CONTEXT. Deferred parsing is even later. And
tsubst_function_decl sets DECL_CONTEXT immediately after tsubsting the
parms.
+ bool viol_is_var = false;
+ if (TREE_CONSTANT (ctor))
I wonder about adding && !check_might_throw to avoid the need for
__tu_has_violation_exception? How much of a win is avoiding the variable?
I see you retracted this - but … for the record:
As mentioned earlier, the violation object ABI is still somewhat in flux which
makes
me reluctant to invest in changes just yet.
In tests, I found that the code size increase from the instructions to generate
the
object on the stack (or parameters for a call to some helper that creates it)
was
greater than the ro-data size and so it was worth doing this.
Code size growth from contracts was/maybe still is considered an important
issue (more so that performance - since this is the ‘failed’ path).
Agreed.
+ violation = build_contract_violation_constant (ctor, contract, /*is_const*/
+ true);
+ else
+ {
+ violation = build_contract_violation_var (ctor, contract);
+ add_decl_expr (violation);
+ BIND_EXPR_VARS (cc_bind) = violation;
+ viol_is_var = true;
+ }
+ /* So now do we need a try-catch? */
+ if (check_might_throw)
+ {
+ /* This will hold the computed condition. */
+ tree check_failed = build_decl (loc, VAR_DECL, NULL, boolean_type_node);
+ DECL_ARTIFICIAL (check_failed) = true;
+ DECL_IGNORED_P (check_failed) = true;
+ DECL_CONTEXT (check_failed) = current_function_decl;
+ layout_decl (check_failed, 0);
+ add_decl_expr (check_failed);
+ DECL_CHAIN (check_failed) = BIND_EXPR_VARS (cc_bind);
+ BIND_EXPR_VARS (cc_bind) = check_failed;
+ /* This will let us check if we had an exception. */
+ tree no_excp_ = build_decl (loc, VAR_DECL, NULL, boolean_type_node);
+ DECL_ARTIFICIAL (no_excp_) = true;
+ DECL_IGNORED_P (no_excp_) = true;
+ DECL_CONTEXT (no_excp_) = current_function_decl;
+ DECL_INITIAL (no_excp_) = boolean_true_node;
+ layout_decl (no_excp_, 0);
+ add_decl_expr (no_excp_);
+ DECL_CHAIN (no_excp_) = BIND_EXPR_VARS (cc_bind);
+ BIND_EXPR_VARS (cc_bind) = no_excp_;
I don't see why you need two variables:
+ tree check_try = begin_try_block ();
+ finish_expr_stmt (cp_build_init_expr (check_failed, cond));
+ finish_try_block (check_try);
+ tree handler = begin_handler ();
+ finish_handler_parms (NULL_TREE, handler); /* catch (...) */
+ tree e = cp_build_modify_expr (loc, no_excp_, NOP_EXPR,
boolean_false_node,
+ tf_warning_or_error);
+ finish_expr_stmt (e);
+ tree s_const = build_int_cst (uint16_type_node, semantic);
+ if (viol_is_var)
+ {
+ /* We can update the detection mode here. */
+ tree memb
+ = lookup_member (builtin_contract_violation_type,
+ get_identifier ("_M_detection_mode"),
+ 1, 0, tf_warning_or_error);
+ tree r
+ = build_class_member_access_expr (violation, memb, NULL_TREE, false,
+ tf_warning_or_error);
+ r = cp_build_modify_expr (loc, r, NOP_EXPR,
+ build_int_cst (uint16_type_node, (uint16_t)CDM_EVAL_EXCEPTION),
+ tf_warning_or_error);
+ finish_expr_stmt (r);
+ finish_expr_stmt (build_call_n (__tu_has_violation, 2,
+ build_address (violation),
+ s_const));
...here instead of calling __tu_has_violation you could set check_failed.
+ }
+ else
+ /* We need to make a copy of the violation object to update. */
+ finish_expr_stmt (build_call_n (__tu_has_violation_exception, 2,
+ build_address (violation),
+ s_const));
...and here after calling __tu_has_violation_exception you could clear
check_failed.
Oops, no, we need to make the call inside the handler because the standard says
there's an active handler.
+ finish_handler (handler);
+ finish_handler_sequence (check_try);
+ cond = build2 (TRUTH_ANDIF_EXPR, boolean_type_node, check_failed,
no_excp_);
...and then just set cond to check_failed.
TODO: on hold until the library ABI is finalised.
Dropping no_excp_ seems like a simple non-ABI change, but I guess it can
wait.
+ else if (fn && processing_contract_condition && DECL_DESTRUCTOR_P (fn))
+ error ("invalid use of %<this%> after it is valid");
else if (fn)
error ("invalid use of %<this%> in non-member function");
else
@@ -4674,6 +4688,9 @@ process_outer_var_ref (tree decl, tsubst_flags_t
complain, bool odr_use)
}
return error_mark_node;
}
+ else if (processing_contract_condition && (TREE_CODE (decl) == PARM_DECL))
+ /* Use of a parameter in a contract condition is fine. */
+ return decl;
Only if it's a parameter of the current function.
Why does outer_var_p return true in this case?
At this point the PARM does not have DECL_CONTEXT since we don’t yet have the
function decl.
If we have a PARM_DECL and outer_var_p is true (and we are processing a
contract condition)
it’s not clear how the parm could belong to anything else, I guess I’m missing
something?
void f (int i) {
struct A {
void g() pre(i);
};
}
I guess you want to add a check that DECL_CONTEXT is null.
...though maybe outer_var_p should return false for a PARM_DECL with
null DECL_CONTEXT.
+/* If we have a successful constant evaluation, now check whether there is
+ a failed or non-constant contract that would invalidate this. */
+
+static bool
+check_for_failed_contracts (constexpr_global_ctx *global_ctx)
+{
+ if (!flag_contracts || !global_ctx->contract_statement)
+ return false;
+
+ /* [basic.contract.eval]/7.3 */
+ location_t loc = EXPR_LOCATION (global_ctx->contract_statement);
+ if (global_ctx->contract_condition_non_const)
+ {
+ error_at (loc, "contract condition is not constant");
+ return true;
+ }
From your [basic.contract.eval] citation, this also counts as a
contract violation, so it also should be an error only for terminating
semantic.
+ /* [basic.contract.eval]/8. */
+ if (contract_terminating_p (global_ctx->contract_statement))
+ {
+ error_at (loc, "contract predicate is false in constant expression");
+ return true;
+ }
+ /* [intro.compliance.general]/2.3.4. */
+ warning_at (loc, 0, "contract predicate is false in constant expression");
Instead of separate error/warning calls you could call emit_diagnostic
with diagnostics::kind::error/warning.
@@ -3656,6 +3666,10 @@ finish_this_expr (void)
}
else if (fn && DECL_STATIC_FUNCTION_P (fn))
error ("%<this%> is unavailable for static member functions");
+ else if (fn && processing_contract_condition && DECL_CONSTRUCTOR_P (fn))
+ error ("invalid use of %<this%> in a constructor %<pre%> condition");
+ else if (fn && processing_contract_condition && DECL_DESTRUCTOR_P (fn))
+ error ("invalid use of %<this%> a destructor %<post%> condition");
Missing "in"
Jason
