Skip to content
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

SETF STREAM-FILE-POSITION should return NEWVAL #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

phoe
Copy link

@phoe phoe commented Oct 3, 2017

Standard-compliant SETF functions should always return NEWVAL.

Standard-compliant SETF functions should always return NEWVAL.
@avodonosov
Copy link
Member

@phoe , thanks.

Can you provide a link where standard requires this?

@avodonosov
Copy link
Member

Also, I'm curious, how did you catch this? How does it manifest itself for a user?

@phoe
Copy link
Author

phoe commented Oct 4, 2017

Via CLHS 5.1.1.2:

Storing form
a form which can reference both the temporary and the store variables, and which changes the value of the place and guarantees to return as its values the values of the store variables, which are the correct values for setf to return.

Also, this is not the standard, but, via PCL:

SETF returns the newly assigned value, so you can also nest calls to SETF as in the following expression, which assigns both x and y the same random value:
(setf x (setf y (random 10)))

Also, no, this does not really manifest itself. I simply ended up looking through the code and noticing "oh, this SETF function does not retun NEWVAL". 😛

@avodonosov
Copy link
Member

avodonosov commented Aug 26, 2018

@phoe , the docs are quite complex, I'm reading CLHS but it's difficult to be 100% sure that returning newval is mandatory. The CLHS 5.1.1.2 you mention speaks about store form, but here we define a setf method. How can we be sure the requirement for the store form applies to setf method body?

Also, note, cl:file-position should return a generalized boolean, so we must return NIL if we don't really change the position.

An interesting note at LispWorks gray function:

The return value is returned by file-position . For the setf function, this is a slight anomaly because setf functions normally return the new value. However in this case it should return the success-p value mandated by the ANSI Common Lisp standard.

The default methods specialized on stream return nil .

http://www.lispworks.com/documentation/lw60/LW/html/lw-1239.htm

So, I'm yet unsure whether setf methods in general must return newval. If so, then using setf method as a way for user to specify implementation for (cl:file-position stream newval) for their streams is a bad choice. It then would be better for trivial-gray-streams to provide a single generic function (trivial-gray-stream:file-position stream &optional new-pos) which users should implement.

@phoe
Copy link
Author

phoe commented Aug 28, 2018

_death on #lisp found a better passage, http://www.lispworks.com/documentation/HyperSpec/Body/05_abi.htm

"A function named (setf f) must return its first argument as its only value in order to preserve the semantics of setf."

@avodonosov
Copy link
Member

@phoe, thank you. That's educating and convincing.

We can not just return newval from the default method, because it doesn't change position and so file-position should return false.

I think the proper fix will be to deprecate the (setf stream-file-position) and provide another way for stream class authors to implement (file-position stream N) for their streams. Without breaking those who already rely onto (setf stream-file-position).

@avodonosov
Copy link
Member

avodonosov commented Aug 31, 2018

@phoe , an interesting fact: Genera implementation of gray streams too was requiring users to provide file-position implementation for their streams as a (setf stream-file-psition) method:

(defmethod (setf gray-streams:stream-file-position)

(Although I don't know how authentic it is - was it a part of the original Genera or it's some modern fix.
Somebody just opened a pull request #7 saying it's support for Genera)

@avodonosov
Copy link
Member

I have this branch for a fix: https://github.com/trivial-gray-streams/trivial-gray-streams/compare/stream-file-position2?expand=1

But not sure is the theoretical clearness worth the disturbance in the library.

@phoe
Copy link
Author

phoe commented Aug 31, 2018

Yes, I can see what you mean. I'm not sure right now if that's what should be done - this part of the standard isn't clear and I have no idea if this issue shouldn't be altogether closed as "we're aware and won't change this behavior".

@avodonosov
Copy link
Member

avodonosov commented Oct 17, 2022

An idea how to prevent introducing a function stream-file-position-2 while keeping backwards compatibility - new package and ASDF system, trivial-gray-streams2, recommended for all new development. The old package trivial-gray-streams is deprecated and preserves the old interface.

The difference between the new and old package is only one symbol.
trivial-gray-stream2:stream-file-position uses the new interface, while trivial-gray-stream:stream-file-position is the old SETF'able function.

(But still not sure if this is the best approach and if this question deserves any changes at all).

@yitzchak
Copy link
Contributor

yitzchak commented Nov 16, 2023

I've been exploring a similar approach with a new ASDF system that would resolve this issue. It is a bit more extreme in that it completely removes the class hierarchy that trivial-gray-streams creates and does not attempt to make the interface for the various extensions to Gray streams identical for all implementations (except for trivial issues like the naming of generic functions). Instead, it provides feature keywords which identify support for the various extensions and capabilities.

One of the issues with the creation of a duplicate class hierarchy is that for the generic functions (such as stream-read-sequence) the native methods of the specific Lisp implementation will always take precedence over the trivial-gray-streams version if that implementation exported a class that implemented the Gray stream protocol. This is probably improbable, but seems a bit odd to me.

Thoughts on this approach would be appreciated. I'm not sure if I am going to add this system to quicklisp. We could also merge it with trivial-gray-streams to make a version 2. It is quite a significant departure in philosophy, though.

yitzchak/nontrivial-gray-streams

@avodonosov
Copy link
Member

@yitzchak, that's better to discuss in a separate ticket. I created one: #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants