[RFC] Fission patch 1/2
Doug Evans
dje@google.com
Fri Apr 13 17:30:00 GMT 2012
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.
More information about the Gdb-patches
mailing list