C#: Fix some new compiler warnings #18246
Conversation
michaelnebel
added
the
no-change-note-required
| if (chain is null || cert is null) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Should we log something in this case? Do we expect any of these to be null in any circumstance?
There was a problem hiding this comment.
Good question, I am not really familiar with this API. This was the minimal change I could make that just replaced a potential crash with a failed validation instead.
@mbg : Do you have an opinion about it?
There was a problem hiding this comment.
No opinion. I couldn't see anything obvious in the docs about when these might be null and they were never null during testing. I didn't implement any checks for whether these are null, since I figured that would be more helpful to know under what circumstances they end up being null than just return false.
There was a problem hiding this comment.
Ok, that indicates that we should add a log message in case these are null.
There was a problem hiding this comment.
Yep, I was just about to add that I guess logging it as @tamasvajk suggested would make sense.
michaelnebel
force-pushed
the
csharp/fixwarnings
branch
from
790c7d0 to
86c6df5
Compare
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs
Outdated
Show resolved
Hide resolved
A couple of new compiler warnings (since Friday)
DCA looks good.