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

Fix internal methods using ReadOnlySpans as writable #104

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

TimonLukas
Copy link
Contributor

(fixes #103)

I've only changed the methods that read directly into ReadOnlySpans. It might be worth it to update the ones that read into WCharTStrings if the library is adapted as well - I'll take a look at the implementation in the coming days, it sounds really interesting! :)

The change is implemented as discussed, but my IDE (Jetbrains Rider) suggests using the object initializer syntax for setting the report ID as well. This would mean there are 3 possible implementations (using the example of GetFeatureReport again):

Option 1 (currently implemented):

public ReadOnlySpan<byte> GetFeatureReport(byte reportId, int maxLength)
{
    if (maxLength < 1)
        throw new ArgumentOutOfRangeException(nameof(maxLength), maxLength, "Please provide a value greater than 1");

    var data = new Span<byte>(new byte[maxLength]);
    data[0] = reportId;
    var result = NativeMethods.GetFeatureReport(handle, data);

    if (result == -1)
        HidException.Throw(handle);

    return data[..result];
}

Option 2 (with object initializer):

public ReadOnlySpan<byte> GetFeatureReport(byte reportId, int maxLength)
{
    if (maxLength < 1)
        throw new ArgumentOutOfRangeException(nameof(maxLength), maxLength, "Please provide a value greater than 1");

    var data = new Span<byte>(new byte[maxLength]) { [0] = reportId };
    var result = NativeMethods.GetFeatureReport(handle, data);

    if (result == -1)
        HidException.Throw(handle);

    return data[..result];
}

Option 3 (implicit conversion):

public ReadOnlySpan<byte> GetFeatureReport(byte reportId, int maxLength)
{
    if (maxLength < 1)
        throw new ArgumentOutOfRangeException(nameof(maxLength), maxLength, "Please provide a value greater than 1");

    Span<byte> data = new byte[maxLength];
    data[0] = reportId;
    var result = NativeMethods.GetFeatureReport(handle, data);

    if (result == -1)
        HidException.Throw(handle);

    return data[..result];
}

Which do you prefer? I think all options are readable, though 2 reads the worst IMO (and I'm not sure it's correctly formatted, it's what my IDE suggested).

@TimonLukas TimonLukas force-pushed the fix/mutable-readonly-spans branch from 5bf13c8 to 2552b55 Compare August 14, 2024 17:33
@badcel
Copy link
Owner

badcel commented Aug 14, 2024

I think avoiding implicit casts makes the code easier to grasp especially for newcomers (option 1).

Nevertheless I think if a developer knows about C# implicit conversions option 3 is easier to read as the types are more separated but it hides the constructor call.

I still tend to option 1 even if it is a bit harder to read for myself than option 3. What do you think?

Option 2 is too noisy for me. Do you know if there is some compiler optimization possible if object initializers are used?

@TimonLukas
Copy link
Contributor Author

TimonLukas commented Aug 14, 2024

Option 1 is totally fine! I'd also have gone with 1 or 3, maybe tending a little towards 3 - mostly because I mentally replace the Span<byte> with byte[]. But it really doesn't matter, all three seem to lead to the same compiled output. So, I'd say it's ready for review!

@badcel badcel merged commit 670e430 into badcel:main Aug 16, 2024
3 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.

Fix internal API writing to ReadOnlySpans
2 participants