Add support for Newspeak's Files #190

daumayr · 2017-10-11T11:18:24Z

Adds support for file access similar to Newspeak, the major difference in functionality is the lack of streams.

FileDescriptor used to read/write files, a integrated buffer is used to store read data and data to write.
FilePath: represents a single file or directory and provides some operations (e.g. copy, move, size)
FilePattern: represents a set of files or directories in the form of a Path with wildcards (*, ?) and provides operations (e.g. pathsDo: , delete).

f:: FilePath for: ('/directory/file'). //create a filepath.
d:: f open: #readwrite ifFail: [:r|]. //returns file descriptor.
d buffer at: 1 put: 127.
d write: 1 at: 0 ifFail: [:r|]. //write 01111111 at the files beginning.

smarr · 2017-10-11T11:23:06Z

Thanks for the PR!

Haven't looked at the code yet. So, just some basic first questions:

The file gives us access to byte buffers, or what exactly is buffer? An integer array?
Any way to read stuff as a string?

Are you already using it for something?
Or do you plan to use it for something specific?

Would it be possible to use the provided changes to change TestRunner to pick up tests automatically? At the moment, we are hardcoding them, which is of course not exactly ideal.

daumayr · 2017-10-11T11:29:49Z

The buffer is a SOMns Array (long), reading stuff as a string is currently not supported, for that streams would be necessary.
Currently it is used in the Tests only, when expanding replay to deal with external communication I am going to make a benchmark that uses it.

I think it should be possible to pick up test automatically, streams would help

smarr

There are a couple of things, I'd prefer to see changed I think:

return values, self/this is often preferable to nil
block dispatch nodes would be better than hardcoded invokes
please remove dead code
tests better integrated with unit tests
and a few other things

smarr · 2017-10-11T11:24:56Z

tests/files/test.sh

@@ -0,0 +1,29 @@
+#!/bin/bash


Why did you chose to run those tests separately and not as unit tests with the other things in TestRunner?

hm, right, i didn't end up needing batch magic for cleanup

smarr · 2017-10-11T13:18:00Z

src/som/vmobjects/SFileDescriptor.java

+    } catch (FileNotFoundException e) {
+      fail.getMethod().invoke(new Object[] {fail, e.toString()});
+    }
+    /*


Please don't commit dead code

smarr · 2017-10-11T13:18:09Z

src/som/vmobjects/SFileDescriptor.java

+  private final File       f;
+
+  public static void setSOMClass(final SClass cls) {
+    // assert fileDescriptorClass == null || cls == null;


No dead code please

smarr · 2017-10-11T13:19:51Z

core-lib/Files.ns

@@ -0,0 +1,657 @@
+class Files usingVmMirror: vmMirror usingPlatform: platform = Value(
+(* This code was derived by converting the Strongtalk File classes to Newspeak, which is why the Sun Microsystems copyright and BSD license below applies.


Is this an unedited copy of the file?
If not, could you first have a commit with the original, and then the necessary changes?
Makes it much easier to track divergence.

Also, any chance you could improve the formatting of the file for human readers? (ideally in a separate commit)

The File API was split into multiple files and classes which I merged, so I don't think committing changes on top of the newspeak original would make sense. Also the formatting was worse. I am going to change that comment to say it's based on NewSpeak File classes.

Then please carefully attribute the source.
We need to be able to at least find the original files, ideally with an URL into the original repo for each, including version information.

smarr · 2017-10-11T13:20:15Z

core-lib/Files.ns

+  public simpleName ^ <String> = (
+    ^elements last name
+  )
+  (*


No dead code please (also in other places)

it's a public method accessible by users (which isn't used internally)

writeStream looks like it is in a comment, so, it's not accessible?

I misinterpreted the comment, only the (* of the comment was visible. I decided to keep the stream based methods as comments for the case that streams are implemented in the future. Those methods could be useful for reading text from files.

smarr · 2017-10-11T13:37:08Z

src/som/vmobjects/SFileDescriptor.java

+    try {
+      if (mode.getString().equals("read")) {
+        raf = new RandomAccessFile(f, "r");
+      } else {


Also, what does Newspeak do if you pass in #foobar? or nil, or whatever for mode? here we get crashes, or if it is any symbol, we get write access. Might be better to have an exception or call the failure block?

smarr · 2017-10-11T15:46:32Z

src/som/vmobjects/SFileDescriptor.java

+    open = true;
+  }
+
+  public void closeFile() {


Hm, you got java doc on the next method, but not here?

smarr · 2017-10-11T15:47:12Z

src/som/vmobjects/SFileDescriptor.java

+
+  public void write(final int nBytes, final long position, final SBlock fail) {
+    if (!open) {
+      fail.getMethod().invoke(new Object[] {fail, "File not open"});


block node please, also the other cases.

smarr · 2017-10-11T15:47:53Z

src/som/vmobjects/SFileDescriptor.java

+    byte[] buff = new byte[bufferSize];
+
+    for (int i = 0; i < bufferSize; i++) {
+      // punish users for using this loophole


ehm, no?! Throw a proper language level exception, please.

smarr · 2017-10-11T15:50:35Z

src/som/vmobjects/SFileDescriptor.java

+      raf.close();
+      open = false;
+    } catch (IOException e) {
+      e.printStackTrace();


A Smalltalk exception would be much nicer

Adds support for file access similar to Newspeak, the major difference in functionality is the lack of streams. - FileDescriptor used to read/write files, a integrated buffer is used to store read data and data to write. - FilePath: represents a single file or directory and provides some operations (e.g. copy, move, size) - FilePattern: represents a set of files or directories in the form of a Path with wildcards (*, ?) and provides operations (e.g. pathsDo: , delete). Example: ``` f:: FilePath for: ('/directory/file'). //create a filepath. d:: f open: #readwrite ifFail: [:r|]. //returns file descriptor. d buffer at: 1 put: 127. d write: 1 at: 0 ifFail: [:r|]. //write 01111111 at the files beginning. ``` Code based on: https://bitbucket.org/newspeaklanguage/newspeak/src/38a47c705f1a1ab3359f2a58b79e8c728bfb218f/Files.ns https://bitbucket.org/newspeaklanguage/newspeak/src/38a47c705f1a1ab3359f2a58b79e8c728bfb218f/Win32Files.ns

smarr · 2017-11-06T18:47:56Z

Ok, I did some more polishing, moving test files a bit around, fix formatting a little. Nothing much of consequence.
I squashed the changes, too, since the history didn't reflect anything useful.

@daumayr if you don't have any further things, I'll merge it.

smarr · 2017-11-06T18:48:40Z

@Richard-Roberts ah, perhaps interesting for you to have a brief look. This adds file support based on Newspeak. Might be useful for Grace.

daumayr · 2017-11-06T18:59:44Z

@smarr you can merge it, the broken tests are already broken in dev, i'll take a look at that separately.

ghost · 2017-11-06T19:05:28Z

Looks nice, thanks @daumayr. Early next year I hope to get our self-hosting Grace compiler running on SOMns, these changes will be useful for getting that going!

smarr requested changes Oct 11, 2017

View reviewed changes

Repository owner deleted a comment Oct 17, 2017

smarr force-pushed the FileIO branch from 9b139fd to 79ef3ef Compare November 5, 2017 18:21

smarr changed the title ~~Adapt Newspeak Files~~ Add support for Newspeak's Files Nov 6, 2017

smarr force-pushed the FileIO branch 2 times, most recently from e555f58 to c7fa31f Compare November 6, 2017 18:28

smarr force-pushed the FileIO branch from c7fa31f to 828bdc7 Compare November 6, 2017 18:42

smarr added the enhancement Improves the implementation with something noteworthy label Nov 6, 2017

smarr added this to the v0.6.0 - Black Diamonds milestone Nov 6, 2017

smarr merged commit bbd0841 into smarr:dev Nov 6, 2017

smarr deleted the FileIO branch November 6, 2017 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Newspeak's Files #190

Add support for Newspeak's Files #190

daumayr commented Oct 11, 2017 •

edited by smarr

Loading

smarr commented Oct 11, 2017 •

edited

Loading

daumayr commented Oct 11, 2017 •

edited

Loading

smarr left a comment

smarr Oct 11, 2017

daumayr Oct 16, 2017

smarr Oct 11, 2017

smarr Oct 11, 2017

smarr Oct 11, 2017

daumayr Oct 16, 2017

smarr Oct 16, 2017

smarr Oct 11, 2017

daumayr Oct 16, 2017

smarr Oct 16, 2017

daumayr Oct 16, 2017

smarr Oct 11, 2017

smarr Oct 11, 2017

smarr Oct 11, 2017

smarr Oct 11, 2017

smarr Oct 11, 2017

smarr commented Nov 6, 2017

smarr commented Nov 6, 2017

daumayr commented Nov 6, 2017

ghost commented Nov 6, 2017

		@@ -0,0 +1,657 @@
		class Files usingVmMirror: vmMirror usingPlatform: platform = Value(
		(* This code was derived by converting the Strongtalk File classes to Newspeak, which is why the Sun Microsystems copyright and BSD license below applies.

Add support for Newspeak's Files #190

Add support for Newspeak's Files #190

Conversation

daumayr commented Oct 11, 2017 • edited by smarr Loading

smarr commented Oct 11, 2017 • edited Loading

daumayr commented Oct 11, 2017 • edited Loading

smarr left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarr commented Nov 6, 2017

smarr commented Nov 6, 2017

daumayr commented Nov 6, 2017

ghost commented Nov 6, 2017

daumayr commented Oct 11, 2017 •

edited by smarr

Loading

smarr commented Oct 11, 2017 •

edited

Loading

daumayr commented Oct 11, 2017 •

edited

Loading