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

Allow reflecting on DebuggableAttribute on CoreRT #1055

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

MichalStrehovsky
Copy link
Member

See dotnet/corert#6955 (comment).

BenchmakeDotNet reflects on this attribute here:

private static bool? Read(DebuggableAttribute debuggableAttribute, string propertyName)
{
// the properties are implemented (https://github.com/dotnet/coreclr/pull/6153)
// but not exposed in corefx Contracts due to .NET Standard versioning problems (https://github.com/dotnet/corefx/issues/13717)
// so we need to use reflection to read this simple property...
var propertyInfo = typeof(DebuggableAttribute).GetProperty(propertyName);
if (debuggableAttribute == null || propertyInfo == null)
return null;
return (bool)propertyInfo.GetValue(debuggableAttribute);
}

@jkotas
Copy link
Member

jkotas commented Feb 7, 2019

Benchmark.NET is targeting .NET Standard 2.0: https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/BenchmarkDotNet.csproj#L5. These methods are available in .NET Standard 2.0.

Would it be better to just avoid the reflection altogether?

@adamsitnik
Copy link
Member

@MichalStrehovsky big thanks for the PR! I am glad I am not the only user of our CoreRtToolchain anymore!

@jkotas you are right, the comments in the code are from the dnx or netcoreapp1.0 era ;)

@adamsitnik
Copy link
Member

I am going to merge this now and fix the way we use this attribute.

I am going to also add one more integration test to make sure we support the latest version of CoreRT.

So far our tests were running against const version of CoreRT:

.UseCoreRtNuGet(microsoftDotNetILCompilerVersion: "1.0.0-alpha-26414-01") // we test against specific version to keep this test stable

@adamsitnik adamsitnik merged commit 190b9be into dotnet:master Feb 8, 2019
@AndreyAkinshin AndreyAkinshin added this to the v0.11.4 milestone Feb 8, 2019
@MichalStrehovsky MichalStrehovsky deleted the patch-1 branch February 8, 2019 08:40
@MichalStrehovsky
Copy link
Member Author

I am glad I am not the only user of our CoreRtToolchain anymore!

You should add telemetry. I'm sure there's many of us :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants