Address PR feedback from #2579

[eerhardt]

We don't need a TryConvert method, we already have a TryParse, which can be used instead.

[stephentoub]

Note that TryParse is going to be slightly more expensive, but it's unlikely to be measurable, especially given everything else going on here.

[eerhardt]

slightly more expensive

Because of the extra write to the out parameter? Or because of the under/overflow check? Or something else?

[stephentoub]

The extra indirect write in the case of success, two extra indirect writes in the case of failure, the overflow check, the cast from the nullable rather than GetValueOrDefault, larger code that's even less likely to be inlined (though it's probably already not getting inlined). In general this code is not particularly clean.

[stephentoub]

This begs the question why the methods are named Convert and TryParse, rather than Convert and TryConvert, or Parse and TryParse. As long as you're cleaning things up, seems it should be consistent.

[codecov]

Codecov Report

Merging #2611 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
+ Coverage   71.53%   71.53%   +<.01%     
==========================================
  Files         800      800              
  Lines      141866   141857       -9     
  Branches    16122    16121       -1     
==========================================
- Hits       101478   101475       -3     
+ Misses      35937    35931       -6     
  Partials     4451     4451

Flag	Coverage Δ
#Debug	71.53% <0%> (ø)	⬆️
#production	67.81% <0%> (ø)	⬆️
#test	85.78% <ø> (+0.01%)	⬆️

[TomFinley]

Looks good thank you @eerhardt

[eerhardt]

@stephentoub - I've logged #2669 for your concerns on the existing TryParse code.