This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [C++ PATCH] PR c++/91369 - Implement P0784R7: constexpr new


On 10/3/19 2:38 PM, Jakub Jelinek wrote:
On Tue, Oct 01, 2019 at 05:56:00PM -0400, Jason Merrill wrote:
+		  else if (DECL_NAME (var) == heap_deleted_identifier)
+		    {
+		      if (!ctx->quiet)
+			error_at (loc, "deallocation of already deallocated "
+				       "storage");
+		      *non_constant_p = true;
+		      return t;
+		    }

Don't we need an error for trying to deallocate something that wasn't
allocated within the constexpr evaluation?

We don't need it, but it might be a good idea.  Right now it will just fall
through into the non-constexpr call in a constexpr context diagnostics,
while a message that it isn't constexpr because it is trying to deallocate
something that hasn't been allocated might be clearer.  Will add.

@@ -3011,8 +3067,10 @@ initialized_type (tree t)
       {
         /* A constructor call has void type, so we need to look deeper.  */
         tree fn = get_function_named_in_call (t);
-      if (fn && TREE_CODE (fn) == FUNCTION_DECL
-	  && DECL_CXX_CONSTRUCTOR_P (fn))
+      if (fn
+	  && TREE_CODE (fn) == FUNCTION_DECL
+	  && (DECL_CXX_CONSTRUCTOR_P (fn)
+	      || (cxx_dialect >= cxx2a && DECL_CXX_DESTRUCTOR_P (fn))))
   	type = DECL_CONTEXT (fn);

Why is this needed?  A destructor doesn't initialize anything, so returning
void seems appropriate.

This was needed initially, because the evaluate outermost expr if it sees
void type just returns early, does nothing.  But later I found out that while
the above worked for the normal dtor case, for the array dtor case it didn't
work anyway and I've added an extra argument.  So this is not needed anymore
and will delete.

I think we want to factor this function more, so we don't have the same code
in multiple places for handling an array, and an array member, and a pointer
to array.  Do you want to take a look at bug 71504 while you're touching
this code?

Patch posted separately.

+  if (!valp
+      && VAR_P (object)
+      && DECL_NAME (object) == heap_identifier)
+    {
+      tree ctor = build_constructor (type, NULL);
+      CONSTRUCTOR_NO_CLEARING (ctor) = true;
+      ctx->values->put (object, ctor);
+      valp = ctx->values->get (object);
+    }

Instead of this, how about giving the object NULL_TREE value when we create
it in cxx_eval_call_expression?

Will try.
+	      if (tree fld1 = TYPE_FIELDS (var_type))
+		if (TREE_CODE (fld1) == FIELD_DECL
+		    && DECL_NAME (fld1) == NULL_TREE
+		    && DECL_ARTIFICIAL (fld1)
+		    && TREE_CODE (TREE_TYPE (fld1)) == ARRAY_TYPE
+		    && COMPLETE_TYPE_P (TREE_TYPE (fld1)))
+		  if (tree fld2 = DECL_CHAIN (fld1))
+		    if (TREE_CODE (fld2) == FIELD_DECL
+			&& DECL_NAME (fld2) == NULL_TREE
+			&& DECL_ARTIFICIAL (fld2)
+			&& TREE_CODE (TREE_TYPE (fld2)) == ARRAY_TYPE
+			&& TYPE_DOMAIN (TREE_TYPE (fld2)) == NULL_TREE
+			&& DECL_CHAIN (fld2) == NULL_TREE)

Maybe give the struct a magic name so you don't need to do as much checking
of the FIELD_DECLs?

Ok.

So here you're completing the type of the array member of the struct.

+		TREE_TYPE (var) = var_type;
+		TREE_TYPE (TREE_OPERAND (op, 0))
+		  = build_pointer_type (var_type);
+	      }
+	  }

Let's factor out all of this code, too.

Yes, it is used twice, so factoring it out makes a lot of sense.

@@ -5525,16 +5762,23 @@ cxx_eval_outermost_constant_expr (tree t
     bool non_constant_p = false;
     bool overflow_p = false;
     hash_map<tree,tree> map;
+  auto_vec<tree, 16> heap_vars;
     HOST_WIDE_INT constexpr_ctx_count = 0;
     constexpr_ctx ctx = { NULL, &map, NULL, NULL, NULL, NULL,
-			&constexpr_ctx_count, allow_non_constant, strict,
-			manifestly_const_eval || !allow_non_constant };
+			&constexpr_ctx_count, &heap_vars, allow_non_constant,
+			strict, manifestly_const_eval || !allow_non_constant };

As we add more stuff to constexpr_ctx, creating new ones on the stack
becomes more and more expensive.  We should really split off the parts that
change frequently: Maybe just ctor/object, maybe also call/save_exprs/...?

I was thinking about creating a constexpr_outermost_ctx that would include
things that are global and don't change, at least the count and this vector.
Not sure about the bool fields, we access them way too often that it might
be too ugly to change all the ctx->quiet to ctx->outermost->quite or
similar.

@@ -5593,6 +5852,32 @@ cxx_eval_outermost_constant_expr (tree t
         non_constant_p = true;
       }
+  if (!heap_vars.is_empty ())
+    {
+      tree heap_var = cp_walk_tree_without_duplicates (&r, find_heap_var_refs,
+						       NULL);

Doesn't verify_constant already complain about remaining references to
allocated objects?

I believe it doesn't, because the heap VAR_DECLs are TREE_STATIC (things
really don't work at all if they aren't).

Ah, sure. I suppose you could clear TREE_STATIC from them before the verify_constant in this function?

@@ -6239,7 +6535,12 @@ potential_constant_expression_1 (tree t,
   		if (!DECL_DECLARED_CONSTEXPR_P (fun)
   		    /* Allow any built-in function; if the expansion
   		       isn't constant, we'll deal with that then.  */
-		    && !fndecl_built_in_p (fun))
+		    && !fndecl_built_in_p (fun)
+		    /* In C++2a, replaceable global allocation functions
+		       are constant expressions.  */
+		    && (cxx_dialect < cxx2a
+			|| !IDENTIFIER_NEWDEL_OP_P (DECL_NAME (fun))
+			|| CP_DECL_CONTEXT (fun) != global_namespace))

This is the second occurrence of this three-line test for a constexpr
(de)allocation function, let's factor it out.

Ok, will factor that out.

i.e.

struct {
   size_t cookie[N];
   elt array[];
};

I guess you want an array for the cookie rather than a non-array size_t data
member to handle ARM cookies that also include the element size. Please
include this in the comment.

That is actually what I'm using.  When build_new_1 creates it, it is
struct { size_t cookie[N]; T array[]; }; and when constexpr cast
rewrites it it becomes struct { size_t cookie[N]; T array[M]; };
with the M bound computed from the allocation size.

Yep, I'd like to see that in the comments.

@@ -1283,24 +1296,15 @@ process_subob_fn (tree fn, tree *spec_p,
         if (diag)
   	{
   	  inform (DECL_SOURCE_LOCATION (fn),
-		  "defaulted constructor calls non-%<constexpr%> %qD", fn);
+		  SFK_DTOR_P (sfk)
+		  ? G_("destructor calls non-%<constexpr%> %qD")

Not "defaulted"?

Will fix.

	Jakub



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]