`_const_cast` is de facto undefined behavior

[badasahog]

data.table/src/fread.c

Lines 126 to 133 in 9c54bd0

	/**
	* Drops `const` qualifier from a `const char*` variable, equivalent of
	* `const_cast<char*>` in C++.
	*/
	static char* _const_cast(const char *ptr) {
	union { const char *a; char *b; } tmp = { ptr };
	return tmp.b;
	}

I am aware that the usage of sidestepping constness is used to invoke copy on write behavior.

This is still undefined behavior.

I'm going to investigate to see if I can find a decent solution to this problem, and I also want to hear back if anyone disagrees with my assessment.

[aitap]

I think it won't be undefined behaviour to just cast the pointer to non-const. We mapped the memory; we know that the underlying buffer is writeable. If a compiler objects, maybe we could launder the pointer through uintptr_t.

[badasahog]

Why even const qualify the pointer, then? Is there any genuine performance benefit?

[aitap]

I don't know. Maybe it started out as const and then the author realised the need for CoW in the last page? It may be worth measuring the difference.

[MichaelChirico]

For context it was added in #2153; there's not much exposition there, but ostensibly they were trying to quiet some compiler warnings.

It's used twice:

data.table/src/fread.c

Line 1573 in 9c54bd0

*_const_cast(eof) = '\0'; // cow page

data.table/src/fread.c

Lines 1865 to 1866 in 9c54bd0

	*_const_cast(eof-1) = eol_one_r ? '\r' : '\n';
	*_const_cast(eof) = '\0';

Agree about proceeding cautiously if there's a big performance impact (particularly looking at the args.filename branch).

[badasahog]

we should at least rename it to just const_cast, because starting a global identifier with an underscore is also undefined behavior

[badasahog]

quick update!

we may be able to sidestep this issue by keeping a non const pointer to use for the '\0' write.

https://stackoverflow.com/questions/79650846/does-const-char-alias-with-char

I still think that removing the cow page is a good idea. It's cleaner and will result in performance gain.

C11's safe functions like strnlen_s would be ideal for this, but we're compiling with c99.

If maintainers find this to be a significant enough incentive to upgrade, I'm ready to update the code accordingly (I know that's a serious decision to be made, not something that can happen all of a sudden).