Update read_string_from_file to not include line feed #288
Conversation
| result.data[last] = '\0'; | ||
| last--; | ||
| } | ||
| } |
There was a problem hiding this comment.
oneliner
// remove trailing line ending characters if present
if (ok && !IsLegacyScript(thread))
{
result.data[strspn(result.data, "\n\r")] = '\0';
}
There was a problem hiding this comment.
I also think it should be a default behavior for File::readString
There was a problem hiding this comment.
seems like opposite what we need
There was a problem hiding this comment.
seems like opposite what we need
what exactly?
There was a problem hiding this comment.
strspn counts characters until first NOT included in second string occur.
So it would return 0 in most cases?
There was a problem hiding this comment.
interesting. so we are going to keep
\rin the string?many modern software treat any combination of
\rand\nas line endings. should we follow the same?
That would need extra logic as stream does not behave like that by default.
There was a problem hiding this comment.
at some point we should modernize the codebase. I can't believe C++ in 2025 does not have any tools to solve it in more elegant way.
There was a problem hiding this comment.
Well that creates some ambiguities. \r\n\r Should be single line or three? Currently only \n is considered new line and CR is threaten as helping character that may occur.
There was a problem hiding this comment.
\r immediately followed by \n should be a single line endings. next \r is debatable, can be a line ending on it's own (very uncommon, especially on Windows) or be an input character (also uncommon). So, I'd say there are two lines: one empty and one with one character \r
1: ""
2: "\r"
There was a problem hiding this comment.
if we accept \r as line endings, you still have 2 lines, but both empty
1: ""
2: ""
There was a problem hiding this comment.
Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (2)
- cleo_plugins/FileSystemOperations/FileSystemOperations.cpp: Language not supported
- tests/cleo_tests/FilesystemOperations/0AD7.txt: Language not supported
|
Copilot: Wow, then why it even exists :P |
yea, it surprised me as well. that's what it supports now
|
MiranDMC
deleted the
update_read_string_from_file_to_not_include_line_feed
branch
Opcode reads until line feed, but line feed is included in the result. When I was doing some other quick test with file as input data it took me by surprise. It is also problematic as in most cases user do not want the line feed characters and there is no easy way to get rid of them in script.
DYOM also has bug where import objective texts from file feature adds unwanted "?" character at the end of every text. I'm not sure if this is DYOM's bug or maybe that feature worked differently in CLEO3.