[FileAccess] Return error codes from store_* methods.

[bruvzg]

Changes FileAccess methods from void store_*(...) to Error store_*().

See #77398, I was not able to reproduce an issue (might be related to overzealous antivirus activity), but file writes can fail for various reasons, and returning error seems like a good idea.

[RedworkDE]

FileAccess::get_error() already exists to get errors from load/store operations, tho it is pretty unclear when you should check it and when it is reset. IMO it should be fixed to be used consistently first, unless you plan on removing it entirely in favor of returning error codes, in which case what about the load methods?

[AThousandShips]

Maybe returning a bool for the write operations instead? To indicate success, and use the error method to check, makes the immediate check simpler, so you don't need to do err != OK, but still signals clearly

Alternatively add a simple convenience function has_error to do the same simplification, because that feels less unwieldy than comparing an error, also unsure how completely the get_error method is implemented, maybe the focus should be on making it more functional

[bruvzg]

Changed return values to bool, errors were non-descriptive anyway. get_error only handle EOF and file opening.

[m4gr3d]

I feel like as long as any bytes are written, we should return true especially as an error would trigger an exception.

[bruvzg]

Not writing all bytes is definitely a failure, but if it will trigger an exception (which seems to be the case for the file), there's no need for this check.

[m4gr3d]

The updates to the Android section look good!

[raulsntos]

Note that this will break compatibility in C# and there's no way to provide compat methods because the only thing that changes is the return type. We could consider adding new methods instead (e.g.: try_store_double).

[Bromeon]

Changed return values to bool, errors were non-descriptive anyway. get_error only handle EOF and file opening.

Are we sure that bool is enough? There are different reasons why writing can fail (opened in wrong mode, I/O error, disk space, etc.). Not that we see ourselves in the same situation in a few months, but with a too limited return code.

So it looks like get_error() is pretty much useless for writing at the moment? Do we plan to change that in the future?

[AThousandShips]

Needs to be moved to the 4.2 file now that it exists

[akien-mga]

Needs rebase.

[akien-mga]

Thanks!

Merged a bit too early - the CI failures are not caused by this PR but a previous merge batch.

[Bromeon]

Are we sure that bool is enough? There are different reasons why writing can fail (opened in wrong mode, I/O error, disk space, etc.). Not that we see ourselves in the same situation in a few months, but with a too limited return code.

So it looks like get_error() is pretty much useless for writing at the moment? Do we plan to change that in the future?

Could someone clarify that? Not arguing against the PR but I'm writing a higher-level abstraction and would appreciate some insight in the intended error handling ☺️ thanks!

[bruvzg]

I'm not sure why you would need any additional information, file write errors are non recoverable, the only thing you can do is close the file handle and assume that file, and disk it was on are gone.

[bruvzg]

Also the amount of info we can get is limited and platform dependent, so it won't be consistent.

But if you have some specific cases for it in mind, we can handle it by setting the last error value returned by get_error.

[Bromeon]

There are tons of reasons why writing can fail, file/medium gone is just one of them. There may be permission issues, another process locking the handle, disk full, ... Even C provides minimal information with errno, most other languages build more on top.

I'm not asking for a cross-platform API or error codes or so, just wondered if there's a particular design reason why the already existing get_error isn't used for writing? 🙂 (It may just be a case of "not yet implemented", which is perfectly fine as well 👍)

[geekley]

Documentation doesn't explain the meaning of the bool return value. Is true returned on success or on error? The comments in this PR don't clarify it either.