gavinhoward / bc

An implementation of the POSIX bc calculator with GNU extensions and dc, moved away from GitHub. Finished, but well-maintained.
https://git.gavinhoward.com/gavin/bc
Other
145 stars 29 forks source link

BC not allowing define methods #42

Closed depler closed 2 years ago

depler commented 2 years ago

The busybox version allows to define new methods within external file. This version does not?

image

gavinhoward commented 2 years ago

This should not happen and does not happen on other platforms as far as I am aware.

Thank you for reporting and helping me fix bc on Windows. I'll get right on investigating this.

gavinhoward commented 2 years ago

The real problem was that bc thought it had failed to read the file correctly. This happened because Windows is, in my opinion, braindead.

From the documentation for _read() on Windows (which I did not read carefully enough the first time through):

In text mode, each carriage return-line feed pair \r\n is replaced with a single line feed character \n. Only the single line feed character is counted in the return value. The replacement does not affect the file pointer.

(Emphasis added.)

This caused me to go "Wat?!".

Nevertheless, the fix was easy. It's commit 191f409260560485706247a7dbd9b147cd86fe8a.

I also added commit c0a0ebd054e670995ffccdfb2a08b753e0ef8529 before 191f409260560485706247a7dbd9b147cd86fe8a in order to make bc more resilient against partial reads.

Can you pull and test the latest master for me please?

depler commented 2 years ago

https://github.com/gavinhoward/bc/commit/191f409260560485706247a7dbd9b147cd86fe8a is ok, working fine. I don't really understand meaning of https://github.com/gavinhoward/bc/commit/c0a0ebd054e670995ffccdfb2a08b753e0ef8529:

size = (size_t) pstat.st_size;
buf = bc_vm_malloc(size + 1);

buf2 = buf;
to_read = size;

do {
        // Read the file. We just bail if a signal interrupts. This is so that
        // users can interrupt the reading of big files if they want.
        ssize_t r = read(fd, buf2, to_read);
        if (BC_ERR(r < 0)) goto read_err;
        to_read -= (size_t) r;
        buf2 += (size_t) r;
    } while (to_read);

to_read has initial value of pstat.st_size, so you are reading all data at once anyway. So why do you need loop? Also, I don't really believe that anyone will ever use more then 1mb of script file, and this will be instantly. Even reading of 1gb file will take less then a second on modern SSD, so I don't see any reason to overcomplicate things here.

gavinhoward commented 2 years ago

The reason is because read() is actually not guaranteed to read all of the available data at once on some platforms. (I don't know if Windows is one of those platforms.) This means that it can read only partial data and return no error. The loop is to ensure that it keeps trying to read as long as there is still data to read, thus ensuring that partial reads are not a problem.

In this case, I made the change separate from the bug fix for a reason: this is not relevant to the bug fix itself. But it is an improvement for some POSIX platforms that don't have ideal semantics for read().