On 1/4/21 9:58 AM, Jeff Chapman wrote:
Ping. re:
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
<https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
> OK, I'll start with -alt then, thanks.
Andrew is exactly correct, contracts-jac-alt is still the current
branch we're focusing our upstreaming efforts on.
It's trailing upstream master by a fair bit at this point. I'll get
a merge pushed shortly.
The latest is still on the same branch, which hasn't been updated since
that last merge:
https://github.com/lock3/gcc/tree/contracts-jac-alt
<https://github.com/lock3/gcc/tree/contracts-jac-alt>
Would you prefer me to keep it from trailing upstream too much through
regular merges, or would it be more beneficial for it to be left alone
so you have a more stable review target?
Please let me know if there's initial feedback I can start addressing,
or anything I can do to help the review process along in general.
Why is some of the code in c-family? From the modules merge there is
now a cp_handle_option function that you could add the option handling
to, and I don't see a reason for cxx-contracts.c to be in c-family/
rather than cp/.
And then much of the code you add to decl.c could also move to the
contracts file, and some of the contracts stuff in cp-tree.h could move
to cxx-contracts.h?
+extern bool cxx23_contract_attribute_p (const_tree);
This name seems optimistic. :)
Let's call it cxx_contract_attribute_p.
+/* Return TRUE iff ATTR has been parsed by the fornt-end as a c++2a contract
"front"
@@ -566,7 +566,11 @@ decl_attributes (tree *node, tree attributes, int flags,
{
if (!(flags & (int) ATTR_FLAG_BUILT_IN))
{
- if (ns == NULL_TREE || !cxx11_attr_p)
+ if (cxx23_contract_attribute_p (attr))
+ {
+ ; /* Do not warn about contract "attributes". */
+ }
I don't want the language-independent code to have to know about this.
If you want decl_attributes to ignore these attributes, you could give
these attributes a dummy spec that just returns?
+set_decl_contracts (tree decl, tree contract_attrs)
+{
+ remove_contract_attributes (decl);
+ if (!DECL_ATTRIBUTES (decl))
+ {
+ DECL_ATTRIBUTES (decl) = contract_attrs;
+ return;
+ }
+ tree last_attr = DECL_ATTRIBUTES (decl);
+ while (TREE_CHAIN (last_attr))
+ last_attr = TREE_CHAIN (last_attr);
+ TREE_CHAIN (last_attr) = contract_attrs;
I think you want to use 'chainon' here.
@@ -5498,10 +5863,17 @@ start_decl (const cp_declarator *declarator,
if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
/* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
permerror (declarator->id_loc,
"declaration of %q#D outside of class is not definition",
decl);
+ else if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
+ /* Aliases are definitions. */
+ && !alias
+ && flag_contract_strict_declarations)
+ warning_at (declarator->id_loc, OPT_fcontract_strict_declarations_,
+ "non-defining declaration of %q#D outside of class", decl);
Let's keep the same message for the two cases.
+void
+finish_function_contracts (tree fndecl, bool is_inline)
This function needs a comment.
+/* cp_tree_defined_p helper -- returns TP if TP is undefined. */
+
+static tree
+cp_tree_defined_p_r (tree *tp, int *, void *)
+{
+ enum tree_code code = TREE_CODE (*tp);
+ if ((code == FUNCTION_DECL || code == VAR_DECL)
+ && !decl_defined_p (*tp))
+ return *tp;
+ /* We never want to accidentally instantiate templates. */
+ if (code == TEMPLATE_DECL)
+ return *tp; /* FIXME? */
In what context are you getting a TEMPLATE_DECL here? I don't see how
this would have an effect on instantiations.
+/* Parse a conditional-expression. */
+/* FIXME: should callers use cp_parser_constant_expression? */
+
+static cp_expr
+cp_parser_conditional_expression (cp_parser *parser)
...
+ /* FIXME: can we use constant_expression for this? */
+ cp_expr cond = cp_parser_conditional_expression (parser);
I don't think we want to use cp_parser_constant_expression for
expressions that are not intended to be constant.
+ bool finishing_guarded_p = true//!processing_template_decl
?
+ /* FIXME: Is this going to leak? */
+ comment_str = xstrdup (expr_to_string (cond));
There's no need to strdup here (and free a few lines later);
build_string_literal copies the bytes. The return value of
expr_to_string is in GC memory.
+ /* If we have contracts, check that they're valid in this context. */
+ // FIXME: These aren't entirely correct.
How not? Can local extern function decls have contract attributes?
+ if (tree pre = lookup_attribute ("pre", contract_attrs))
+ error_at (EXPR_LOCATION (TREE_VALUE (pre)),
+ "preconditions cannot be statements");
+ else if (tree post = lookup_attribute ("post", contract_attrs))
+ error_at (EXPR_LOCATION (TREE_VALUE (post)),
+ "postconditions cannot be statements");
+
+ /* FIXME: move fallthrough up here so it applies to decls/etc? */
That would make sense.
+ temp_override<tree> saved_cct(current_class_type);
+ temp_override<tree> saved_ccp(current_class_ptr);
+ temp_override<tree> saved_ccr(current_class_ref);
Are these actually needed?
+++ b/gcc/tree-inline.c
@@ -31,6 +31,7 @@ along with GCC; see the file COPYING3. If not see
#include "ssa.h"
#include "cgraph.h"
#include "tree-pretty-print.h"
+#include "cp/cp-tree.h"
Can't include cp-tree.h in a language-independent file. We already call
the tree-inline machinery from the front-end in clone_body; can't you do
the same for contracts?
+build_arg_list (tree fn)
+{
+ gcc_assert (TREE_CODE (fn) == FUNCTION_DECL);
+
+ vec<tree, va_gc> *args = make_tree_vector ();
+ for (tree t = DECL_ARGUMENTS (fn); t != NULL_TREE; t = TREE_CHAIN (t))
+ {
+ if (TREE_CODE (TREE_TYPE (t)) == POINTER_TYPE
+ && DECL_NAME (t) != NULL_TREE
+ && IDENTIFIER_POINTER (DECL_NAME (t)) != NULL
+ && id_equal (DECL_NAME (t), "this"))
You can use is_this_parameter here.
+ /* FIXME when we instatiate a template class with guarded
+ * members, particularly guarded template members, the resulting
+ * pre/post functions end up being inaccessible because their
+ * template info's context is the original uninstantiated class.
This sounds like a significant functionality gap. I'm guessing you want
to reconsider the unshare_template approach.
+// FIXME: are we sure we shouldn't just mangle or use some existing machinery?
+static const char *
+get_contracts_internal_decl_name (const char *ident)
Definitely use the mangling machinery.
+ /* FIXME do we need magic to perfectly forward this so we don't clobber
+ RVO/NRVO etc? */
Yes. CALL_FROM_THUNK_P will probably get you some of the magic you want.
More soon.
Jason
