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

Enable CA1821, CA1825-CA1834 #33041

Merged
merged 3 commits into from
May 26, 2021
Merged

Enable CA1821, CA1825-CA1834 #33041

merged 3 commits into from
May 26, 2021

Conversation

pranavkm
Copy link
Contributor

Contributes to #24055

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 26, 2021
@pranavkm pranavkm changed the title Enable CA1821, CA1825, CA1826 Enable CA1821, CA1825-CA1834 May 26, 2021
@pranavkm pranavkm marked this pull request as ready for review May 26, 2021 18:25
@pranavkm pranavkm requested a review from JamesNK May 26, 2021 18:26
@@ -46,7 +46,7 @@ public static byte[] FromBase32(string input)
input = input.TrimEnd('=').ToUpperInvariant();
if (input.Length == 0)
{
return new byte[0];
return Array.Empty<byte>();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we even have this class?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hear you have to use base32 if you are on a 32-bit processor.

Copy link
Member

Choose a reason for hiding this comment

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

This was mostly just to get the finicky authenticator apps to work properly with TFA, its possible we don't need this helper anymore, but that's why it was added originally

@@ -210,7 +210,7 @@ private List<GCHandle> PinDataBuffers(bool endOfRequest, ArraySegment<byte> data
else if (!hasData && !addTrailers)
{
// No data
dataChunks = new HttpApiTypes.HTTP_DATA_CHUNK[0];
dataChunks = Array.Empty<HttpApiTypes.HTTP_DATA_CHUNK>();
Copy link
Member

@BrennanConroy BrennanConroy May 26, 2021

Choose a reason for hiding this comment

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

This will be pinned and passed to native code, are there any weird implications with using the global empty array in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tratcher, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably better to pin a singleton like this than an object in gen0. The pin for these is usually very short anyways.

@pranavkm pranavkm requested a review from a team as a code owner May 26, 2021 18:45
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

I did not review blazor/mvc/razor.

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Identity changes look fine, only random tidbit I have is I remember Diego back in the day told me to avoid using Any with EF because for whatever reason was way slower on than Count() > 0... but 🤷

@pranavkm pranavkm enabled auto-merge (squash) May 26, 2021 21:52
@dougbu
Copy link
Member

dougbu commented May 26, 2021

only random tidbit I have is I remember Diego back in the day told me to avoid using Any with EF because for whatever reason was way slower on than Count() > 0... but 🤷

@ajcvickers what are your thoughts❔

@pranavkm pranavkm merged commit e4f7bfb into main May 26, 2021
@pranavkm pranavkm deleted the prkrishn/more-fxcop branch May 26, 2021 23:05
@ghost ghost added this to the 6.0-preview6 milestone May 26, 2021
@ajcvickers
Copy link
Member

@dougbu Any should be fine.

@ghost
Copy link

ghost commented Jun 8, 2021

Hi @ajcvickers. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants