git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Johannes SchindelinNext: Junio C Hamano
Message 20 of 29 in “Really fix the isatty() problem on Windows”
  1. 0/2 Really fix the isatty() problem on WindowsJohannes Schindelin, Dec 21, 2016
  2. 1/2 mingw: adjust is_console() to work with stdinJohannes Schindelin, Dec 21, 2016
  3. 2/2 mingw: replace isatty() hackJohannes Schindelin, Dec 21, 2016
  4. Junio C HamanoDec 21, 2016
  5. Johannes SixtDec 21, 2016
  6. Junio C HamanoDec 21, 2016
  7. Johannes SixtDec 22, 2016
  8. Johannes SchindelinDec 22, 2016
  9. 0/3 Really fix the isatty() problem on WindowsJohannes Schindelin, Dec 22, 2016
  10. 2/3 mingw: fix colourization on Cygwin pseudo terminalsJohannes Schindelin, Dec 22, 2016
  11. 3/3 mingw: replace isatty() hackJohannes Schindelin, Dec 22, 2016
  12. Johannes SixtDec 22, 2016
  13. Johannes SchindelinDec 22, 2016
  14. Johannes SixtDec 22, 2016
  15. Johannes SchindelinDec 22, 2016
  16. 1/3 mingw: adjust is_console() to work with stdinJohannes Schindelin, Dec 22, 2016
  17. Beat BolliDec 22, 2016
  18. Junio C HamanoDec 22, 2016
  19. Johannes SchindelinDec 23, 2016
  20. Beat BolliDec 23, 2016
  21. Junio C HamanoDec 22, 2016
  22. Johannes SchindelinDec 22, 2016
  23. Junio C HamanoDec 22, 2016
  24. Junio C HamanoDec 22, 2016
  25. 0/3 Really fix the isatty() problem on WindowsJohannes Schindelin, Dec 22, 2016
  26. 1/3 mingw: adjust is_console() to work with stdinJohannes Schindelin, Dec 22, 2016
  27. 2/3 mingw: fix colourization on Cygwin pseudo terminalsJohannes Schindelin, Dec 22, 2016
  28. 3/3 mingw: replace isatty() hackJohannes Schindelin, Dec 22, 2016
  29. Johannes SchindelinJan 18, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.