This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [C/ObjC/C++/ObjC++] cleanup diagnostics initialization


On Mon, Apr 9, 2012 at 6:19 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 9 April 2012 12:43, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
>> On Mon, Apr 9, 2012 at 5:12 AM, Manuel López-Ibáñez
>> <lopezibanez@gmail.com> wrote:
>>> ? ? ? ?* c-common.h (c_common_initialize_diagnostics): Likewise.
>>
>> Make the comment less personal; we don't who "I" is in "I'm putting them here"
>> in three months (nor should we have to know.) ?I suggest to just remove
>> that comment.
>
> That comment has been there for ages, I just moved it around. But I am
> happy to remove it.

yes, thanks.

>
>> This deletion moves the initialization of cxx_pp to cxx_initialize_diagnostics.
>> That is the wrong place. ?As the comment says, cxx_pp is not for
>> diagnostics, so it should be initialized separately -- if possible as
>> early as possible.
>>> ? ? ? ?* cp-tree.h (init_error): Delete.
>>> ? ? ? ?* lex.c (cxx_init): Do not call init_error.
>>
>> this should still call a routine that initializes cxx_pp.
>
> Where?

Maybe in a dedicated function called construct_cxx_pp(), called
from cxx_init?

> cxx_initialize_diagnostics is run earlier than cxx_init, so it
> is now initialized earlier than before. Moreover, by putting both
> together, it is clear to anyone that there are two pretty-printers,
> and the comment clarifies what the second is for. I understand that
> init_error means "initialize_error routines", and indeed it contained
> code initializing diagnostics.

since it has nothing to do with diagnostics, it is better not to place
its initialization there.  Otherwise, in 3 months somewhere will come
and complain that the diagnostics and pretty printers are hard to debug
etc ;-)

>
> However, if the above does not convince you. What about renaming
> init_error to cxx_pp_init, and move the cxx_pp initialization there so
> it is clear that this function is ONLY to initialize cxx_pp and not
> for anything else?

that or the suggestion above.

>
> Is the patch OK with the above changes?

I appreciate your impatience but I would like to look at the revised
version first.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]