freemint / libcmini

small footprint C standard library for Atari ST and m68k-atari-mint cross compiler toolchain
GNU Lesser General Public License v2.1
53 stars 6 forks source link

putc() et al do not automatically write carriage return when single new line is written to stdout #91

Closed rosenrost closed 4 years ago

rosenrost commented 4 years ago

E.g. putchar('\n') should write CRLF. Same should every fputc() to a file opened in text mode. This should not happen if the previous character written already was CR.

th-otto commented 4 years ago

i'm actually thinking about reverting all that binary mode handling. It is utterly broken, and handled at totally wrong places. Eg. in vfprintf() you introduced a single, static variable that is checked no matter what stream is actually written. It is not handled at all in fwrite, leading to inconsistencies depending on which functions are used.

rosenrost commented 4 years ago

After reviewing the code and doing some tests with Pure C, I think, this should be done in fwrite() because every f...() output function finally calls fwrite().

The last character written can be stored in the FILE structure to check for '\r' at the next call.

rosenrost commented 4 years ago

Maybe like this (not tested!):

size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream)
{
    long rc;

    if (stream->__mode.__binary)
    {
        rc = Fwrite(FILE_GET_HANDLE(stream), size * nmemb, ptr);
    } else
    {
        size_t remaining = size * nmemb;
        const char* str = ptr;
        const char* start = str;
        const char* end = str + remaining - 1;
        char carriage_return = '\r';
        long rcwr;

        rc = 0;

        while (remaining-- > 0)
        {
            if (*str == '\n' && stream->__last_char != carriage_return)
            {
                rcwr = Fwrite(FILE_GET_HANDLE(stream), 1, &carriage_return);

                if (rcwr < 0)
                {
                    rc = rcwr;
                    break;
                } else
                {
                    rc += rcwr;
                    rcwr = Fwrite(FILE_GET_HANDLE(stream), str - start + 1, start);

                    if (rcwr < 0)
                    {
                        rc = rcwr;
                        break;
                    } else
                    {
                        rc += rcwr;
                        start = str + 1;
                    }
                }
            }

            stream->__last_char = *str++;
        }

        if (rc >= 0 && start <= end)
        {
            rcwr = Fwrite(FILE_GET_HANDLE(stream), end - start + 1, start);

            if (rcwr < 0)
            {
                rc = rcwr;
            } else
            {
                rc += rcwr;
                stream->__last_char = *end;
            }
        }
    }

    if (rc < 0)
    {
        stream->__error = 1;
        __set_errno(-rc);
        rc = 0;
    } else
    {
        rc /= size;
    }

    return rc;
}
rosenrost commented 4 years ago

Is this still on your radar, Thorsten? I am using the code above for three months now and it looks quite good. I can push my changes for review. This includes the removal of this static variable in vfprintf.c, too, of course.

mfro0 commented 4 years ago

Let me answer this one.

I do not like to push this unless it's carefully tested (including corner cases like I/O redirection, etc).

I'm probably biased anyway (since I have my IT roots in the U*X world) regarding binary and "text" writing modes and always thought that automatic CR->CF/LF conversion is nonsense. In C, I usually write what I want to do and do not expect any "magic" conversion done behind my back, so I rather write "\r\n" if I really want that instead of experiencing strange errors when I do something that hasn't been tried (yet). Of course that's not your fault but that of the C standard ;).

If you have the feeling that you've covered most of the cases that might happen (as I do not have the time to do my own testing right now) and the code does what the standard says, I'm happy to accept a pull request, however.

rosenrost commented 4 years ago

I can understand you. But the opinion that the automatic conversion is nonsense ignores the fact that that is exactly what everyone is being taught when learning C. And which is one of the main advantages of this language: don't care about your environment, '\n' is always a newline (when you are in text mode, of course)!

In my opinion, the library doesn't have to care about redirection. If the file has been opened in text mode, the library must put a CR in front of a LF if it has not been passed explicitely.

We can wait for further opinions, but a library for an OS depending on CRLF for a full newline which doesn't automatically add a missing CR is useless for (in my opinion) the vast majority of source code available.

C was intended to be a portable language. And nobody wants to split his source code into #ifdefs just to take care of the newline sequence needed on the target machine, or am I wrong here?

th-otto commented 4 years ago

Theoretically you are right, but don' forget the goal of libcmini. If you need correct implementation of this on TOS ,you can always use mintlib. But libcmini was meant to be a lightweight alternative, which is mainly intended for GEM programs. Adding such a feature will only add a lot of complexity, and also lot of code. And the only case where it really matters is, when you write text to the console (that what markus meant with redirection i think).

As already mentioned earlier, instead of merging this fix, i would actually prefer to remove that conversion altogether again.