Re: [Python-Dev] [Python-checkins] cpython: Issue #14127: Add ns= parameter to utime, futimes, and lutimes.
2012/5/3 larry.hastings
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, ×, &ns); + else + return_value = PyArg_ParseTupleAndKeywords(ua->args, ua->kwargs, + format, kwlist, ua->path, ×, &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
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/
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
On Fri, May 4, 2012 at 4:04 PM, Larry Hastings
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@gmail.com | Brisbane, Australia
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
Larry Hastings
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
2012/5/4 Larry 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
participants (7)
-
Antoine Pitrou -
Benjamin Peterson -
Georg Brandl -
Greg Ewing -
Larry Hastings -
Nick Coghlan -
Stefan Krah