Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FontEditor tests, fix bugs and debug asserts #760

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@hughbe
Copy link
Contributor

hughbe commented Apr 13, 2019

Fixes #761

@hughbe hughbe requested a review from dotnet/dotnet-winforms as a code owner Apr 13, 2019

{
this.value = fontDialog.Font;
return _fontDialog.Font;

This comment has been minimized.

Copy link
@zsd4yr

zsd4yr Apr 15, 2019

Member

Won't this prevent the finally from occurring?

This comment has been minimized.

Copy link
@zsd4yr

zsd4yr Apr 16, 2019

Member

I see https://stackoverflow.com/questions/421797/what-really-happens-in-a-try-return-x-finally-x-null-statement

I am trying to weigh the pros and cons here. On one hand:

  • this return adds more clarity be removing the need to use an additional variable

However, on the other hand:

  • this return adds confusion because the underlying IL and assembly will create a local variable, so this code may not do what a programmer expects it to do (for example, it was not straight-forward to me)

Thoughts?

@jaredpar do you have any opinions about c# code cleanliness versus eventual IL/assembly code clarity here?

This comment has been minimized.

Copy link
@hughbe

hughbe Apr 16, 2019

Author Contributor

I don't see this as a problem - essentially a using statement is the same/similar to a try/finally block and we don't have problems returning from inside usings! TBH I don't know much about compiler internals, so am very read to be proved wrong!

(although it strikes me any concerns about variable allocation would be slightly premature optimization)

This comment has been minimized.

Copy link
@JuditRose

JuditRose Apr 17, 2019

Member

I agree with @hughbe in this question, by definition finally block will run after try.
"The transfer of control can occur as a result of normal execution, of execution of a break, continue, goto, or return statement, or of propagation of an exception out of the try statement." (https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/try-finally)

This comment has been minimized.

Copy link
@zsd4yr

zsd4yr Apr 19, 2019

Member

I understand that the finally will always run; I am arguing that it is not clear that any code will run post return within the same scope.

@hughbe when you return inside a using statement, the method ends. Dispose will be called, but no code internal to that function's scope will be run.

I don't like it.

This comment has been minimized.

Copy link
@hughbe

hughbe Apr 21, 2019

Author Contributor

IMO I don't see putting returns inside try/finally blocks as a problem - it just is a common code pattern I've seen across various dotnet repos

This comment has been minimized.

Copy link
@RussKie

RussKie Apr 22, 2019

Contributor

Agree, any developer worth his/her salt is expected to understand this. Especially working on a framework repo.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #760 into master will increase coverage by 0.94156%.
The diff coverage is 68.75%.

@@                 Coverage Diff                 @@
##              master        #760         +/-   ##
===================================================
+ Coverage   28.05148%   28.99305%   +0.94157%     
===================================================
  Files           1065        1078         +13     
  Lines         292530      313620      +21090     
  Branches       38519       44460       +5941     
===================================================
+ Hits           82059       90928       +8869     
- Misses        206414      218341      +11927     
- Partials        4057        4351        +294
Flag Coverage Δ
#Debug 28.99305% <68.75%> (+0.94156%) ⬆️
#production 19.59199% <0%> (+1.2263%) ⬆️
#test 98.74889% <100%> (+0.08093%) ⬆️

@hughbe hughbe force-pushed the hughbe:fonteditor-tests branch from ae5530d to 79ed2f4 Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.