Hacker News Reader

What's even with this style of code that seems to be trying to save up on local variables? It instead writes directly into the output buffer's pointer then immediately re-reads the value back from there and then performing the relevant logic (which may include un-writing the value from the buffer). Memory reads aren't that easily optimized away and this style easily leads to logic errors just like this one.

Also, I think you got the "what will work" question backwards with signed/unsigned. MSVC on x86/x64 has signed char and so, e.g., "isupper(c = fgetc(f))" segfaults on reading a non-ASCII char; and similarly in this case reading "я" from a Win-1251-encoded file (or "ÿ" from a Win-1252-encoded file) will be treated as (premature) EOF.

I dunno about this style of code, it looks hard to follow :/

> No newlines in keys, values, or section names. Empty values are not allowed. Comments only on their own lines (minus whitespace). Whitespace-insensitive (whitespace at the start of line, end of line, around the “=”, is all ignored). No need for a terminating newline either. Oh that's more than most C ini parsers do? Isn't that convenient

Nonsense. My ini parser has fewer restrictions (only one, on line length) and looks nothing like that unreadable mess.

Maybe unrelated, but I'm pretty certain I've seen gcc warnings when assigning ints to chars, unsigned or otherwise

The compiler is able to warn you about this:

    <source>:8:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]
    8 |         if (*ptr == EOF) {

Of course this warning only happens when you target ARM, so I can imagine it's still hard to catch if you do all your development on x86, and only occasionally cross-compile to ARM without heeding compiler warnings.

A good reason to always build with warnings as errors.

The bug is that it's missing abstractions. 1) reading directly from a FILE. 2) Saving local state through a pointer (ptr). 3) weird way to report back the out state, in case of EOF can't distinguish between ferror or not. 4) Probably it would be better to not check for errors right away -- just share a common path with EOF everywhere, and check for I/O errors only at the very end. 5) What is the use of this function? Not sure what's the point, it seems unergonomic. Probably better just code one loop for e.g. names ([_a-z][_a-z0-9]*) and integers / floats etc. 6) What's with the strange (*--ptr), overwriting previous work. Another bug waiting to happen. 7) Why are we returning out + skipped? Returning the consumed characters? That could just be coded in the parser input abstraction (where you would also keep track of the current file offset / possibly line and column (but you can compute those when you need them)). 8) What about the ferror(src) ? (skipped - out) : (out - skipped)? 9) 3 locations where a zero-terminator is written, when there should be _at most_ 1 location, but probably none.

If the code wouldn't do 2), the bug you describe would probably never have happened. The return value from fgetc() is int, not char. It has to be larger than char to be able to return a char as well as EOF.

I would probably code something along the lines of

    void identifier(Parser *parser)
    {
        My_String_Builder *builder = get_string_builder(parser);
        reset_string_builder(builder);
        for (;;)
        {
            string_builder_push(builder, c);
            if (! next_byte(parser))
                break;
            int c = parser_get(parser);           
            if (!(is_alpha(c) || c == '_' || ...))
                break;
        }
        String *string = string_builder_finalize(builder);

        // check string builder "overflow" / too long / further input sanitization

        push_string_token(parser, TOKEN_IDENTIFIER, string);
    }

Isn't this c++ not c, because in c there is no string class?

Just define your own one?

> Hint: it relies on implementation-defined behavior

I don’t think so.

fgetc returns an int (https://en.cppreference.com/w/c/io/fgetc) and EOF is an int (https://en.cppreference.com/w/c/io)

So,

   *ptr = fgetc(src);

discards part of the result of fgetc.

If my C isn’t too rusty, that means the code can’t discriminate between hitting end of file and reading some byte value from the files (AFAIK EOF is -1 on most systems, so that value would often be 0xFF)

EOF being -1 means that if (*ptr == EOF) can never fire if *ptr can only take on the values 0..255 [i.e., char is unsigned] as opposed to -128..127 [i.e., char is signed]. Whether or not char is signed or unsigned is implementation-defined.

This is interesting and I'm curious how is this being solved in the wild?

I'm on an ARM (m1 mac) machine and looks like char type is a signed type. EOF on this machine is -1

1. How can you distinguish between the char in the file and EOF (-1)?

2. Is EOF reserved for all files on macOS?

When fgetc succeeds, the int it returns has a value based on interpreting the byte as an unsigned char. On a platform with an 8-bit char, that means it’s in the range 0 to 255. This is required by the C spec regardless of whether char is signed or unsigned. Meanwhile, EOF is required to be negative. Thus, you can always distinguish the cases as long as you look at the original int return value rather than casting to char.

Fun fact: This approach causes trouble on obscure embedded platforms where char and int are the same size (and therefore an unsigned char value can’t fit inside a signed int). Such platforms are allowed by the C standard as freestanding implementations that don’t implement the full standard library, but they can’t conformantly implement fgetc. https://stackoverflow.com/questions/3860943/can-sizeofint-ev...

Every byte in a file, as represented in an int, takes on a value of 0..255. fgetc doesn't return a char, it returns an int, which means it returns a value of -1..255 (i.e., 257 possible values). If you try to represent the return value of fgetc as a char, two of those values get the same representation, namely 255 and -1. The difference between ARM and x86 is that on ARM (unsigned char), the -1-as-255 is represented as 255 when cast back to an int for comparison, whereas on x86, the 255 when cast back to an int is -1, so it would return the same value as EOF (although it would also do so had the value in the file originally been -1).

Representing binary data as unsigned char (as opposed to char or signed char) is the norm however.

Solve in the wild by never clipping the output of things like fgets()

    int ch = fgetc(src);
    if(ch == EOF)
       goto oops_eof;

    out++;
    *ptr++ = ch; *ptr = '\0';

    if(strchr(s, ch))
      return out;

or maybe

    int ch;
    if((ch = fgetc(src)) == EOF)
       goto oops_eof;

    *ptr[out++] = ch; *ptr[out] = '\0';

    if(strchr(s, ch))
      return out;

This (distinguishing -1 from a character value when char is unsigned) is one of the reasons character literals in C have type int instead of char (C++ changed them to char.) Note the return type of getchar().

>fgetc() reads the next character from stream and returns it as an unsigned char cast to an int, or EOF on end of file or error.

>getc() is equivalent to fgetc() except that it may be implemented as a macro which evaluates stream more than once.

>getchar() is equivalent to getc(stdin).

One thing I'm seeing: fgetc returns an int, so on a big-endian architecture the `*ptr = fgetc(src);` might yield even more interesting results.

Then, that fgetc() returns EOF on either an EOF or error is its major weakness — mixing in data and control all in-band — one is supposed to check with feof() and ferror() which of those two have happened, and if neither is true, then it's a data byte that just happens to be equal to the EOF constant. (This makes it quite pessimal when you are reading in a file that happens to contain lots of bytes equal to EOF, though).

> If instead you're on a system like ARM where char is unsigned, then you can never read EOF.

Then you should probably use an intermediate variable which is a proper int, gives you a surefire way to tell 0x000000ff and 0xffffffff apart.

But it's all gotchas like this which make me cringe every time someone suggests C is oh so very good and "simple", or even (gasp!) "convenient" to work with strings in general and text in particular. It's anything but. Hell, maybe it was better than alternatives back in some 1976, but then awk and Perl got invented, and Python followed soon after.

char cannot legally represent EOF (as you point out). This doesn't really have anything to do with ARM, though; for instance, the ARM compiler I use has command-line options and pragmas that control whether char is signed or unsigned. Char is just too small.

The type of 'out' is a different type than ssize_t, which may not matter for the practical ranges involved in parsing a file, but it certainly a bad sign.

Writes to memory it doesn't own if maxlen == 0 (unlikely, but who knows?)

Additionally: calling strchr on every input character is crazy. Frankly, the same applies to fgetc.

Ah, implicit type conversions being a footgun yet again.

    int fgetc( FILE *stream );

But the retval is treated as a char.

The retval is an int. It only gets treated as a char if it isn't EOF. The caller is supposed to know this and do the right thing rather than blindly downcasting. This is more apparent with the wide char library where there is a special wint_t with the sole purpose of serving as a return value that represents a wchar_t or WEOF.

AFAICS this is actually potential UB on most implementations: not only does fgetc() return -1 as an int in the failure case, it returns the byte as unsigned char in the success case. So on an implementation that defines plain char to have the same size and representation as signed char (as x86 does), reading a byte beyond SCHAR_MAX causes UB through signed overflow on the first line of the loop.

(This is a close relative of the well-known footgun that the is*() functions from <ctype.h> accept an unsigned char value as an int, and it’s completely valid—though rare in practice—for them to blow up when passed, say, "\xFF"[0] instead of ((const unsigned char *)"\xFF")[0] on an implementation with CHAR_BIT==8 and SCHAR_MIN<0.)

Overflow from signed arithmetic always results in UB. But an unsigned-to-signed conversion outside the range of the target type just results in implementation-defined behavior; see C17 §6.3.1.3 ("Signed and unsigned integers"). And most people these days (including me) take the simple approach of not supporting any implementation that doesn't just perform two's-complement wrapping for all conversions.

> And most people these days (including me) take the simple approach of not supporting any implementation that doesn't just perform two's-complement wrapping for all conversions.

C2x in fact requires 2's complement.

Not quite; C23 requires a two's-complement representation, but it doesn't require two's-complement wrapping for unsigned-to-signed conversions. §6.3.1.3 hasn't been changed, so it's still left to the implementation to guarantee that. The only easily-visible effects of requiring a two's-complement representation are that X_MIN == -X_MAX - 1 for all signed integer types, and in general that the object representations of x and -x - 1 differ by only 1 bit, if the integer type has no padding bits.

Is there any modern day architecture don't use twos complements?

GPUs notionally - I can't speak for truly "modern" GPUs but I recall older GPUs having language level "char"/"byte" implemented as a float (presumably with some minimal support to get expected semantics like clamping?)

Ah, so the “two’s complement from outer space” option (with X_MIN == -X_MAX and a trap representation in place of -XMAX-1) is also finally gone? I did not notice that, thanks.

C18 6.2.6.2p3 had:

> [It] is implementation-defined [...] whether the value with sign bit 1 and all value bits zero [...] is a trap representation or a normal value [for two’s complement].

Another bug: if `maxlen <= 0`, then it will overwrite the character before the start of `ptr`. And then there's the infinite loop if `maxlen > INT_MAX` on 64-bit machines. And maybe `out + skipped` can wrap as well?

maxlen is meant to be a constant provided by a #define, so I wouldn't be worried about things going haywire if maxlen is a weird value.

But C-like code in general makes me nervous in parsers, since there's many ways things can go wrong and C is inherently fail-deadly if you make a mistake.

Coding in C doesn't mean that you shouldn't create a layering of appropriate abstractions.

The problem is that writing abstractions in C is more difficult than virtually any other practical language.

That's a wild claim. Why do you think is that?

What is an abstraction and what do you need to code it?

One more: if an error occurs while reading the first character, the function returns -out = 0, which violates the documented behavior that errors are negative, and means a caller can’t distinguish between a failing to read and reading an empty file.

Years ago I ported some software from IRIX to some Linux-based OS. There were a few differences in the result. I eventually tracked it down to how on IRIX char is unsigned while the Linux-based OS used signed char.

The program used a 'char' to store the value of the formal atomic charge of an atom, which is typically 0, but for the sorts of chemistry I deal with can be +1, +2, -1, or -2, so it makes sense to allocate only 8 bits to store the 'char'ge. :)

On IRIX, where they had deployed the code for years, the charges were actually being interpreted as 0, +1, +2, 255, and 254.

Going back to the code example you gave, isn't there also an issue if read failure occurs on the first byte? Then `out` is 0, returning -0, which is 0, giving no way for the caller to distinguish between EOF and read failure.

That's a simple case, I've seen much worse because of endianess and different archs have different conventions for whether char is signed or unsigned by default (common these days with arm/X86). Any C programmer who's coded for X86 and ARM will know what the rules are and how not to break them (I'd like to hope, anyway).

I almost got it. I was like “whoa wait, does not fgetc return int? You truncate it to char, can you even compare it to EOF constant now?”. I did not remember the actual EOF constant or that char is unsigned on ARM, but I think I would pay attention to the compiler throwing a warning at me (it does throw a warning, does it not?)

fgetc() must return to int instead of char, otherwise you can not detect EOF.