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: [PATCH] Caret diagnostics


On Sun, Apr 8, 2012 at 11:10 AM, Manuel LÃpez-IbÃÃez
<lopezibanez@gmail.com> wrote:
> On 8 April 2012 17:14, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
>> On Sun, Apr 8, 2012 at 7:06 AM, Manuel LÃpez-IbÃÃez
>> <lopezibanez@gmail.com> wrote:
>>> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>>>> On 04/07/2012 06:29 PM, Manuel LÃpez-IbÃÅez wrote:
>>>>>
>>>>> I'll be happy to change it to whatever is more understandable. I think
>>>>> in CSS is called "padding".
>>>>
>>>>
>>>> That wouldn't be any clearer; my confusion was that I thought you were
>>>> padding the right side of the source line, but you aren't; you are only
>>>> making sure that the caret isn't too close to the edge of the terminal. ÂNo
>>>> need to change anything here.
>>>>
>>>>> +getenv_columns (void)
>>>>
>>>>
>>>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>>>> don't think the value can change while the compiler is running since we
>>>> don't respond to SIGWINCH. ÂAnd let's use this value in
>>>> c_common_initialize_diagnostics, too.
>>>
>>> That code is useless.
>>
>> I do not understand what you by that. ÂCould you elaborate?
>> c-family/ChangeLog says it was split out of c_common_init_options.
>
> I mean it has no effect because cxx_initialize_diagnostics overrides
> it before returning. Step in that function and you will see
> diagnostic_line_cutoff (context) change from 0 to 80 to 0.

OK.

>
>>> It is overridden by cxx_initialize_diagnostics
>>> (line 2429). It is not strange that nobody noticed this because the
>>> pretty-printer code is hard to understand and impossible to debug.
>>
>> I find the syllogism a bit specious.
>
> Sorry, ÂI didn't express myself in the best way out of frustration. I
> mean that the pretty-printer code is quite complex since there are
> many interacting parts, it emulates inheritance in plain C, and there
> is no encapsulation, so variables are sometimes modified directly and
> other times by using various functions. The abundant use of
> preprocessor macros as accessors and for implementing inheritance
> makes quite hard to debug the code in gdb. But this is a well-known
> problem of the whole GCC codebase. Probably, there is no better way to
> implement the same functionality in plain C, or perhaps gdb is not
> powerful enough, or perhaps I am using it incorrectly. In any case, I
> am having a very hard time following how the various pretty-printers
> are initialized.

Many of the pretty printer macros were introduced to abstract away
direct access to the fields.  The fact that some fields are accessed
directly in client codes is a bug in client codes, not a feature of the
pretty printer interface.  Alas C does not provide a convenient access
control, so these access violations go undetected when they slip
through careful reviews.  GDB has support for macros but it true
that debugging macro-based interfaces require some gymnastics,
and this one is no exception.

>
>>> ÂI'd rather not touch it if I can avoid it.
>>>
>>> I am not even sure if that is the correct way to set the line-cut-off
>>> in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
>>> knows what is the correct way?
>>
>> The documentation of pp_base_set_line_maximum_length says:
>>
>> /* Sets the number of maximum characters per line PRETTY-PRINTER can
>> Â output in line-wrapping mode. ÂA LENGTH value 0 suppresses
>> Â line-wrapping. Â*/
>
> So when is it appropriate to use
>
> diagnostic_line_cutoff (context) = 80;
>
> like in the above code?

First let me say this:  if you are doing diagnostics with caret, then you do
not want line wrapping, otherwise it gets a bit ugly.  So you need
a way to tell the diagnostic outputter that you do not want line wrap.

diagnostic_line_cutoff suggests the maximum number of characters that
the diagnostic outputter could send before emitting a newline to begin
line wrap.  0 suggests no line wrap.

So, I would not try to reuse that field -- otherwise, it can get messed up.
You suggested in your previous mail to use a distinct field to store the
width the terminal; I think that is a better option.

>
>>> Also, using COLUMNS to initialize pp's line-cut-off will change the
>>> current default (which is unlimited despite what invoke.texi says).
>>>
>>> I can add "locus_max_width" to diagnostic_context, and use
>>> getenv_columns to initialize it once. Would that be ok?
>>
>> I am not sure how its semantics would differ from pp_line_cutoff
>> but if you are going to disable line-wrapping mode (which this
>> effectively does), then the structure pp_wrapping_mode_t is
>> where to place this kind of information.
>
> Actually, what Jason is proposing is to initialize line_cutoff to
> getevn("COLUMNS") if present or to unlimited if not, and then use that
> for the caret diagnostic. I see two problems with this:
>
> * We change the current default, which is no line-wrapping ever.
>
> * Where should the default be set? By each FE? by diagnostic.c? by
> pretty-printer.c?
>

I would suggest the front-ends that want it.  If all front ends want it
then it makes sense to set it once for all in diagnostic.c.


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