Re: [RFC] Fission patch 1/2

[Doug Evans <dje at google dot 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.