-
Notifications
You must be signed in to change notification settings - Fork 9
Add Get_Line function #213
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
base: master
Are you sure you want to change the base?
Conversation
Hi @godunko, would you mind taking a look at this? No worries if not, it would just be convenient to have it upstream. Thanks! |
begin | ||
loop | ||
if Self.Buffer.Slice (First_Character, Last_Character) | ||
.Ends_With (Terminators) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for MR!
However, things is not so easy:
- text buffer might contain incomplete data, containing single
CR
character at the end, whileCRLF
sequence is enabled, in this case next read operation will processLF
character as single character and not as continuation ofCRLF
sequence; - text buffer might contain incomplete data of the line; or last line of the file with out line terminator;
- application might want receive lines with or without line termination sequences.
(Data (Data'First .. Data'First - 1), True); | ||
|
||
elsif Interfaces.C_Streams.ferror (Self.Stream) /= 0 then | ||
Self.Error := "File IO error"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Success
be set to False
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably right, though this is just moved code from what's already in the repo. To be honest, I'm not sure if this is the way it is for a reason that I'm missing, or if it ought to be fixed.
Hi, thanks for looking over this, and my apologies for the delayed response! Let me know if this addresses your comments above; I'd hope that the version I just pushed is much more correct. In my brief testing, this worked fine, but I think that adding automatic tests would be warranted. (If you agree, I'd appreciate some guidance on how to get started using the test infrastructure you all have.) I'm also unsure that I'm handling inheritance correctly --- I see that Thanks again, and please let me know if there's anything I ought to change. |
You can add test driver here https://github.com/AdaCore/VSS/tree/master/testsuite/stream and modify https://github.com/AdaCore/VSS/blob/master/testsuite/common/test_support.ads for new tests. |
|
Hi, Sorry about this late response. I took a closer look and don't think that it makes sense to implement Get_Line as an inherited method; it requires being able to look ahead one character, which might not be supported by all input streams and doesn't really work if Input_Text_Stream is implemented on some object doing unbufferred IO. Let me know if you think that's reasonable, otherwise I can add Get_Line to the interface as you suggest. Regarding tests, is there a good way to reference a file with test data? Thanks again :) |
Hi,
Fixes #212. Please let me know if there's anything wrong or that can be improved here.
Thanks a ton!