This is the mail archive of the
mailing list for the GCC project.
Re: [patch] tree-ssa-dom.c: Use VEC(vrp_element_p,heap) instead ofVARRAY.
- From: Jeffrey A Law <law at redhat dot com>
- To: Kazu Hirata <kazu at cs dot umass dot edu>
- Cc: gcc-patches at gcc dot gnu dot org, nathan at codesourcery dot com
- Date: Tue, 03 May 2005 14:03:37 -0600
- Subject: Re: [patch] tree-ssa-dom.c: Use VEC(vrp_element_p,heap) instead ofVARRAY.
- References: <email@example.com>
- Reply-to: law at redhat dot com
On Wed, 2005-04-27 at 09:39 -0400, Kazu Hirata wrote:
> Attached is a patch to use VEC(vrp_element_p,heap) instead of VARRAY.
> There are two unintuitive changes.
> - vrp_records = vrp_hash_elt_p->records;
> - if (vrp_records == NULL)
> - return NULL;
> + vrp_records = &vrp_hash_elt_p->records;
> In this case, we have the following code immediately following the
> code above.
> limit = VEC_length (vrp_element_p, *vrp_records);
> if (limit == 0
> So if vrp_records is NULL, then VEC_length returns 0, so the the limit
> check catches the cases of vrp_records being NULL
This doesn't look right to me. If you read the comments earlier in the
code it states that two tests must be performed. First you have to
verify the existance of the varray (or VEC) then you have to look at
the number of active entries.
Your change will cause a segfault in the first case as *vrp_records
would be dereferencing a null pointer.
So my question to you would be is there some reason why it's no longer
possible for vrp_records to be null?
> - if (*vrp_records_p == NULL)
> - VARRAY_GENERIC_PTR_INIT (*vrp_records_p, 2, "vrp records");
> - VARRAY_PUSH_GENERIC_PTR (*vrp_records_p, element);
> + VEC_safe_push (vrp_element_p, heap, *vrp_records_p, element);
> In this case, *vrp_records_p == NULL really means that the vector is
No, it means the vector does not exist. There is a difference.
> VEC_safe_push reserves memory if a pointer holding a vector is
> null or we have not allocated any memory for the vector at all, so we
> don't need to check *vrp_records_p. It's included in VEC_safe_push.
> Since we are now explicitly managing the memory for these hash
> elements, I created a new function vec_free, which is responsible for
> vrp_hash_elt as well as the vector in it.
You may be trying to explain my objection above -- but I can't really
parse what you're trying to say. Can you try to reword this to be