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.

Sign up for GitHub

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

Jump to bottom

bpo-12743: Delete obsolete comment from marshalling docs #8457

Merged
merged 5 commits into from
Jul 27, 2018
Merged

bpo-12743: Delete obsolete comment from marshalling docs #8457

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b02d6ff
bpo-12743: Delete obsolete comment from marshalling docs
berkerpeksag Jul 25, 2018
f170eff
address review comments
berkerpeksag Jul 25, 2018
9bf1c72
address another review comment
berkerpeksag Jul 25, 2018
892f152
address another comment
berkerpeksag Jul 26, 2018
70bf977
document ValueError in remaining functions
berkerpeksag Jul 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
bpo-12743: Delete obsolete comment from marshalling docs
  • Loading branch information
@berkerpeksag
berkerpeksag committed Jul 25, 2018
commit b02d6ff69563e40005f43b832c06d2c6a0469171
12 changes: 4 additions & 8 deletions Doc/c-api/marshal.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,15 @@ unmarshalling. Version 2 uses a binary format for floating point numbers.

The following functions allow marshalled values to be read back in.

XXX What about error detection? It appears that reading past the end of the
file will always result in a negative numeric value (where that's relevant),
but it's not clear that negative values won't be handled properly when there's
no error. What's the right way to tell? Should only non-negative values be
written using these routines?


.. c:function:: long PyMarshal_ReadLongFromFile(FILE *file)

Return a C :c:type:`long` from the data stream in a :c:type:`FILE\*` opened
for reading. Only a 32-bit value can be read in using this function,
regardless of the native size of :c:type:`long`.

On error, raise an exception and return ``-1``.
On error, sets the appropriate exception (:exc:`EOFError` or
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValueError can't be raised in the current code. But it can raise MemoryError. It would be nice to make it raising also OSError and KeyboardInterrupt if appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I definitely missed MemoryError (thanks!), but isn't it possible to hit the following branch?

    if (read != n) {
        if (!PyErr_Occurred()) {
            if (read > n)
                PyErr_Format(PyExc_ValueError,
                             "read() returned too much data: "
                             "%zd bytes requested, %zd returned",
                             n, read);
            else
                PyErr_SetString(PyExc_EOFError,
                                "EOF read where not expected");
        }
        return NULL;
    }

It would be nice to make it raising also OSError and KeyboardInterrupt if appropriate.

Is it OK to leave that to another issue? (or is there an already open issue?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely missed MemoryError

It is missed also in lists of exceptions for PyMarshal_ReadObjectFromFile() etc.

but isn't it possible to hit the following branch?

It looks to me that this branch can be hit only when read from Python file-like object with broken readinto(). PyMarshal_ReadLongFromFile() reads from the C file, and fread() should return <= n, unless libc is is broken.

Is it OK to leave that to another issue? (or is there an already open issue?)

I'm working on this. It will be another issue.

I have doubts that it is worth to specify full lists of exceptions. They almost always will be incomplete.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will revert to "On error, raise an exception and return -1." in PyMarshal_ReadLongFromFile and PyMarshal_ReadShortFromFile docs.

Do you want me to remove the list of exceptions from the remaining functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no opinion. Providing a full list of exception is unusual, but in this case it may be possible. Look at the history of this documentation, maybe there were special reasons for listing exceptions.

Copy link
Member Author

@berkerpeksag berkerpeksag Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, there is not much in the commit logs. The docs for marshal functions were added in berkerpeksag@0fae49f and they didn't change much since 2001.

I think I'm going to keep EOFError and add a general note about MemoryError.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

:exc:`ValueError`) and returns ``-1``.


.. c:function:: int PyMarshal_ReadShortFromFile(FILE *file)
Expand All @@ -62,7 +57,8 @@ written using these routines?
for reading. Only a 16-bit value can be read in using this function,
regardless of the native size of :c:type:`short`.

On error, raise an exception and return ``-1``.
On error, sets the appropriate exception (:exc:`EOFError` or
:exc:`ValueError`) and returns ``-1``.


.. c:function:: PyObject* PyMarshal_ReadObjectFromFile(FILE *file)
Expand Down