Remove ContainsKey where relevant#6282
Conversation
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
I have a few unrelated changes like in RoslynCodeTaskFactory, but I think it's all fairly straightforward. Also, some of my variable names are bad. If you have better ideas, please suggest them.
There was a problem hiding this comment.
This change technically means removing hostContext isn't atomic, but I don't think that's an issue.
There was a problem hiding this comment.
I would slightly prefer the actual logic to be on its own lines and not part the VerifyThrow() call. It makes it easier to read for me when error handling does not mutate the state.
src/Tasks/Copy.cs
Outdated
There was a problem hiding this comment.
Note the absence of !. Finding it in the dictionary means it cannot be added, so ContainsKey and TryAdd return opposite values here.
There was a problem hiding this comment.
It appears this prevented the creation of some directories where necessary, so I removed it.
There was a problem hiding this comment.
There are numerous cases in which we could eliminate another dictionary access via TryAdd or some similar method, but that isn't available on Framework, sadly.
p2 p3 p4 p5 P6 p7
Forgind
force-pushed
the
remove-contains
branch
from
f36801a to
928968c
Compare
|
|
||
| // Add any other targets specified by the user that were not already added | ||
| foreach (string targetName in _targetNames.Where(i => !traversalInstance.Targets.ContainsKey(i))) | ||
| foreach (string targetName in _targetNames.Except(traversalInstance.Targets.Keys, StringComparer.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Was the old code really doing case-insensitive comparison? traversalInstance.Targets looks case sensitive.
There was a problem hiding this comment.
I'd initially had it case-sensitive, and it failed in a way that made this look guilty, but I found a couple other bugs since then. Let me try again and get back to you.
There was a problem hiding this comment.
SolutionProjectGenerator_Tests.IllegalUserTargetNamesDoNotThrow(forceCaseDifference: True) was the test that failed. Looking at it, it explicitly changes the casing to be different and still expects it to work the same. I would assume that if we should do case-insensitive comparisons at one point, we should do it everywhere in SolutionProjectGenerator when looking at targets' names, though I'm open to being wrong.
src/Build/Evaluation/Evaluator.cs
Outdated
| // Defaults to false | ||
| _projectSupportsReturnsAttribute.TryGetValue(currentProjectOrImport, out NGen<bool> projectSupportsReturnsAttribute); | ||
|
|
||
| _projectSupportsReturnsAttribute[currentProjectOrImport] = projectSupportsReturnsAttribute | (target.Returns != null); |
There was a problem hiding this comment.
nit: Is the bit-wise OR intentional here? Would it work as well with || which is more natural for bool values?
There was a problem hiding this comment.
I was just maintaining how it was before, but I agree that's more natural. Thanks!
| // If we haven't seen this project before (by full path) then | ||
| // allocate a new key for it and save it away | ||
| if (!_projectKey.ContainsKey(e.ProjectFile)) | ||
| // allocate a new key for it and save it away. Otherwise, retrive it. |
| if (fusionNameToResolvedPath.ContainsKey(strongName)) | ||
| if (fusionNameToResolvedPath.TryGetValue(strongName, out string fusionName)) | ||
| { | ||
| fusionNameToResolvedPath.TryGetValue(strongName, out string fusionName); |
There was a problem hiding this comment.
I would slightly prefer the actual logic to be on its own lines and not part the VerifyThrow() call. It makes it easier to read for me when error handling does not mutate the state.
Forgind
added
the
merge-when-branch-open
3 participants
Each instance of ContainsKey that I deleted was followed by querying the same dictionary for the same key. These can be done at the same time, so I replaced them.
I do not expect that this will have any substantial user impact, but it theoretically improves perf, and it's a pet peeve of mine.