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.

Sign up for GitHub

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

Jump to bottom

Callsite fix for async methods #1138

Merged
merged 11 commits into from
Jan 17, 2016
Merged

Callsite fix for async methods #1138

merged 11 commits into from
Jan 17, 2016

Conversation

304NotModified
Copy link
Member

fixes #255

@codecov-io
Copy link

Current coverage is 73.47%

Merging #1138 into master will increase coverage by +0.15% as of 879aee9

@@            master   #1138   diff @@
======================================
  Files          264     265     +1
  Stmts        14944   15003    +59
  Branches      1636    1646    +10
  Methods          0       0       
======================================
+ Hit          10958   11024    +66
- Partial        417     418     +1
+ Missed        3569    3561     -8

Review entire Coverage Diff as of 879aee9

Uncovered Suggestions

  1. +0.09% via ...nternal/AspHelper.cs#206...218
  2. +0.09% via ...nternal/AspHelper.cs#188...200
  3. +0.09% via ...nternal/AspHelper.cs#170...182
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@304NotModified 304NotModified added bug Bug report / Bug fix has unittests labels Jan 8, 2016
bhaeussermann
bhaeussermann reviewed Jan 9, 2016
View reviewed changes
tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs
@@ -31,6 +31,9 @@
// THE POSSIBILITY OF SUCH DAMAGE.
//

using System.IO;
using System.Threading.Tasks;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must these using statements not go inside the namespace block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be more consistent yes. Resharper did this for me, but will fix it (cannot configure this in Resharper AFAIK)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed Visual Studio's "Quick Actions" adds the using inside the namespace (if there are no usings outside of it). Interesting.

@bhaeussermann
Copy link
Contributor

I don't find anything wrong with the logic of the code, but I made a few suggestions related to the code itself.

@304NotModified 304NotModified added this to the 4.3 milestone Jan 10, 2016
@304NotModified
Copy link
Member Author

@bhaeussermann Thanks for your suggestions. I applied them and renamed and documented a bit. Please check in short and if you think it's OK, merge it :)

bhaeussermann
bhaeussermann reviewed Jan 11, 2016
View reviewed changes
src/NLog/LoggerImpl.cs
if (next != null)
{
var declaringType = next.StackFrame.GetMethod().DeclaringType;
if (declaringType != null && declaringType == typeof(System.Runtime.CompilerServices.AsyncTaskMethodBuilder))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This null test is redundant now.

@304NotModified
Copy link
Member Author

it seems we have a deadlock in the master :(

@bhaeussermann any idea if this is fixed in another PR?

@bhaeussermann
Copy link
Contributor

@bhaeussermann any idea if this is fixed in another PR?

Not that I know of.
Is there an indication of which part of the build deadlocks?

@304NotModified
Copy link
Member Author

Well I was hoping #1126 would fix it.

@bhaeussermann
Copy link
Contributor

Well I was hoping #1126 would fix it.

#1126 does not fix any deadlocks that were already in master that I know of.

@304NotModified
Copy link
Member Author

Rebuild all. I don't think this one introduces a new deadlock. So if it al builds, it can be merged

304NotModified added a commit that referenced this pull request Jan 17, 2016
@304NotModified
Merge pull request #1138 from NLog/callsite-async
adbaa48 
Callsite fix for async methods
@304NotModified 304NotModified merged commit adbaa48 into master Jan 17, 2016
@304NotModified 304NotModified deleted the callsite-async branch February 3, 2016 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@304NotModified 304NotModified

Labels
bug Bug report / Bug fix
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
3 participants
@304NotModified @codecov-io @bhaeussermann