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

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

Closed
wants to merge 6 commits into from
Closed

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f7832f
bpo-35081: Remove Py_BUILD_CORE from datetime.h
vstinner Oct 30, 2018
1858252
Add #undef PyDateTime_TimeZone_UTC to C file
vstinner Oct 30, 2018
22abaee
Add a comment
vstinner Oct 30, 2018
8a11d06
PyDateTimeAPI variable must not be defined in _datetimemodule.c
vstinner Oct 31, 2018
a2750c6
Extend the scope of _PY_DATETIME_IMPL
vstinner Oct 31, 2018
69a455d
Update Include/datetime.h
pganssle Nov 1, 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
27 changes: 5 additions & 22 deletions Include/datetime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,26 +180,9 @@ typedef struct {
#define PyDateTime_CAPSULE_NAME "datetime.datetime_CAPI"


#ifdef Py_BUILD_CORE

/* Macros for type checking when building the Python core. */
#define PyDate_Check(op) PyObject_TypeCheck(op, &PyDateTime_DateType)
#define PyDate_CheckExact(op) (Py_TYPE(op) == &PyDateTime_DateType)

#define PyDateTime_Check(op) PyObject_TypeCheck(op, &PyDateTime_DateTimeType)
#define PyDateTime_CheckExact(op) (Py_TYPE(op) == &PyDateTime_DateTimeType)

#define PyTime_Check(op) PyObject_TypeCheck(op, &PyDateTime_TimeType)
#define PyTime_CheckExact(op) (Py_TYPE(op) == &PyDateTime_TimeType)

#define PyDelta_Check(op) PyObject_TypeCheck(op, &PyDateTime_DeltaType)
#define PyDelta_CheckExact(op) (Py_TYPE(op) == &PyDateTime_DeltaType)

#define PyTZInfo_Check(op) PyObject_TypeCheck(op, &PyDateTime_TZInfoType)
#define PyTZInfo_CheckExact(op) (Py_TYPE(op) == &PyDateTime_TZInfoType)

#else

/* When datetime.h is included from _datetimemodule.c,
the macros are defined in _datetimemodule.c. */
#ifndef _PY_DATETIME_IMPL
/* Define global variable for the C API and a macro for setting it. */
static PyDateTime_CAPI *PyDateTimeAPI = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

This variable will be created in every compilation unit that includes "datetime.h".

Copy link
Member

@pganssle pganssle Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Was that already a problem for the C API? this was a pre-existing part of the public API.

At the moment, within CPython, this is only included in _datetimemodule.c and _testcapimodule.c. The latter was, according to Victor, already being compiled without Py_BUILD_CORE.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it's a little bit strange to use static here. But my PR doesn't change that, and I don't see how to change the API to avoid it.

@serhiy-storchaka: Would you mind to open a new issue to track this issue?

Copy link
Member

@pganssle pganssle Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@vstinner I think your PR does change one thing, which is that this static variable is now created even when Py_BUILD_CORE is on, right? I don't think that there are any major negative consequences, though, and I don't see a way around it without either changing the public interface or re-introducing Py_BUILD_CORE to this file, which I gather was the aim of this PR.


Expand All @@ -224,6 +207,8 @@ static PyDateTime_CAPI *PyDateTimeAPI = NULL;

#define PyTZInfo_Check(op) PyObject_TypeCheck(op, PyDateTimeAPI->TZInfoType)
Copy link
Member

Choose a reason for hiding this comment

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

Why these macros are defined for non-core C API at all? PyDate_Check() and like are not defined.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what you mean, PyDate_Check is defined in this block, these are all part of the documented C API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand. In which case PyDate_Check() is not defined when you include datetime.h?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I was confused by changes in this PR. Now PyDate_Check is defined unconditionally, but PyTZInfo_Check only if _PY_DATETIME_IMPL is not defined, right? Then the question is why it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

_PY_DATETIME_IMPL is only used when datetime.h is included from _datetimemodule.c. In this case, PyDate_Check() and PyTZInfo_Check() are defined in _datetimemodule.c.

Previously, I always defined these macros, and then used #undef in _datetimemodule.c. But this approach doesn't work if tomorrow these macros are converted to static inline functions.

@serhiy-storchaka: do you prefer the #undef approach, and only exclude "static PyDateTime_CAPI *PyDateTimeAPI = NULL;" using _PY_DATETIME_IMPL?

Copy link
Member

@pganssle pganssle Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Before Victor's PR, all of these macros are defined here, twice - they have different definitions when built as part of CPython than when built as part of an extension module.

The reason they are different is that the datetime module exposes all of its functionality via a capsule (PyDateTimeAPI), but the datetime module itself doesn't need to use the capsule because it has direct access to the relevant symbols.

Victor's PR takes the "CPython only" builds and moves them out of datetime.h and into _datetimemodule.c, which is the only place they are (and can be) used. The public macros are under _PY_DATETIME_IMPL in this case so that he doesn't have to #undef them in _datetimemodules.c.

There's probably a case to be made in favor of dropping the macros defined in _datetimemodules.c entirely and having _datetimemodules.c just use the public macros, but I am not sure what the consequences there could be. I imagine a performance hit for starters (probably minor), and it would mean that _datetimemodules.c would need access to the PyDateTimeAPI singleton as well.

I think Victor's current proposed changes are an improvement over the current state of things and unless we missed something, should have no change in behavior for end users while making datetime easier to maintain. We can start with these changes and then do an analysis later of whether the code can be further improved by unifying the *_Check macros and having _datetimemodule.c generally use the public API where possible.

Copy link
Member

Choose a reason for hiding this comment

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

Also, hopefully I understood your comment correctly. If you meant that PyTZInfo_Check is different from the other ones, I don't think it is. PyTZInfo_Check is used in `_datetimemodule.c, so it has to be present in both the public and private APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would add that currently, including datetime.h in a C file which defines Py_BUILD_CORE doesn't work, since it uses types which are only exported by an external library... (_datetime is compared as a shared library by default on Linux)

#define PyTZInfo_CheckExact(op) (Py_TYPE(op) == PyDateTimeAPI->TZInfoType)
#endif /* !defined(_PY_DATETIME_IMPL) */


/* Macros for accessing constructors in a simplified fashion. */
#define PyDate_FromDate(year, month, day) \
Expand Down Expand Up @@ -264,8 +249,6 @@ static PyDateTime_CAPI *PyDateTimeAPI = NULL;
PyDateTimeAPI->Date_FromTimestamp( \
(PyObject*) (PyDateTimeAPI->DateType), args)

#endif /* Py_BUILD_CORE */

#ifdef __cplusplus
}
#endif
Expand Down
28 changes: 20 additions & 8 deletions Modules/_datetimemodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
* http://www.zope.org/Members/fdrake/DateTimeWiki/FrontPage
*/

/* When datetime.h is included from _datetimemodule.c,
the macros are defines in _datetimemodule.c. */
#define _PY_DATETIME_IMPL

#include "Python.h"
#include "datetime.h"
#include "structmember.h"

#include <time.h>
Expand All @@ -11,14 +16,21 @@
# include <winsock2.h> /* struct timeval */
#endif

/* Differentiate between building the core module and building extension
* modules.
*/
#ifndef Py_BUILD_CORE
#define Py_BUILD_CORE
#endif
#include "datetime.h"
#undef Py_BUILD_CORE
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes no difference in this case, but it's somewhat interesting that in the old version, Py_BUILD_CORE was un-defined regardless of whether it was originally defined.

I think this means that if Py_BUILD_CORE is defined when compiling _datetimemodules.c, it will now remain so for the rest of the file, as opposed to the pre-PR version, where it is unconditionally flipped off after importing datetime.h. I don't see any other references to it in the file, though, so it should be safe to remove this.

#define PyDate_Check(op) PyObject_TypeCheck(op, &PyDateTime_DateType)
#define PyDate_CheckExact(op) (Py_TYPE(op) == &PyDateTime_DateType)

#define PyDateTime_Check(op) PyObject_TypeCheck(op, &PyDateTime_DateTimeType)
#define PyDateTime_CheckExact(op) (Py_TYPE(op) == &PyDateTime_DateTimeType)

#define PyTime_Check(op) PyObject_TypeCheck(op, &PyDateTime_TimeType)
#define PyTime_CheckExact(op) (Py_TYPE(op) == &PyDateTime_TimeType)

#define PyDelta_Check(op) PyObject_TypeCheck(op, &PyDateTime_DeltaType)
#define PyDelta_CheckExact(op) (Py_TYPE(op) == &PyDateTime_DeltaType)

#define PyTZInfo_Check(op) PyObject_TypeCheck(op, &PyDateTime_TZInfoType)
#define PyTZInfo_CheckExact(op) (Py_TYPE(op) == &PyDateTime_TZInfoType)


/*[clinic input]
module datetime
Expand Down