Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin
- From
Beat Bolli <dev+git@drbeat.li>
- Date
- Dec 23, 2016, 12:51 UTC
- Message-ID
- <f6964a29e4bdc5a68fff56900cf67c36@drbeat.li>
- In-Reply-To
- <alpine.DEB.2.20.1612231024400.155951@virtualbox>
Hi Dscho
On 2016-12-23 10:30, Johannes Schindelin wrote:
Show 48 quoted lines
> Hi Beat,
>
> On Fri, 23 Dec 2016, Beat Bolli wrote:
>
>> On 22.12.16 18:08, Johannes Schindelin wrote:
>> > diff --git a/compat/winansi.c b/compat/winansi.c
>> > index cb725fb02f..590d61cb1b 100644
>> > --- a/compat/winansi.c
>> > +++ b/compat/winansi.c
>> > @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
>> > static int is_console(int fd)
>> > {
>> > CONSOLE_SCREEN_BUFFER_INFO sbi;
>> > + DWORD mode;
>>
>> Nit: can we move this definition into the block below where it's used?
>>
>> > HANDLE hcon;
>> >
>> > static int initialized = 0;
>> > @@ -98,7 +99,10 @@ static int is_console(int fd)
>> > return 0;
>> >
>> > /* check if its a handle to a console output screen buffer */
>> > - if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>> > + if (!fd) {
>>
>> Right here:
>> + DWORD mode;
>
> By that reasoning, the CONSOLE_SCREEN_BUFFER_INFO declaration that has
> function-wide scope should also move below:
>
>> > + if (!GetConsoleMode(hcon, &mode))
>> > + return 0;
>
> Right here.
>
>> > + } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>> > return 0;
>> >
>> > /* initialize attributes */
>
> As the existing code followed a different convention, so does my patch.
>
> If you choose to submit a change that moved the `mode` declaration to
> narrow its scope, please also move the `sbi` declaration for
> consistency.It's probably not worth it. It just jumped at me when reading the patch, and, writing much C++ recently, it looked weird to have a definition so far away from the single use of the variable.
Cheers, Beat