Simplify verify throw and remove unnecessary usings #6999
Conversation
These statements only do anything if the condition is false, but they evaluate their arguments either way. These do nontrivial work when evaluating their arguments, so figure out if we should skip it early. This is specifically tuned to BoolEvaluate (part of evaluating conditions)
Slightly reduce other work done
Rather than checking whether false is false.
| /* helpfully display unexpanded token and expanded result in error message */ | ||
| isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), | ||
| isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); | ||
| if ((!isLeftNum && !isLeftVersion) || (!isRightNum && !isRightVersion)) |
There was a problem hiding this comment.
💗 simple change yet the code is FAR more readable
There was a problem hiding this comment.
Not to mention performant! This prevents us from randomly expanding values when we don't expect to need them.
| else if (actualSolutionFiles.Count > 1) | ||
| { | ||
| InitializationException.VerifyThrow(false, projectDirectory == null ? "AmbiguousProjectError" : "AmbiguousProjectDirectoryError", null, projectDirectory); | ||
| InitializationException.VerifyThrow(false, projectDirectory == null ? "AmbiguousProjectError" : "AmbiguousProjectDirectoryError", null, projectDirectory, false); |
There was a problem hiding this comment.
Nit: Since every other VerifyThrow(false,... was turned into Throw(..., should we make an overload for this?
There was a problem hiding this comment.
There kinda already is one:
InitializationException.Throw(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword(projectDirectory == null ? "AmbiguousProjectError" : "AmbiguousProjectDirectoryError", projectDirectory), null);
Do you like that better? I don't really care either way. The VerifyThrow --> Throw change should have virtually no user impact.
Forgind
added
the
merge-when-branch-open
4 participants
This has a little random cleanup plus moving the condition outside VerifyThrow calls if simply calling the method would do a nontrivial amount of work. The fourth commit switches VerifyThrow* to Throw* as appropriate.
I haven't measured the performance impact. The first and fourth commits should have ~0 impact, though technically the fourth commit makes it slightly faster in the failure case (by one branch). The third helps us avoid a couple small allocations. The second lets us avoid a nontrivial amount of work in an extremely common case, so if this has a positive perf impact, I would suspect that commit.
It should be easiest to look at this commit-by-commit.