Re: [1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Frédéric Delanoy]

2011/10/4 Dan Kegel :
> -
> - handle_type: type of hIn handle
> -
> - 0 if file, 1 if console, anything else if unknown (autodetect)
>
> I suspect you want an enum for that.

Well I thought about that, but found it a bit overkill for such a
limited set of possible values.
Also, if I used sthg like

enum CMD_HANDLE_TYPE {
   CMD_HT_FILE = 0,
   CMD_HT_CONSOLE = 1,
   CMD_HT_UNKNOWN = 2
}

, I'd have to add error checking for unknown parameters passed to
WCMD_ReadFile, or never check for CMD_HT_UNKNOWN, and just use it as
default value.
So TBH I don't think an enum is really necessary (especially since
it's just a corner case; all "similar" functions use a BOOL
is_console_handle, covering cases 0 and 1. I wouldn't want to change
those)

[1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Dan Kegel]

2011/10/4 Frédéric Delanoy :
> Well I thought about that, but found it a bit overkill for such a
> limited set of possible values.
> Also, if I used sthg like
>
> enum CMD_HANDLE_TYPE {
>   CMD_HT_FILE = 0,
>   CMD_HT_CONSOLE = 1,
>   CMD_HT_UNKNOWN = 2
> }
>
> , I'd have to add error checking for unknown parameters passed to
> WCMD_ReadFile, or never check for CMD_HT_UNKNOWN, and just use it as
> default value.
> So TBH I don't think an enum is really necessary (especially since
> it's just a corner case; all "similar" functions use a BOOL
> is_console_handle, covering cases 0 and 1. I wouldn't want to change
> those)

It's not overkill, and you don't have to add error checking, the
compiler will or should do that as long as you make all
the types that enum.

Really, the annoying part is the magic opencoded constants.
Those will set off alarm bells in anyone reviewing the code.

[1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Eric Pouech]

Le 4 octobre 2011 13:40, Frédéric Delanoy  a
écrit :
> 2011/10/4 Dan Kegel :
> > -
> > - handle_type: type of hIn handle
> > -
> > -
> >          0 if file, 1 if console, anything else if unknown
>
> (autodetect)
> > I suspect you want an enum for that.
>
> Well I thought about that, but found it a bit overkill for such a
> limited set of possible values.
> Also, if I used sthg like
>
> enum CMD_HANDLE_TYPE {
>   CMD_HT_FILE = 0,
>   CMD_HT_CONSOLE = 1,
>   CMD_HT_UNKNOWN = 2
> }
>
> again, this is not needed (passing all thoses CMD_HT bits)

static inline BOOL is_console_handle(HANDLE h) {return ((DWORD_PTR)h) & 3 ==
3;}
will test every handle and tell whether it's a console or a regular (file,
pipe...) handle

A+
> --
>
> Eric Pouech

[1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Frédéric Delanoy]

2011/10/4 Eric Pouech :
> Le 4 octobre 2011 13:40, Frédéric Delanoy  a
> écrit :
> > 2011/10/4 Dan Kegel :
> > > -
> > > - handle_type: type of hIn handle
> > > -
> > > - 0 if file, 1 if console, anything else if unknown
> > >
> > > (autodetect)
> > >
> > > I suspect you want an enum for that.
> >
> > Well I thought about that, but found it a bit overkill for such a
> > limited set of possible values.
> > Also, if I used sthg like
> >
> > enum CMD_HANDLE_TYPE {
> >   CMD_HT_FILE = 0,
> >   CMD_HT_CONSOLE = 1,
> >   CMD_HT_UNKNOWN = 2
> > }
>
> again, this is not needed (passing all thoses CMD_HT bits)
>
> static inline BOOL is_console_handle(HANDLE h) {return ((DWORD_PTR)h) & 3 ==
> 3;}
> will test every handle and tell whether it's a console or a regular (file,
> pipe...) handle
>
> Eric Pouech

OK but the purpose is to avoid checking the handle type for every line
read. Granted, one could use '((DWORD_PTR)h) & 3 == 3' instead of
GetConsoleMode or similar function.
(there's currently code like "BOOL is_console = GetConsoleMode(...);
...; while WCMD_fgets(..., is_console)" and the handle type shouldn't
change between lines/iterations, so why bother recomputing it every
time?)

[1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Juan Lang]

> OK but the purpose is to avoid checking the handle type for every line
> read. Granted, one could use '((DWORD_PTR)h) & 3 == 3' instead of
> GetConsoleMode or similar function.
> (there's currently code like "BOOL is_console = GetConsoleMode(...);
> ...; while WCMD_fgets(..., is_console)" and the handle type shouldn't
> change between lines/iterations, so why bother recomputing it every
> time?)

You're probably not.  The compiler will probably inline this function,
or at least its return value, since it knows it's guaranteed not to
change.  Please don't do the compiler's work for it:  IMHO, it's more
readable to make an inline function to determine whether the handle is
a console handle.
--Juan

[1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Frédéric Delanoy]

2011/10/4 Juan Lang :
> > OK but the purpose is to avoid checking the handle type for every line
> > read. Granted, one could use '((DWORD_PTR)h) & 3 == 3' instead of
> > GetConsoleMode or similar function.
> > (there's currently code like "BOOL is_console = GetConsoleMode(...);
> > ...; while WCMD_fgets(..., is_console)" and the handle type shouldn't
> > change between lines/iterations, so why bother recomputing it every
> > time?)
>
> You're probably not.  The compiler will probably inline this function,
> or at least its return value, since it knows it's guaranteed not to
> change.  Please don't do the compiler's work for it:  IMHO, it's more
> readable to make an inline function to determine whether the handle is
> a console handle.

I don't quite understand what you mean. WCMD_ReadFile is called
directly/indirectly in a lot of places, mostly around line-by-line
reads.
The question is not whether the compiler will inline this function
(you can't know that for sure, think -O0, different compilers, etc.),
but whether you want to check that every time.

This function is_console_handle() could be inlined or not/fast, but
that's not the point. The point is why would one want to run that
function when not necessary, i.e. when callers already know the type.
It's just about finding/detecting the handle type once and only once
(per handle).

[1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile

[Eric Pouech]

> I don't quite understand what you mean. WCMD_ReadFile is called
> directly/indirectly in a lot of places, mostly around line-by-line
> reads.
> The question is not whether the compiler will inline this function
> (you can't know that for sure, think -O0, different compilers, etc.),
> but whether you want to check that every time.
>
> This function is_console_handle() could be inlined or not/fast, but
> that's not the point. The point is why would one want to run that
> function when not necessary, i.e. when callers already know the type.
> It's just about finding/detecting the handle type once and only once
> (per handle).

the point is about passing twice the information to a given function
the point is about adding unnecessary enums that clutter all the code
the point is about thinking that you're only computing once a value, 
while you still
need to compare the handle type (as you added it) to a given number
A+