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 nullable usage on authenticode (#13791) #13804
Conversation
Co-authored-by: Ilya <darpa@yandex.ru>
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@mkswd Thanks for your contribution! |
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iSazonov Why changing to GetValueOrDefault()
? HasValue
is already checked and thus it's fine to access .Value
, not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
PR Summary
Changed nullable usage for better implementation.
Change in Authenticode.cs
PR Context
Change has done due to issue #13791
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.