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

Do not ignore MemoryMarshal.TryWrite result #108661

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Oct 8, 2024

It is unidiomatic to ignore the return value of a Try method.

Supersedes #104728

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 8, 2024
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 8, 2024

@MihuBot

@stephentoub
Copy link
Member

Do not ignore MemoryMarshal.TryWrite result in Guid

What is the concern?

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 8, 2024

Do not ignore MemoryMarshal.TryWrite result in Guid

What is the concern?

Code style, it is unidiomatic to ignore the return value of a Try method.

@vcsjones
Copy link
Member

vcsjones commented Oct 8, 2024

All the Write is going to do is length checks, which is already known at the call site.

In S.S.Cryptography we do use Debug.Assert for calls like this so we don't "ignore" the return value, but release builds don't pay the penalty of doing a length check that is otherwise known to be safe. Something like

bool ret = MemoryMarshal.TryWrite(bytes, in this);
Debug.Assert(ret);

@vcsjones vcsjones added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 8, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@am11
Copy link
Member

am11 commented Oct 9, 2024

All the Write is going to do is length checks, which is already known at the call site.

Looks like both Write and TryWrite are same in terms of number of checks. (Try has one less throw statement)

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 9, 2024

@stephentoub It looks the use of MemoryMarshal.TryWrite was a workaround to improve codegen, but is no longer beneficial.

@stephentoub
Copy link
Member

Just doesn't seem worth the churn to me.

@xtqqczze xtqqczze changed the title Do not ignore MemoryMarshal.TryWrite result in Guid Do not ignore MemoryMarshal.TryWrite result Dec 14, 2024
@xtqqczze
Copy link
Contributor Author

@MihuBot

@xtqqczze
Copy link
Contributor Author

@stephentoub minimized churn and rebased

@xtqqczze
Copy link
Contributor Author

It looks the use of MemoryMarshal.TryWrite was a workaround to improve codegen, but is no longer beneficial.

Found 261 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 40203552
Total bytes of diff: 40203552
Total bytes of delta: 0 (0.00 % of base)


0 total files with Code Size differences (0 improved, 0 regressed), 259 unchanged.

0 total methods with Code Size differences (0 improved, 0 regressed), 232934 unchanged.

--------------------------------------------------------------------------------

@tannergooding
Copy link
Member

@stephentoub any concern with the new version that switches it to just use Write rather than TryWrite?

This is very minor code cleanup, and so if we don't want to take the churn here I'd be liable to just close it.

@stephentoub
Copy link
Member

I'm fine either way.

@tannergooding
Copy link
Member

I'll take this then since it does remove the ignored result and makes the code a little bit cleaner.

@tannergooding tannergooding merged commit bc2ac2f into dotnet:main Jan 10, 2025
139 checks passed
@xtqqczze xtqqczze deleted the GuidTryWrite branch January 10, 2025 20:54
grendello added a commit to grendello/runtime that referenced this pull request Jan 13, 2025
* main:
  JIT: Model GT_RETURN kills with contained operand (dotnet#111230)
  Update dependencies from https://github.com/dotnet/runtime-assets build 20250110.2 (dotnet#111290)
  [NativeAOT/ARM64] Generate frames compatible with Apple compact unwinding (dotnet#107766)
  Cleanup unused JIT stubs in vm (dotnet#111237)
  Ensure that Shuffle is marked as HW_Flag_CanBenefitFromConstantProp (dotnet#111303)
  Fix CMP0173 policy warning with cmake 3.31 (dotnet#110522)
  [RISC-V] Fix HostActivation.Tests unknown-rid (dotnet#110687)
  Fix accidentally duplicated global-build-step.yml in runtime-official.yml (dotnet#111302)
  JIT: run extra SPMI queries for arrays (dotnet#111293)
  Split the Runtime Shared Framework project and combine legs in the official build (dotnet#111136)
  Do not ignore `MemoryMarshal.TryWrite` result (dotnet#108661)
  Update dependencies from https://github.com/dotnet/emsdk build 20250109.1 (dotnet#111263)
  Clean up in Number.Formatting.cs (dotnet#110955)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants