Skip to content

Fix nullable usage on authenticode (#13791) #13804

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.

Sign up for GitHub

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

Merged
Rob Holt (rjmholt) merged 2 commits into from
Oct 30, 2020
Merged

Fix nullable usage on authenticode (#13791) #13804

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
505d7e4
fixed nullable usage on authenticode
Oct 17, 2020
c80ec93
Update src/System.Management.Automation/security/Authenticode.cs
mkswd Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/System.Management.Automation/security/Authenticode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ internal static Signature GetSignature(string fileName, string fileContent)
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2001:AvoidCallingProblematicMethods")]
private static Signature GetSignatureFromCatalog(string filename)
{
if (Signature.CatalogApiAvailable.HasValue && !Signature.CatalogApiAvailable.Value)
if (Signature.CatalogApiAvailable.HasValue && !Signature.CatalogApiAvailable.GetValueOrDefault())

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ilya (@iSazonov) Why changing to GetValueOrDefault()? HasValue is already checked and thus it's fine to access .Value, not?

@iSazonov Ilya (iSazonov) Nov 17, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Dongbo Wang (@daxian-dbw) It is a recommendation from .Net team dotnet/runtime#33792 I opened #13791 for Hacktoberfest exclusively. It is not critical for us (no hot paths) but I hoped it is good for first time contributions.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the comment dotnet/runtime#33792 (comment) in that issue has a point. .Value is semantically more correct given that HasValue is already checked. There is a duplicate check on the boolean hasvalue field, but I don't think that affects the perf in practice.

For places where we can use GetValueOrDefault to replace both .HasValue and .Value, it makes sense to make the change, like in #13805 and #13808. But if we need to keep the .HasValue check, then I think using .Value afterwards is better.

@iSazonov Ilya (iSazonov) Nov 17, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree in common. Only there is another side - using different patterns decreases readability. I would prefer to use one pattern even though it looks unusual, especially since there are only a few of them in our code base. There is another reason why I prefer this. Now we often look at a code in .Net Runtime for a better understanding of how an application works and we consider this code as the best practice and high quality. Switching to a different code with a different style immediately causes discomfort. I believe that following .Net in this sense is the right direction - many of those who learn PowerShell code will be grateful to us.

{
// Signature.CatalogApiAvailable would be set to false the first time it is detected that
// WTGetSignatureInfo API does not exist on the platform, or if the API is not functional on the target platform.
Expand Down