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

[smart_holder] git merge master #5433

Merged
merged 4 commits into from
Nov 5, 2024
Merged

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Nov 5, 2024

Description

Helper/scratch PR for testing.

Suggested changelog entry:

francesco-ballarin and others added 4 commits October 12, 2024 11:19
* Incomplete attempt to address regression introduced in pybind#5381

* style: pre-commit fixes

* Revert "style: pre-commit fixes"

This reverts commit 9d107d2.

* Revert "Incomplete attempt to address regression introduced in pybind#5381"

This reverts commit 8cf1cdb.

* Simpler fix for the regression introduced in pybind#5381

* style: pre-commit fixes

* Added if constexpr workaround

This can probably be done better but at least this is a start.

* style: pre-commit fixes

* Replace if constexpr with template struct

if constexpr was not added until C++ 17.
I think this should do the same thing as before.

* style: pre-commit fixes

* Made comment clearer

* Added test cases

* style: pre-commit fixes

* Fixed is_same_or_base_of reference

* style: pre-commit fixes

* Added static assert messages

* style: pre-commit fixes

* Replaced typedef with using

* style: pre-commit fixes

* Back out `ForwardClassPtr` (to be discussed separately). Tested locally with clang-tidy.

* Shuffle new `static_assert()` and leave error messages blank (they are more distracting than helpful here).

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: gentlegiantJGC <[email protected]>
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>
* Fix buffer protocol implementation

According to the buffer protocol, `ndim` is a _required_ field [1], and
should always be set correctly. Additionally, `shape` should be set if
flags includes `PyBUF_ND` or higher [2]. The current implementation only
set those fields if flags was `PyBUF_STRIDES`.

[1] https://docs.python.org/3/c-api/buffer.html#request-independent-fields
[2] https://docs.python.org/3/c-api/buffer.html#shape-strides-suboffsets

* Apply suggestions from review

* Obey contiguity requests for buffer protocol

If a contiguous buffer is requested, and the underlying buffer isn't,
then that should raise. This matches NumPy behaviour if you do something
like:
```
struct.unpack_from('5d', np.arange(20.0)[::4])  # Raises for contiguity
```

Also, if a buffer is contiguous, then it can masquerade as a
less-complex buffer, either by dropping strides, or even pretending to
be 1D. This matches NumPy behaviour if you do something like:
```
a = np.full((3, 5), 30.0)
struct.unpack_from('15d', a)  # --> Produces 1D tuple from 2D buffer.
```

* Handle review comments

* Test buffer protocol against NumPy

* Also check PyBUF_FORMAT results
@rwgk rwgk merged commit 3b35ce4 into pybind:smart_holder Nov 5, 2024
82 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 5, 2024
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Nov 5, 2024
@rwgk rwgk deleted the sh_merge_master branch November 5, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants