Rename parameters in native code to be more standardized#1513
Open
ds5678 wants to merge 1 commit into
Open
Conversation
ds5678
force-pushed
the
name-standardization
branch
from
68a3f22 to
337f6f9
Compare
ds5678
force-pushed
the
name-standardization
branch
3 times, most recently
from
8906ed4 to
78fe15b
Compare
Contributor
Author
|
@alinpahontu2912 is this fine to merge? |
ds5678
force-pushed
the
name-standardization
branch
2 times, most recently
from
a7523e5 to
392e21f
Compare
Contributor
Author
|
The CI appears to be hit-and-miss. |
ds5678
force-pushed
the
name-standardization
branch
from
392e21f to
83fdf1f
Compare
ds5678
force-pushed
the
name-standardization
branch
from
83fdf1f to
b1c83be
Compare
Member
|
Hey @ds5678, why do we need these changes? |
Contributor
Author
|
It would make my downstream code generation simpler if these parameter names in the header are changed, which I've done in this pull request. Anywhere there's a deviation from the naming scheme (which is rare), I have to write additional code downstream to rename. |
ds5678
force-pushed
the
name-standardization
branch
from
b1c83be to
96ac9c6
Compare
Contributor
Author
|
@alinpahontu2912 Can this be merged? |
ds5678
force-pushed
the
name-standardization
branch
2 times, most recently
from
9bf8c23 to
b40102e
Compare
ds5678
force-pushed
the
name-standardization
branch
from
b40102e to
714368f
Compare
alinpahontu2912
approved these changes
May 11, 2026
alinpahontu2912
force-pushed
the
name-standardization
branch
from
714368f to
146c2da
Compare
This aligns parameter naming for these functions with the rest of the project. See also: https://github.com/AssetRipper/AssetRipper.Bindings.LibTorchSharp/blob/a76753d26df195f667c003f8f10b3085882168a0/AssetRipper.Bindings.LibTorchSharp.SourceGenerator/ParameterNameChanges.cs#L7-L12
ds5678
force-pushed
the
name-standardization
branch
from
146c2da to
02f5905
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Standardizes native API parameter names in LibTorchSharp headers/implementations to align with existing project conventions (and downstream bindings/source generation expectations).
Changes:
- Renamed single-result allocator parameters from
allocator1→allocatorfor consistency. - Renamed first allocator parameters in paired-allocation APIs from
allocator→allocator1to match the existingallocator1/allocator2pattern. - Updated corresponding
.cppimplementations to match the updated header declarations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Native/LibTorchSharp/THSNN.h | Renames THSNN_Module_get_parameters allocator parameter to allocator (single allocator case). |
| src/Native/LibTorchSharp/THSModule.cpp | Updates THSNN_Module_get_parameters implementation signature and call site to use allocator. |
| src/Native/LibTorchSharp/THSJIT.h | Renames first allocator parameters to allocator1 where an allocator2 is also present. |
| src/Native/LibTorchSharp/THSJIT.cpp | Updates implementations to call allocator1 consistently with the header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This aligns parameter naming for these functions with the rest of the project.
See also: https://github.com/AssetRipper/AssetRipper.Bindings.LibTorchSharp/blob/a76753d26df195f667c003f8f10b3085882168a0/AssetRipper.Bindings.LibTorchSharp.SourceGenerator/ParameterNameChanges.cs#L7-L12