Mailing List Archive: Python: Dev

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes.

Index | Next | Previous | View Threaded

benjamin at python

May 3, 2012, 10:07 PM

Post #1 of 7 (133 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes.

2012/5/3 larry.hastings <python-checkins [at] python>:
> diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
> --- a/Modules/posixmodule.c
> +++ b/Modules/posixmodule.c
> @@ -3572,28 +3572,194 @@
> #endif /* HAVE_UNAME */
>
>
> +static int
> +split_py_long_to_s_and_ns(PyObject *py_long, time_t *s, long *ns)
> +{
> + int result = 0;
> + PyObject *divmod;
> + divmod = PyNumber_Divmod(py_long, billion);
> + if (!divmod)
> + goto exit;
> + *s = _PyLong_AsTime_t(PyTuple_GET_ITEM(divmod, 0));
> + if ((*s == -1) && PyErr_Occurred())
> + goto exit;
> + *ns = PyLong_AsLong(PyTuple_GET_ITEM(divmod, 1));
> + if ((*s == -1) && PyErr_Occurred())
> + goto exit;
> +
> + result = 1;
> +exit:
> + Py_XDECREF(divmod);
> + return result;
> +}
> +
> +
> +typedef int (*parameter_converter_t)(PyObject *, void *);
> +
> +typedef struct {
> + /* input only */
> + char path_format;
> + parameter_converter_t converter;
> + char *function_name;
> + char *first_argument_name;
> + PyObject *args;
> + PyObject *kwargs;
> +
> + /* input/output */
> + PyObject **path;
> +
> + /* output only */
> + int now;
> + time_t atime_s;
> + long atime_ns;
> + time_t mtime_s;
> + long mtime_ns;
> +} utime_arguments;
> +
> +#define DECLARE_UA(ua, fname) \
> + utime_arguments ua; \
> + memset(&ua, 0, sizeof(ua)); \
> + ua.function_name = fname; \
> + ua.args = args; \
> + ua.kwargs = kwargs; \
> + ua.first_argument_name = "path"; \
> +
> +/* UA_TO_FILETIME doesn't declare atime and mtime for you */
> +#define UA_TO_FILETIME(ua, atime, mtime) \
> + time_t_to_FILE_TIME(ua.atime_s, ua.atime_ns, &atime); \
> + time_t_to_FILE_TIME(ua.mtime_s, ua.mtime_ns, &mtime)
> +
> +/* the rest of these macros declare the output variable for you */
> +#define UA_TO_TIMESPEC(ua, ts) \
> + struct timespec ts[2]; \
> + ts[0].tv_sec = ua.atime_s; \
> + ts[0].tv_nsec = ua.atime_ns; \
> + ts[1].tv_sec = ua.mtime_s; \
> + ts[1].tv_nsec = ua.mtime_ns
> +
> +#define UA_TO_TIMEVAL(ua, tv) \
> + struct timeval tv[2]; \
> + tv[0].tv_sec = ua.atime_s; \
> + tv[0].tv_usec = ua.atime_ns / 1000; \
> + tv[1].tv_sec = ua.mtime_s; \
> + tv[1].tv_usec = ua.mtime_ns / 1000
> +
> +#define UA_TO_UTIMBUF(ua, u) \
> + struct utimbuf u; \
> + utimbuf.actime = ua.atime_s; \
> + utimbuf.modtime = ua.mtime_s
> +
> +#define UA_TO_TIME_T(ua, timet) \
> + time_t timet[2]; \
> + timet[0] = ua.atime_s; \
> + timet[1] = ua.mtime_s
> +
> +
> +/*
> + * utime_read_time_arguments() processes arguments for the utime
> + * family of functions.
> + * returns zero on failure.
> + */
> +static int
> +utime_read_time_arguments(utime_arguments *ua)
> +{
> + PyObject *times = NULL;
> + PyObject *ns = NULL;
> + char format[24];
> + char *kwlist[4];
> + char **kw = kwlist;
> + int return_value;
> +
> + *kw++ = ua->first_argument_name;
> + *kw++ = "times";
> + *kw++ = "ns";
> + *kw = NULL;
> +
> + sprintf(format, "%c%s|O$O:%s",
> + ua->path_format,
> + ua->converter ? "&" : "",
> + ua->function_name);
> +
> + if (ua->converter)
> + return_value = PyArg_ParseTupleAndKeywords(ua->args, ua->kwargs,
> + format, kwlist, ua->converter, ua->path, &times, &ns);
> + else
> + return_value = PyArg_ParseTupleAndKeywords(ua->args, ua->kwargs,
> + format, kwlist, ua->path, &times, &ns);
> +
> + if (!return_value)
> + return 0;
> +
> + if (times && ns) {
> + PyErr_Format(PyExc_RuntimeError,

Why not a ValueError or TypeError?

> + "%s: you may specify either 'times'"
> + " or 'ns' but not both",
> + ua->function_name);
> + return 0;
> + }
> +
> + if (times && (times != Py_None)) {

Conditions in parenthesis like this is not style.

> + if (!PyTuple_CheckExact(times) || (PyTuple_Size(times) != 2)) {
> + PyErr_Format(PyExc_TypeError,
> + "%s: 'time' must be either"
> + " a valid tuple of two ints or None",
> + ua->function_name);
> + return 0;
> + }
> + ua->now = 0;
> + return (_PyTime_ObjectToTimespec(PyTuple_GET_ITEM(times, 0),
> + &(ua->atime_s), &(ua->atime_ns)) != -1)
> + && (_PyTime_ObjectToTimespec(PyTuple_GET_ITEM(times, 1),

Put && on previous line like Python.

> + &(ua->mtime_s), &(ua->mtime_ns)) != -1);
> + }
> +
> + if (ns) {
> + if (!PyTuple_CheckExact(ns) || (PyTuple_Size(ns) != 2)) {
> + PyErr_Format(PyExc_TypeError,
> + "%s: 'ns' must be a valid tuple of two ints",
> + ua->function_name);
> + return 0;
> + }
> + ua->now = 0;
> + return (split_py_long_to_s_and_ns(PyTuple_GET_ITEM(ns, 0),
> + &(ua->atime_s), &(ua->atime_ns)))
> + && (split_py_long_to_s_and_ns(PyTuple_GET_ITEM(ns, 1),
> + &(ua->mtime_s), &(ua->mtime_ns)));
> + }
> +
> + /* either times=None, or neither times nor ns was specified. use "now". */
> + ua->now = 1;
> + return 1;
> +}

--
Regards,
Benjamin
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

larry at hastings

May 3, 2012, 11:04 PM

Post #2 of 7 (119 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes. [In reply to]

On 05/03/2012 10:07 PM, Benjamin Peterson wrote:
>> + if (times&& ns) {
>> + PyErr_Format(PyExc_RuntimeError,
> Why not a ValueError or TypeError?

Well it's certainly not a TypeError. The 3.2 documentation defines
TypeError as:

Raised when an operation or function is applied to an object of
inappropriate type. The associated value is a string giving details
about the type mismatch.

If someone called os.utime with both times and ns, and the values of
each would have been legal if they'd been passed in in isolation, what
would be the type mismatch?

ValueError seems like a stretch. The 3.2 documentation defines
ValueError as

Raised when a built-in operation or function receives an argument
that has the right type but an inappropriate value, and the
situation is not described by a more precise exception such as
IndexError.

To me this describes a specific class of errors where a single value is
invalid in isolation, like an overly-long string for a path on Windows,
or a negative integer for some integer value that must always be 0 or
greater. The error with utime is a different sort of error; you are
passing in two presumably legal values, but the function requires that
you pass in at most one.

The only way I can see ValueError as being the right choice is from the
awkward perspective of "if you passed in times, then the only valid
value for ns is None" (or vice-versa).

Are there existing APIs that use ValueError for just this sort of
situation? I dimly recall there being something like this but I can't
recall it.

Is using RuntimeError some sort of Pythonic faux pas?

>> + if (times&& (times != Py_None)) {
> Conditions in parenthesis like this is not style.

Can you point me to where this is described in PEP 7? I can't find it.

>> + return (_PyTime_ObjectToTimespec(PyTuple_GET_ITEM(times, 0),
>> +&(ua->atime_s),&(ua->atime_ns)) != -1)
>> +&& (_PyTime_ObjectToTimespec(PyTuple_GET_ITEM(times, 1),
> Put&& on previous line like Python.

Okay.

Since I have questions regarding two of your three suggested changes,
I'll hold off on making any changes until the dust settles a little.

Finally, I appreciate the feedback, but... why post it to python-dev?
You could have sent me private email, or posted to the issue (#14127),
the latter of which would have enabled using rich chocolaty Rietveld.
I've seen a bunch of comments on checkins posted here and it all leaves
me scratching my head.

//arry/

greg.ewing at canterbury

May 3, 2012, 11:22 PM

Post #3 of 7 (112 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes. [In reply to]

Larry Hastings wrote:
>
> On 05/03/2012 10:07 PM, Benjamin Peterson wrote:
>
>>>+ if (times && ns) {
>>>+ PyErr_Format(PyExc_RuntimeError,
>>>
>>Why not a ValueError or TypeError?
>>
>
> Well it's certainly not a TypeError.

TypeError is not just for values of the wrong type,
it's also used for passing the wrong number of arguments
to a function and the like. So TypeError would be a
reasonable choice here, I think.

--
Greg
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

ncoghlan at gmail

May 3, 2012, 11:36 PM

Post #4 of 7 (121 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes. [In reply to]

On Fri, May 4, 2012 at 4:04 PM, Larry Hastings <larry [at] hastings> wrote:
> Finally, I appreciate the feedback, but... why post it to python-dev?� You
> could have sent me private email, or posted to the issue (#14127), the
> latter of which would have enabled using rich chocolaty Rietveld.� I've seen
> a bunch of comments on checkins posted here and it all leaves me scratching
> my head.

It's just the way post-checkin review is set up - the "Follow-up-to"
header for the python-checkins mailing list is python-dev. Such
comments are rare enough and the fact that they apply to already
committed code is important enough that there hasn't been a major push
to get the scheme changed to anything else (by contrast, the old
process where comments went back to python-checkins *was* problematic,
as they would get lost in the flow of actual checkin messages, hence
the switch to the current system).

Cheers,
Nick.

--
Nick Coghlan�� |�� ncoghlan [at] gmail�� |�� Brisbane, Australia
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

g.brandl at gmx

May 4, 2012, 12:01 AM

Post #5 of 7 (121 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes. [In reply to]

On 05/04/2012 08:04 AM, Larry Hastings wrote:
>
> On 05/03/2012 10:07 PM, Benjamin Peterson wrote:
>>> + if (times && ns) {
>>> + PyErr_Format(PyExc_RuntimeError,
>> Why not a ValueError or TypeError?
>
> Well it's certainly not a TypeError. The 3.2 documentation defines TypeError as:
>
> Raised when an operation or function is applied to an object of
> inappropriate type. The associated value is a string giving details about
> the type mismatch.
>
> If someone called os.utime with both times and ns, and the values of each would
> have been legal if they'd been passed in in isolation, what would be the type
> mismatch?

What exception do you get otherwise when you call a function with inappropriate
argument combinations?

> Is using RuntimeError some sort of Pythonic faux pas?

RuntimeError is not used very much in the stdlib, and if used, then for somewhat
more dramatic errors.

> Finally, I appreciate the feedback, but... why post it to python-dev? You could
> have sent me private email, or posted to the issue (#14127), the latter of which
> would have enabled using rich chocolaty Rietveld. I've seen a bunch of comments
> on checkins posted here and it all leaves me scratching my head.

It has been argued in the past that python-committers is a better place for the
review comments, but it was declined as being "not public enough". I agree that
python-checkins or private email *definitely* isn't public enough.

Georg

_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

stefan at bytereef

May 4, 2012, 2:33 AM

Post #6 of 7 (111 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes. [In reply to]

Larry Hastings <larry [at] hastings> wrote:
> On 05/03/2012 10:07 PM, Benjamin Peterson wrote:
>
> + if (times && ns) {
> + PyErr_Format(PyExc_RuntimeError,
>
> Why not a ValueError or TypeError?
>
>
> Well it's certainly not a TypeError. The 3.2 documentation defines TypeError
> as:
>
> Raised when an operation or function is applied to an object of
> inappropriate type. The associated value is a string giving details about
> the type mismatch.
>
> If someone called os.utime with both times and ns, and the values of each would
> have been legal if they'd been passed in in isolation, what would be the type
> mismatch?

I had the same question a while ago, and IIRC Raymond said that the convention
is to raise a TypeError if a combination of arguments cannot be handled by a
function.

In OCaml this would be quite natural:

$ ocaml
Objective Caml version 3.12.0
# type kwargs = TIMES | NS;;
type kwargs = TIMES | NS

let utime args =
match args with
| (_, TIMES) -> "Got times"
| (_, NS) -> "Got NS";;
val utime : 'a * kwargs -> string = <fun>

# utime ("/etc/passwd", TIMES);;
- : string = "Got times"
# utime ("/etc/passwd", NS);;
- : string = "Got NS"
# utime ("/etc/passwd", TIMES, NS);;
Error: This expression has type string * kwargs * kwargs
but an expression was expected of type 'a * kwargs

In Python it makes sense if (for the purpose of raising an error) one assumes
that {"times":(0, 0)}, {"ns":(0, 0)} and {"times":(0, 0), "ns":(0, 0)} have
different types.

Stefan Krah

_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

benjamin at python

May 4, 2012, 8:01 AM

Post #7 of 7 (111 views)
Permalink

Re: [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes. [In reply to]

2012/5/4 Larry Hastings <larry [at] hastings>:
> + if (times && (times != Py_None)) {
>
> Conditions in parenthesis like this is not style.
>
>
> Can you point me to where this is described in PEP 7? I can't find it.

It's not explicitly stated, but there is the following nice example:

if (type->tp_dictoffset != 0 && base->tp_dictoffset == 0 &&
type->tp_dictoffset == b_size &&
(size_t)t_size == b_size + sizeof(PyObject *))
return 0; /* "Forgive" adding a __dict__ only */

There's also the consistency with surrounding code imperative.

--
Regards,
Benjamin
_______________________________________________
Python-Dev mailing list
Python-Dev [at] python
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/list-python-dev%40lists.gossamer-threads.com

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads