Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BC not allowing define methods #42

Closed
depler opened this issue Sep 26, 2021 · 4 comments
Closed

BC not allowing define methods #42

depler opened this issue Sep 26, 2021 · 4 comments

Comments

@depler
Copy link
Contributor

depler commented Sep 26, 2021

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

image

@gavinhoward
Copy link
Owner

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
Copy link
Owner

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 191f409.

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

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

@depler
Copy link
Contributor Author

depler commented Sep 26, 2021

191f409 is ok, working fine. I don't really understand meaning of c0a0ebd:

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
Copy link
Owner

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().

@depler depler closed this as completed Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants