Add FileIO library (read, write to file) #374

PhucVH888 · 2016-04-12T17:34:26Z

I have added the basic File IO library to Encore prototype along with the test.

The library has basic functions as the following:

opening and closing file.
reading line from a file.
writing a line (String, or char*) to a file.
end of file checking.

EliasC · 2016-04-13T13:53:09Z

As far as I can see, I will never want to have a a File that is not (or has never been) open. Therefore I suggest merging fopen with the constructor so that typical usage becomes

let f = new File("foo.txt", "w");
f.write("foobar\n");
f.close();

EliasC · 2016-04-13T13:58:40Z

bundles/prototype/FileIO/IOTest.enc

+    while (f.isOpen()) {
+      print("{}",f.readline());
+    };
+    f.fclose();


This looks a bit weird. "While f is open, read lines from f. When f is no longer open, close f". If f.isOpen() returns false, I would expect that I do not need to close it. Maybe f.eof() is a better name?

EliasC · 2016-04-13T13:59:21Z

Minor comment, I think open and close are better names than fopen and fclose.

supercooldave · 2016-04-13T14:06:34Z

Just noting that you should create branches in your own copy of the repository and not in this copy (paraplu/encore) to keep the main repository as clean as possible.

Obviously, don't change anything for now. But for the next pull request.

EliasC · 2016-04-13T14:08:21Z

bundles/prototype/FileIO/IO.enc

+  -----------------------------------
+  -- Write to file.
+  -----------------------------------
+  def write(fout:String, fmode:String, content:String) : void {


Why do I need to pass the out-file and mode to this method? Don't I already have this information in the corresponding fields?

@supercooldave : Oh, I am sorry for my bad. I used git push without specifying my repository's name.

EliasC · 2016-04-13T14:20:14Z

At some point we could consider separating input streams from output streams to avoid having to guess if a File is for input or output, but I think this solution is good enough for now.

supercooldave · 2016-04-13T14:24:32Z

@pengstrom might look into doing some nicer IO based on Iteratees sometime in the future.

PhucVH888 · 2016-04-13T16:40:42Z

@EliasC : I agree with you, the names is quite misleading.

I think in general the name can be File but the identifiers such as fout = new File("test.txt","w") and fin = new File("test.txt","r") can be used to distinguish.

EliasC · 2016-04-13T17:00:05Z

bundles/prototype/FileIO/IO.enc

+  -----------------------------------
+  -- Write to file.
+  -----------------------------------
+  def write(fout:String, content:String) : void {


The fout argument is never used (it is passed to writeChar, which binds it to a variable that is never used). It should be removed.

(third time's the charm)
This argument is never used and should be removed.

EliasC · 2016-04-13T17:07:35Z

I think in general the name can be File but the identifiers such as fout = new File("test.txt","w") and fin = new File("test.txt","r") can be used to distinguish.

I strongly disagree. We cannot force our users to use a good naming convention, but we can design our libraries in a way that helps the user. Since an instream and an outstream present different interfaces, they shouldn't be the same type. If I open a file with mode "w" calling readline should be disallowed. Similarly, if I open a file with mode "r" calling write should be disallowed.

With this said, I think that this could be fixed later.

EliasC · 2016-04-13T17:08:00Z

(also, you will probably want to squash your commits for the final push)

albertnetymk · 2016-04-13T20:37:20Z

The squash can be done on github via merge button. FYI.

EliasC · 2016-04-14T14:10:58Z

bundles/prototype/FileIO/IO.enc

+
+  def eof() : bool {
+    let f = this.file;
+    embed bool


Could you use embed bool feof(#{f}); end instead? (only asking since you need another commit to remove the fout argument from write and writeChar)

It seems the embed bool feof(#{f}); end does not work correctly. It prints nothing.
While using previous implementation, it prints the file's content.

def eof() : bool { let f = this.file; embed bool bool res = true; if (ungetc(fgetc(#{f}),#{f})==EOF) res = false; else res = true; res; end; }

EliasC · 2016-04-19T07:01:31Z

Don't forget to fix the last tiny things so that we can get this merged :)

PhucVH888 · 2016-04-19T07:07:40Z

Thanks @EliasC . I have pushed the revised version.

TobiasWrigstad · 2016-04-19T07:07:58Z

What about async I/O? I worry about adding a file abstraction that will kill performance … because we have a tendency of letting that file become the file, plus breaking legacy code.

supercooldave · 2016-04-19T07:13:05Z

@TobiasWrigstad: There is no legacy code.
Perhaps we create a subdirectory of the module hierarchy called "FutureDeprecated".

EliasC · 2016-04-19T07:14:14Z

@TobiasWrigstad: I don't think that merging this is a big problem. There are other refactorings we should do here anyway, so it won't stay the file.

kikofernandez · 2016-04-19T07:18:33Z

I think this is good enough! Better to have a synchronous version than nothing at all.
We can do the same as other programming languages, we can provide the synchronous and asynchronous implementation

PhucVH888 · 2016-04-19T07:29:09Z

@TobiasWrigstad : I think the asyn I/O would be better. The sync version is used when I review the RequestHttp PL, I think a FileIO module is necessary to write out the result. I also have another asyn I/O version. Should I push it here?

TobiasWrigstad · 2016-04-19T07:31:55Z

I think a FileIO module is necessary to write out the result

What does this mean?

I also have another asyn I/O version. Should I push it here?

Do you have an async file abstraction?

PhucVH888 · 2016-04-19T07:41:55Z

I think a FileIO module is necessary to write out the result
As I want to write the test result to a file, so I implemented sync IO version.

There is still segmentation fault with this async version.
IO.enc

class File {
   ....
}

IOTest.enc

class Main 
  def main () : void {
    let file = "IOTest.out";    
    let f    = new File(file,"w");

    let msg  = "Hello World!\n";

    -- Test write file
    f.open(file,"a");
    f.write(file,msg);
    f.close();

    -- Test read file
    f.open(file,"r");
    while (not (get f.eof())) {
      print("{}", get f.readline());
    };
    f.close();

    print "Finished!"
  }

TobiasWrigstad · 2016-04-19T07:44:34Z

OK. I'm going with @kikofernandez's recommendation. Let's support what works now and add async I/O later.

EliasC · 2016-04-19T08:01:30Z

bundles/prototype/FileIO/IO.enc

+  def writeChar(fout:String, content:embed char* end) : void {
+    let 
+      fout1 = fout.data 
+      mode  = this.mode.data


These two variables also aren't used.

EliasC · 2016-04-19T08:05:47Z

@PhucVH888: Please remove the unused variables from write and writeChar!

PhucVH888 · 2016-04-19T09:29:25Z

@EliasC : I have fixed it. Tack!

EliasC · 2016-04-19T09:41:57Z

It seems the embed bool feof(#{f}); end does not work correctly. It prints nothing.

I think has uncovered a bug! Your implementation of eof returns true when the end of file has not been reached:

    while (f.eof()) {
      print("{}", f.readline());
    };

The conditional of this loop should be not f.eof(), i.e. "while we have not reached the end of the file". So I think you can go back to using feof (and update the test).

PhucVH888 · 2016-04-19T10:49:49Z

You are right. The very first name of the function is isOpen() and changing the name to eof() without adding not causes hidden bug. So, it's fair to use feof().

EliasC · 2016-04-19T13:14:25Z

bundles/prototype/FileIO/IO.enc

+  def readlineChar() : embed char* end {
+    let f = this.file;
+    embed (embed char* end)
+      char* line = encore_alloc(_ctx,255);      


@albertnetymk: Does this code run a risk of an outdated context? Consider if readline is called in a loop that also does get.

(@PhucVH888: this is related to a bug discovered by Albert yesterday, you shouldn't worry about having introduced it)

@EliasC : Does it solve the outdated context if the _ctx is refreshed before passing to encore_alloc()?

Yes, @albertnetymk's solution to these problems is calling encore_ctx() instead of using the (possibly stale) _ctx variable. I'm working on a PR to make sure that all embedded uses of encore_alloc refresh the context.

Then to be safe, let's change _ctx for a call to encore_ctx(), and then I think this is ready to be merged!

Does encore_ctx() return _ctx?

Should I change like this char* line = encore_alloc(encore_ctx(),255);
or this encore_ctx(); char* line = encore_alloc(_ctx,255);?

Because it works in both ways.

kikofernandez · 2016-04-20T06:31:02Z

I would do the following (since the evaluation of function arguments in C is not defined):

_ctx = encore_ctx();
char* line = encore_alloc(_ctx,255);

albertnetymk · 2016-04-20T08:18:46Z

I don't think the evaluation order matters here. Actually, the one-liner looks more compact. Subjective, sure.

kikofernandez · 2016-04-20T10:14:55Z

I read somewhere in stackoverflow that in C99, encore_alloc(encore_ctx(), 255) doesn't even guarantee that encore_ctx() gets evaluated before encore_alloc since this behavior is undefined.
I would rather be safe than sorry :)

supercooldave · 2016-04-20T10:56:24Z

@kikofernandez That's very surprising. How can a function call be evaluated before it's arguments ... except in Haskell.

albertnetymk · 2016-04-20T11:04:25Z

I am only aware that arguments evaluation order is undefined in C, but that the evaluation could happen after the function call is beyond me. Reference?

EliasC · 2016-04-20T11:05:29Z

@kikofernandez: I think only the order of evaluation is unspecified, so if you have foo(bar(), baz()) you cannot know which argument is evaluated first. You are however guaranteed that they are evaluated.

EDIT: What @albertnetymk said...

kikofernandez · 2016-04-20T11:08:11Z

Ok, maybe this is not the most reliable source but I understand something different from here: http://www.geeksforgeeks.org/g-fact-20/

EliasC · 2016-04-20T11:11:51Z

foo(x++, x++) is a different story! It has something to do with sequence points (and there not being one between the two instances of x++), but this doesn't apply to function calls.

EliasC · 2016-04-20T11:12:59Z

Here is a good text on the subject: http://stackoverflow.com/a/4176333

albertnetymk · 2016-04-20T11:15:15Z

For record purpose, I think inlining encore_ctx() in the function call is fine.

PhucVH888 · 2016-04-21T07:00:52Z

Let's try with inlining encore_ctx() in the function call.

kikofernandez · 2016-04-21T12:05:24Z

I think this is ready to be merged! Merging in 30 min unless there are objections (I will try to auto-squash to see how it works :) )

TobiasWrigstad added the ready for review label Apr 12, 2016

EliasC reviewed Apr 13, 2016
View reviewed changes

EliasC reviewed Apr 14, 2016
View reviewed changes

EliasC reviewed Apr 19, 2016
View reviewed changes

PhucVH888 added 7 commits April 21, 2016 14:03

Add FileIO library (read, write to file)

9cdf95e

Add args to init, rename methods

e925237

Remove the comment and correct the exit code.

103f84c

Revise eof() function

cec0144

Remove unused var in write() and writeChar(). Revise eof().

39c51ff

Revise eof() using feof(), and readline()

1616e0c

Refresh the context variable before calling encore_alloc()

5cc38cc

PhucVH888 force-pushed the file-io branch from a440f4a to 5cc38cc Compare April 21, 2016 12:03

kikofernandez merged commit 0e6fac7 into development Apr 21, 2016

kikofernandez removed the ready for review label Apr 21, 2016

kikofernandez deleted the file-io branch April 21, 2016 13:36

Add FileIO library (read, write to file) #374

Add FileIO library (read, write to file) #374

Conversation

PhucVH888 commented Apr 12, 2016

EliasC commented Apr 13, 2016

Choose a reason for hiding this comment

EliasC commented Apr 13, 2016

supercooldave commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliasC commented Apr 13, 2016

supercooldave commented Apr 13, 2016

PhucVH888 commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliasC commented Apr 13, 2016

EliasC commented Apr 13, 2016

albertnetymk commented Apr 13, 2016

Choose a reason for hiding this comment

PhucVH888 Apr 19, 2016 • edited Loading

Choose a reason for hiding this comment

EliasC commented Apr 19, 2016

PhucVH888 commented Apr 19, 2016

TobiasWrigstad commented Apr 19, 2016

supercooldave commented Apr 19, 2016

EliasC commented Apr 19, 2016

kikofernandez commented Apr 19, 2016

PhucVH888 commented Apr 19, 2016

TobiasWrigstad commented Apr 19, 2016 • edited Loading

PhucVH888 commented Apr 19, 2016

TobiasWrigstad commented Apr 19, 2016

EliasC Apr 19, 2016 • edited Loading

Choose a reason for hiding this comment

EliasC commented Apr 19, 2016

PhucVH888 commented Apr 19, 2016

EliasC commented Apr 19, 2016

PhucVH888 commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhucVH888 Apr 20, 2016 • edited Loading

Choose a reason for hiding this comment

kikofernandez commented Apr 20, 2016 • edited Loading

albertnetymk commented Apr 20, 2016

kikofernandez commented Apr 20, 2016

supercooldave commented Apr 20, 2016

albertnetymk commented Apr 20, 2016

EliasC commented Apr 20, 2016 • edited Loading

kikofernandez commented Apr 20, 2016

EliasC commented Apr 20, 2016

EliasC commented Apr 20, 2016

albertnetymk commented Apr 20, 2016

PhucVH888 commented Apr 21, 2016

kikofernandez commented Apr 21, 2016 • edited Loading

PhucVH888 Apr 19, 2016 •

edited

Loading

TobiasWrigstad commented Apr 19, 2016 •

edited

Loading

EliasC Apr 19, 2016 •

edited

Loading

PhucVH888 Apr 20, 2016 •

edited

Loading

kikofernandez commented Apr 20, 2016 •

edited

Loading

EliasC commented Apr 20, 2016 •

edited

Loading

kikofernandez commented Apr 21, 2016 •

edited

Loading