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.

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

gh-106078: Move static objects related to CONTEXTVAR to the decimal module global state #106395

Merged
merged 12 commits into from
Jul 8, 2023

Conversation

CharlieZhao95
Copy link
Contributor

@CharlieZhao95 CharlieZhao95 commented Jul 4, 2023

Move following objects related to the CONTEXTVAR to decimal module global state.

  • tls_context_key
  • cached_context
  • current_context_var

@CharlieZhao95 CharlieZhao95 changed the title gh-106078: Move objectes related to the CONTEXTVAR to decimal module global state gh-106078: Move static objects related to CONTEXTVAR to the decimal module global state Jul 4, 2023
@CharlieZhao95
Copy link
Contributor Author

I executed the build and unit tests locally as follows. The result looks good.

# build
./configure --without-decimal-contextvar
make

# unittest
./python -m test test_decimal

cd ./Modules/_decimal/tests
./runall-memorydebugger.sh

BTW, do we have a CI pipeline that tests build with different compile options, such as --without-decimal-contextvar here. It seems that the current CI pipeline cannot detect build errors in the #ifndef WITH_DECIMAL_CONTEXTVAR block.

@CharlieZhao95 CharlieZhao95 marked this pull request as ready for review July 4, 2023 06:30
@CharlieZhao95
Copy link
Contributor Author

cc: @kumaraditya303 @erlend-aasland

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Instead of moving the entire struct, you can use forward declarations for the needed types.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@CharlieZhao95
Copy link
Contributor Author

Instead of moving the entire struct, you can use forward declarations for the needed types.

 This is an alias for an anonymous struct. We cannot forward declare anonymous structs or typedefs :(

A solution is to name these structs. For example:

struct PyDecContextObject;

typedef struct {
    ...
    struct PyDecContextObject *cached_context;
    ...
} decimal_state;

typedef struct PyDecContextObject {
    PyObject_HEAD
    mpd_context_t ctx;
    PyObject *traps;
    PyObject *flags;
    int capitals;
    PyThreadState *tstate;
} PyDecContextObject;

There is no harm and now you can forward declare it.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 4, 2023

It's no hinder that the structs are anonymous. C has solutions for that as well. Let me know if you need a hint.

Alternatively, add a name to them. There is no harm in that, and it is quite normal throughout the code base.

Examples:

  • Modules/cjkcodecs/multibytecodec.h
  • Include/pytypedefs.h

@CharlieZhao95
Copy link
Contributor Author

It's no hinder that the structs are anonymous. C has solutions for that as well. Let me know if you need a hint.

I don't know if your solution is to move the typedef struct {...} struct_name; to the top of the .c file or into the .h file. If not, please let me know!

Alternatively, add a name to them. There is no harm in that, and it is quite normal throughout the code base.

Thanks! I prefer to add names to these structures :)

@rhettinger rhettinger removed their request for review July 4, 2023 14:07
Modules/_decimal/_decimal.c Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
CharlieZhao95 and others added 5 commits July 6, 2023 10:00
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Tested locally with ./configure --without-decimal-contextvar. LGTM.

@kumaraditya303 kumaraditya303 merged commit 1c9e493 into python:main Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants