pantoniou / libfyaml

Fully feature complete YAML parser and emitter, supporting the latest YAML spec and passing the full YAML testsuite.
MIT License
241 stars 74 forks source link

Added missing `unistd.h` include in `libfyaml.h` #40

Closed mfederczuk closed 2 years ago

mfederczuk commented 3 years ago

Added the missing include directive for unistd.h in libfyaml.h.

ssize_t was used in libfyaml.h even though unistd.h was not included, and ssize_t is defined in unistd.h.

I've had the issue that I included libfyaml.h before including unistd.h, which lead to compiler errors.

jlblancoc commented 2 years ago

It seems that the proper include for ssize_t should be #include <sys/types.h>, but #include <stdint.h> and #include <stdio.h> should already define it in a more platform-independent way (non Linux builds).

mfederczuk commented 2 years ago

Hmm, I don't think that there's really a guarantee that ssize_t will be defined in stdint.h or stdio.h, plus sys/types.h is already being included in fy-atom.c.

Also, AFAIK ssize_t is POSIX-only in the first place, so we're already being platform-dependent just by using ssize_t.

jlblancoc commented 2 years ago

Mmm... You are right :+1: Making the whole library more portable would be another big plus.

mfederczuk commented 2 years ago

Agreed, but that'll be a topic for another day.

For now I'll just change the include to sys/types.h.

pantoniou commented 2 years ago

There's another issue with unistd.h, namely that it's not present in non-posix platform likes windows. Neither is sys/types.h available.

I am very surprised you got any compiler errors by a missing ssize_t definition, since the header includes stdio.h for the definition of FILE, and along that the definition of the fread/fwrite functions which have size_t arguments.

All of these can be handled if I used the HAVE_UNISTD_H autoconf bit, but I don't want to generate the header file to install.

The best I can do is guard with an #ifdef expression, but can't just include either unistd.h or sys/types.h