DevSolar / pdclib

The Public Domain C Library
https://pdclib.rootdirectory.de
Creative Commons Zero v1.0 Universal
228 stars 41 forks source link

Definition of `EOF` macro might cause issues with existing code (missing parentheses) #20

Closed JayFoxRox closed 3 years ago

JayFoxRox commented 3 years ago

This is the definition of EOF in pdclib: https://github.com/DevSolar/pdclib/blob/5950958ff57391789d9a164a56cd1ed87dedaa12/include/stdio.h#L35

it leads to issues with this code in jsoncpp: https://github.com/open-source-parsers/jsoncpp/blob/94cda30dbddc1859f111848fdd05dfb85d3287c7/src/lib_json/json_reader.cpp#L108

The error is:

/tmp/wdaoihs/jsoncpp/src/lib_json/json_reader.cpp:108:42: error: expected '(' after 'static_cast'
  std::getline(is, doc, static_cast<char> EOF);
                                         ^
                                         (

So this existing piece of (popular) code seems to assume that EOF will contain parentheses. I'm not sure if this is part of a C standard, a C++ standard, a convention / tradition, or a bug in my / our other toolchain components. I also checked the standard and it merely seems to say that the EOF macro is a negative int number.

However, for comparison, my GNU libc installation has this:

#define EOF (-1)

I also checked newlib: https://www.sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/stdio.h;h=164d95bca4a1e3cc66cad9319f4925d168874e05;hb=5fcbbf7ead615c116f8e08047093232b1325faf4#l118 (also (-1))

I could imagine that there could also be potential operator precedence issues without the parantheses?

DevSolar commented 3 years ago

I can certainly adapt PDCLib to have parenthesis in the definition of EOF. But this is not one, but two bugs in the implementation of jsoncpp.

The "stylistic" one, static_cast requires parenthesis, and assuming that the parameter provides them is questionable at best.

Two, the code is assuming that the input stream does not contain (char)EOF as character, which is a broken assumption. This is not the way one should "read until EOF" in C++.

I will add those parens, but this should definitely be reported to jsoncpp upstream as well.

JayFoxRox commented 3 years ago

I could imagine that there could also be potential operator precedence issues without the parantheses?

I was unable to find any.

I momentarily thought one would also need parantheses to prevent -EOF becoming --1, but it turns out this isn't an issue: https://stackoverflow.com/questions/56226486/macro-expansion-with-unary-minus

I will add those parens

Thanks! That'd be appreciated.

When I notice such issues I'll continue reporting and fixing them in the respective projects. However, staying closer to established conventions like (-1) will probably help avoiding similar problems with existing code, until it's all cleaned up.

but this should definitely be reported to jsoncpp upstream as well.

Absolutely agreed; reported in https://github.com/open-source-parsers/jsoncpp/issues/1288.

But even if its fixed there, then I'm sure that somewhere, someone, sometime also repeated a similar mistake elsewhere.

JayFoxRox commented 3 years ago

Closed by https://github.com/DevSolar/pdclib/commit/3757d6e81de71a3609ecf85a97d13c521b4dc5e1