xiaoyin0208 / lz4

Automatically exported from code.google.com/p/lz4
0 stars 0 forks source link

Add the missing header for isatty #95

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Windows has the right headers, non-windows is missing unistd.h

Original issue reported on code.google.com by luca.bar...@gmail.com on 16 Oct 2013 at 2:29

GoogleCodeExporter commented 8 years ago
https://github.com/lu-zero/lz4/commit/ed57f05823f76cfcc11ee485a791562d802ed269

Original comment by luca.bar...@gmail.com on 16 Oct 2013 at 2:30

GoogleCodeExporter commented 8 years ago
OK, sounds fine,

I was just wondering : 
how come <unistd.h> has not been missing in the list of includes so far ? 
lz4cli.c compiles fine in Unix, as far as I know.
Could <unistd.h> be automatically included into <stdio.h> ?

Original comment by yann.col...@gmail.com on 16 Oct 2013 at 9:58

GoogleCodeExporter commented 8 years ago
You do not make missing prototype an error so it won't break building lz4.

Original comment by luca.bar...@gmail.com on 16 Oct 2013 at 10:00

GoogleCodeExporter commented 8 years ago
missing prototype would at least trigger a warning. I don't get any so far.

Original comment by yann.col...@gmail.com on 16 Oct 2013 at 10:02

GoogleCodeExporter commented 8 years ago
-Wno-implicit-function-declaration obviously suppress it.

Original comment by luca.bar...@gmail.com on 16 Oct 2013 at 10:15

GoogleCodeExporter commented 8 years ago
Right, good point, I completely forgot that one.
Maybe time to revisit it.

Original comment by yann.col...@gmail.com on 16 Oct 2013 at 11:09

GoogleCodeExporter commented 8 years ago
If I can help you in better ways let me know, I started to play with
lz4 (thus the patchset in my personal tree).

Currently, beside that warning, lz4 seems building fine with the
standard warnings on.

Original comment by luca.bar...@gmail.com on 16 Oct 2013 at 12:59

GoogleCodeExporter commented 8 years ago
Apparently, I'm unable to find where fileno() is defined.

I removed -Wno-implicit-function-declaration, and applied your patch 
<unistd.h>, which works great for isatty().
However, fileno() is still issuing a warning.

All documentation I could find mention <stdio.h> as the library.
However, <stdio.h> is included; and nonetheless, fileno() generates a warning.

Original comment by yann.col...@gmail.com on 19 Oct 2013 at 9:36

GoogleCodeExporter commented 8 years ago

Original comment by yann.col...@gmail.com on 19 Oct 2013 at 9:55

GoogleCodeExporter commented 8 years ago
The following attached file is a release candidate
integrating your unistd.h suggestion.
It also removes the no-implicit-declaration switch on GCC.

Original comment by yann.col...@gmail.com on 19 Oct 2013 at 9:57

Attachments:

GoogleCodeExporter commented 8 years ago
You need to add -D_POSIX_SOURCE or -D_XOPEN_SOURCE (or both).

Original comment by luca.bar...@gmail.com on 19 Oct 2013 at 11:34

GoogleCodeExporter commented 8 years ago
yes, in the makefile.
But then, anyone trying to compile on its own (without the Makefile) would meet 
the same warning message.
Whenever applicable, I prefer to avoid dependencies to Makefile.

Original comment by yann.col...@gmail.com on 19 Oct 2013 at 11:43

GoogleCodeExporter commented 8 years ago
#define works fine in the file as well =) (-D something is easier for
me to tell you here)

Original comment by luca.bar...@gmail.com on 19 Oct 2013 at 11:45

GoogleCodeExporter commented 8 years ago
True,
the issue is that the #include <stdio.h> 
happens before any specific compiler/OS setting.
So it would require to modify this sequence.

Original comment by yann.col...@gmail.com on 19 Oct 2013 at 11:52

GoogleCodeExporter commented 8 years ago
Probably would fit next to where you set _FILE_OFFSET_BITS (the same effect is 
triggered by switching to C99 but I guess you would prefer to somehow support 
C89)

Original comment by luca.bar...@gmail.com on 19 Oct 2013 at 12:10

GoogleCodeExporter commented 8 years ago
OK, here is another release candidate,
with _POSIX_SOURCE defined.

Original comment by yann.col...@gmail.com on 19 Oct 2013 at 12:12

Attachments:

GoogleCodeExporter commented 8 years ago
Looks fine to me.

Original comment by luca.bar...@gmail.com on 19 Oct 2013 at 12:26

GoogleCodeExporter commented 8 years ago
Integrated into r107

Original comment by yann.col...@gmail.com on 21 Oct 2013 at 8:08