More Optional Class Cleanup #2262
Conversation
InsertCreativityHere
changed the title
| @@ -29,7 +29,7 @@ function readPendingValues(obj) | |||
| end | |||
|
|
|||
| function readValue(obj, cb) | |||
| %assert(~isempty(cb)); | |||
There was a problem hiding this comment.
I'm not sure why we have this commented out check... but it's safe to uncomment it now.
Empty patch functions should only be supplied when skipping an optional class.
This is now no longer possible.
| function r = readOptional(~, ~, ~) | ||
| r = false; | ||
| end | ||
|
|
There was a problem hiding this comment.
This code was dead, so I removed it.
But, there's something fishy here.
In every other language, EncapsDecoder10 inherits this function as-is (since no 1.0 optional support) and EncapsDecoder11 overrides this function with a real implementation... Except in MATLAB, where we use a totally different code-path to reach the optional decoding implementation. I'm not sure why.
Co-authored-by: Jose <[email protected]>
| @@ -1622,43 +1622,6 @@ public void readValue(java.util.function.Consumer<Value> cb) { | |||
| readValue(cb, Value.class); | |||
| } | |||
There was a problem hiding this comment.
Please simplify the readValue above now that (presumably) the callback is never null.
| @@ -2595,7 +2595,7 @@ private void skipOptional(OptionalFormat format) | |||
| } | |||
| case OptionalFormat.Class: | |||
| { | |||
| throw new MarshalException("cannot unmarshal or skip an \"optional class\" parameter or field"); | |||
| throw new MarshalException("cannot skip an optional class"); | |||
There was a problem hiding this comment.
Please simplify readValue above. The callback is non-nullable now.
| @@ -1830,18 +1830,6 @@ class InputStream | |||
| }); | |||
| } | |||
|
|
|||
| readOptionalValue(tag, cb, T) | |||
There was a problem hiding this comment.
It's likely the cb in the readValue above can no longer be null.
| @@ -239,7 +239,7 @@ public class InputStream { | |||
| case .FSize: | |||
| try skip(read()) | |||
| case .Class: | |||
| try read(UnknownSlicedValue.self, cb: nil) | |||
| throw MarshalException(reason: "cannot skip an optional class") | |||
| @@ -29,7 +29,7 @@ function readPendingValues(obj) | |||
| end | |||
|
|
|||
| function readValue(obj, cb) | |||
| %assert(~isempty(cb)); | |||
Which function are you talking about specifically? For the main function that I touched: |
My mistake, the code is correct. |
This PR removes the public API for encoding/decoding classes from all the languages that still have it.
And changes our logic to throw an exception if we try to skip an optional class.
Note that it is still possible for users to send/receive optional classes if they use the lower-level stream API's, but we can only do so much here. A sufficiently motivated user will always find a way to send these.
It also brings all the tests back into alignment with one another, and removes the last place where we were manually sending/checking for optional classes using the stream APIs.
The error messages that were added to C++ and C# were different, so I standardized on
cannot skip optional class.This seemed the most consistent with our other error messages in the marshaling/unmarshaling logic:
cannot deserialize object,cannot unmarshal a proxy without a communicator,invalid Object slice,empty indirection table, etc. Feel free to comment alternatives.