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

Avoid zero-length array allocations #19300

Merged
merged 2 commits into from
Jan 16, 2024
Merged

Conversation

molesmoke
Copy link
Contributor

Description of Change

Noticed while looking at another issue that CA1825 is configured as an error in .editorconfig, but there are various violations.
Pretty minor, but looks like it saves a reasonable number of allocations in some bindings scenarios at least. Thought the blanket fix-all should be reasonable since it should be a non-breaking change.

@molesmoke molesmoke requested a review from a team as a code owner December 7, 2023 21:43
@ghost ghost added the community ✨ Community Contribution label Dec 7, 2023
@ghost
Copy link

ghost commented Dec 7, 2023

Hey there @molesmoke! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@symbiogenesis
Copy link
Contributor

Maybe the CA1825 warning should be treated as an error

@jfversluis
Copy link
Member

Does this get rid of all the warnings? If yes, I would indeed say treat this as an error from now on? @hartez?

@molesmoke
Copy link
Contributor Author

@jfversluis Yup, this fixes all occurrences - or at least all that I'm aware of :). At the moment its enforcement is limited to Core, but seems reasonable to me that it should be solution-wide.

@molesmoke molesmoke force-pushed the array-empty branch 3 times, most recently from e739f7a to 3cc58c1 Compare December 10, 2023 23:21
@molesmoke
Copy link
Contributor Author

@jfversluis Seems like I did miss a few 😅 The iOS ones needed a bit of manual effort to fix up

@Eilon Eilon added the legacy-area-perf Startup / Runtime performance label Dec 13, 2023
@rmarinho
Copy link
Member

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho
Copy link
Member

/rebase

@rmarinho rmarinho requested review from hartez and removed request for StephaneDelcroix December 20, 2023 18:22
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Jan 8, 2024
@hartez
Copy link
Contributor

hartez commented Jan 10, 2024

Does this get rid of all the warnings? If yes, I would indeed say treat this as an error from now on? @hartez?

@jfversluis Yup, this fixes all occurrences - or at least all that I'm aware of :). At the moment its enforcement is limited to Core, but seems reasonable to me that it should be solution-wide.

We should enforce this in all of the projects eventually (we already do in Core). But if we're going to PR a change to fix all the rule violations in a project, we also need to turn on enforcement of the rule for that project (so no new violations creep in).

So at the very least, this PR needs to add editorconfig changes to enforce CA1825 for each other project. I would prefer we do these one project per PR (it makes for a much easier review), but I'd consider taking this if the editorconfig entries were there.

Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

Add missing editorconfig entries.

@molesmoke
Copy link
Contributor Author

@hartez The PR already moves dotnet_diagnostic.CA1825.severity = error to the root editor config:

aa84d7c

I put it in a separate commit in case the enforcement wasn't wanted (easier to cherry-pick/revert etc.), but I could squash it if you want? I'll rebase in the meantime...

@rmarinho
Copy link
Member

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nbaulesglobalsolutions
Copy link

nbaulesglobalsolutions commented Jan 12, 2024 via email

@hartez
Copy link
Contributor

hartez commented Jan 12, 2024

@hartez The PR already moves dotnet_diagnostic.CA1825.severity = error to the root editor config:

Sorry, my bad. Somehow I missed that you'd put it at the root.

@rmarinho rmarinho merged commit 5727faa into dotnet:main Jan 16, 2024
47 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2024
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community ✨ Community Contribution fixed-in-8.0.7 fixed-in-9.0.100-preview.1.9973 legacy-area-perf Startup / Runtime performance stale Indicates a stale issue/pr and will be closed soon t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants