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? It would be good to have Gaby's opinion as well, since what I am proposing is more far-reaching. Cheers, Manuel.
