Re: [RFC] Fission patch 1/2

[Doug Evans <dje@google.com>]

On Fri, Apr 13, 2012 at 10:01 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> +void
> Doug> +nullify_cleanup (struct cleanup *cleanup)
> Doug> +{
> Doug> +  struct cleanup *c;
> Doug> +
> Doug> +  for (c = cleanup_chain; c->next != cleanup; c = c->next)
> Doug> +    continue;
> Doug> +  c->function = null_cleanup;
> Doug> +}
>
> I'd rather not have a general facility for this kind of thing in
> cleanups.  I think it makes them even harder to reason about.  Instead
> the problem can be solved locally by making the particular cleanup work
> conditionally.

I wonder if to some, but not complete, extent (*1) cleanups are more
fragile than necessary because the API is more fragile than necessary.
My intent was the opposite, but ok, such is life.

[(*1) insert C++ vs C war :-)]

> Doug> +   FIXME: As an implementation detail between our callers and us,
> Doug> +   USE_EXISTING_CU and KEEP are OK.  But bubbling them up into their callers
> Doug> +   isn't as clean as I'd like.  Having more callers with good names
> Doug> +   may be the way to go.  */
>
> I'd just remove it.

"works for me"

> Doug> +  if (free_cu_cleanup != NULL)
>
> This sort of check is dangerous.  A call to make_cleanup can return NULL
> in some situations -- not this particular situation, but if someone
> later modifies the code this can break.
>
> It is better to keep a separate flag.

That sounds pretty odd (and error prone).  Are there *useful*
situations in which make_cleanup can return NULL?  Is it only the
first one?  It feels like it would be cleaner if that were never true,
and thus the users needn't have a separate flag, and thus can be
simpler (and thus the intuitive choice isn't the wrong thing to do).

> Doug> +   The CU "per_cu" pointer is needed because offset alone is not enough to
> Doug> +   uniquely identify the type.  A file may have multiple .debug_types sections,
> Doug> +   or the type may come from a DWO file.
>
> I wonder if this fixes PR 13627.

I didn't know the PR, but it did feel like a bug fix for that exact situation.