Skip to content

Make argsort const-correct#229

Merged
r-devulap merged 7 commits intonumpy:mainfrom
AnkitAhlawat7742:fix/argsort-const-correctness
Feb 17, 2026
Merged

Make argsort const-correct#229
r-devulap merged 7 commits intonumpy:mainfrom
AnkitAhlawat7742:fix/argsort-const-correctness

Conversation

@AnkitAhlawat7742
Copy link
Contributor

Fix: #223

Summary
argsort does not modify the input array, but its public and internal APIs previously accepted a non-const pointer. This PR makes argsort const-correct by updating the function signatures to take a const T*, improving API clarity and preventing accidental modification of input data.

Changes
Updated argsort signatures to accept const T* instead of T*

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR makes the argsort function const-correct by updating its signature to accept const T* instead of T* for the input array parameter, addressing issue #223. Since argsort only reads from the input array to compute sorted indices (it doesn't modify the array), this change improves API clarity and prevents accidental modification of input data.

Changes:

  • Updated argsort signature from T* to const T* across all public and internal APIs
  • Updated documentation in README.md to reflect the new const-correct signature

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/x86simdsort.h Updated public API declaration of argsort to accept const T*
lib/x86simdsort.cpp Updated DECLARE_INTERNAL_argsort macro to use const TYPE* for both function pointer and template implementation
lib/x86simdsort-skx.cpp Updated SKX-specific argsort implementation to accept const type*
lib/x86simdsort-scalar.h Updated scalar fallback implementation of argsort to accept const T*
lib/x86simdsort-internal.h Updated internal API declaration of argsort to accept const T*
lib/x86simdsort-avx2.cpp Updated AVX2-specific argsort implementation to accept const type*
README.md Updated documentation example to show const T* in argsort signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AnkitAhlawat7742 AnkitAhlawat7742 marked this pull request as draft February 11, 2026 15:54
@AnkitAhlawat7742 AnkitAhlawat7742 marked this pull request as ready for review February 11, 2026 18:29
@AnkitAhlawat7742
Copy link
Contributor Author

Hi @r-devulap ,
Please re-trigger the pipeline, I have resolved the conflicts and applied the SIMD type changes.

@r-devulap
Copy link
Member

@AnkitAhlawat7742 lint failures.

@AnkitAhlawat7742
Copy link
Contributor Author

@AnkitAhlawat7742 lint failures.

I’ll fix it right away.

@AnkitAhlawat7742
Copy link
Contributor Author

AnkitAhlawat7742 commented Feb 13, 2026

@r-devulap
Done...! the changes has been done to fix lint issue

@AnkitAhlawat7742
Copy link
Contributor Author

Hi @r-devulap ,
file src/xss-common-argsort.h, caused by line-ending inconsistencies (CRLF) and template alignment.

This commit normalizes line endings and applies clang-format to that file only, so that the repository now passes the CI formatting check cleanly.

No functional or logic changes are introduced.
Please trigger the CI CD pipeline

@AnkitAhlawat7742
Copy link
Contributor Author

@r-devulap — is it possible the CI is still running an older job?

I verified the formatting locally on the current branch head using:

find . -type f \( -name "*.c" -o -name "*.cpp" -o -name "*.h" -o -name "*.hpp" \) \
  -print0 | xargs -0 clang-format -style=file --dry-run -Werror
(py313_env) ankitahlawat@AnkitAhlawats-MacBook-Pro x86-simd-sort % git status
On branch fix/argsort-const-correctness
Your branch is up to date with 'origin/fix/argsort-const-correctness'.

nothing to commit, working tree clean

The working tree is clean and the branch fix/argsort-const-correctness is fully in sync with origin.

@r-devulap
Copy link
Member

r-devulap commented Feb 15, 2026

I am using: Ubuntu clang-format version 18.1.3 (1ubuntu1) anmd the command I use:

clang-format -i -style=file lib/*.h

@AnkitAhlawat7742
Copy link
Contributor Author

I am using: Ubuntu clang-format version 18.1.3 (1ubuntu1) anmd the command I use:

clang-format -i -style=file lib/*.h

Got it, thanks for pointing this out.

The earlier failures were due to formatting differences between macOS clang-format (v21) and the CI environment.
I’ve now verified and applied formatting using the same toolchain as CI (Ubuntu clang-format 18.1.3).
The current commit reflects the clang-format-18 output and matches the CI check exactly.

This should now pass the CI formatting step.

@AnkitAhlawat7742
Copy link
Contributor Author

I am using: Ubuntu clang-format version 18.1.3 (1ubuntu1) anmd the command I use:

clang-format -i -style=file lib/*.h

Hey @r-devulap ,

Got it, thanks for pointing this out.

The earlier failures were due to formatting differences between macOS clang-format (v21) and the CI environment. I’ve now verified and applied formatting using the same toolchain as CI (Ubuntu clang-format 18.1.3). The current commit reflects the clang-format-18 output and matches the CI check exactly.

This should now pass the CI formatting step.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@r-devulap r-devulap left a comment

Choose a reason for hiding this comment

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

LGTM. Do you want to update the argselect method as well? Might be good to complete that change in one patch.

@AnkitAhlawat7742
Copy link
Contributor Author

LGTM. Do you want to update the argselect method as well? Might be good to complete that change in one patch.

Thanks @r-devulap
To keep this PR scoped, I’ll address argselect in a follow-up PR and will raise it shortly.

@r-devulap r-devulap merged commit cb9f080 into numpy:main Feb 17, 2026
18 of 19 checks passed
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.

const data pointer in x86simdsort::argsort

3 participants