On Wed, Oct 17, 2012 at 6:26 AM, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote: > On 17 October 2012 11:55, Dodji Seketeli <do...@redhat.com> wrote: >> Hello Manuel, >> >> Let's CC Gaby on this one as well. >> >> Manuel López-Ibáñez <lopeziba...@gmail.com> writes: >> >>> The problem is that the macro unwinder code is changing the original >>> diagnostic type and not restoring it, so the code detecting that we >>> ICE fails to abort, which triggers another ICE, and so on. But there >>> is no point in modifying the original diagnostic, we can simply create >>> a temporary copy and use that for macro unwinding. >> >> We modify the context as well, and we set it back to its original value >> before getting out. Why not just doing the same for the diagnostic_info >> type? I mean, just save diagnostics->kind before changing it, and set it >> back to the saved value before getting out? That is less expensive than >> copying all of the diagnostic_info. > > Well, the difference is that for context, we are not sure that what we > get at the end of the function is actually the same that we received. > I am not sure if there is some buffer/obstack growth there. For > diagnostic_info it is very different: we want to return exactly the > original. (And in fact, both maybe_unwind_expanded_macro_loc and > diagnostic_build_prefix should take a const * diagnostic_info). > > Also, I am not sure why we need to restore the prefix. Once the > warning/error has been printed and we are just attaching notes from > the unwinder, we don't care about the original prefix so we may simply > destroy it. In fact, I think we are *leaking memory* by not destroying > the prefix. Perhaps the prefix should be destroyed always after the > finalizer and the default finalizer should be empty? > > Actually, I would propose going even a step further and use a more > high-level api instead of calling into the pretty-printer directly. > Something like: diagnostic_attach_note(context, const * > diagnostic_info, location_t, const char * message, ...) that for > example will check for context->inhibit_notes_p, will make sure the > message is translated, will make sure that diagnostic_info is not > overriden, will print the caret (or not), etc. This will live in > diagnostic.c and could be used by customized diagnostic hooks to > attach notes to an existing diagnostic. It would be a bit less > optimized than the current code, but more re-usable. > > What do you think?
That makes sense and is what we should do. > > It would be good to have Gaby's opinion as well, since what I am > proposing is more far-reaching. > > Cheers, > > Manuel.
