On Tue, Apr 10, 2012 at 3:33 PM, Manuel López-Ibáñez
<lopeziba...@gmail.com> wrote:
> On 10 April 2012 21:41, Manuel López-Ibáñez <lopeziba...@gmail.com> wrote:
>> On 10 April 2012 21:31, Jason Merrill <ja...@redhat.com> wrote:
>>> On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:
>>>> +  max_width = context->caret_max_width;
>>>> +  if (max_width<= 0)
>>>> +    max_width = INT_MAX;
>>> I don't think we need the test here; diagnostic_set_caret_max_width should
>>> make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.
>> It was just a bit of defensive programming, since nothing stops anyone
>> to set the value directly. But I will remove it.
>>> On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:
>>>> There is a novelty in this new version that I don't think we discussed
>>>> before: automatic expansion of tabs to 8 hard space characters.  That
>>>> number should not be hardcoded as there is no reason to believe a tab
>>>> character always expands to 8 space characters.  You should check
>>>> the environment first; if not present the default expansion number should
>>>> be a symbolic constant as opposed to a magic number sprinkled all over the
>>>> places.
>>> Hmm.  I don't know if there's any reliable way to query tab stops; the "it"
>>> termcap/terminfo capability tells you what it was originally (presumably 8),
>>> but it might have changed.
>> No idea either, but it is in fact easier to print tabs a single
>> spaces. This simplifies the code a lot, as pointed by Gabriel. So if
>> you also prefer the simpler version, I am fine with committing that
>> one (it also saves space in the output).
> A new version of the patch. It removes the special handling of tabs.
> And abstracts out the adjusting of the line as Gabriel suggested. I am
> not sure what else could be abstracted out. The function is much
> shorter than other functions in diagnostic.c.
> This patch builds and I am running a full boostrap+check now.
> OK if it passes?


Reply via email to