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. |
|
Marcos Ramos (@mkswd) Thanks for your contribution! |
| 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.
Ilya (@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.
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.
There was a problem hiding this comment.
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.
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.
…hell#13804)" (PowerShell#14134) This reverts commit 42e7b66.
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,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.