Skip to content

Allow reflecting on DebuggableAttribute on CoreRT#1055

Merged
adamsitnik merged 2 commits into
dotnet:masterfrom
MichalStrehovsky:patch-1
Feb 8, 2019
Merged

Allow reflecting on DebuggableAttribute on CoreRT#1055
adamsitnik merged 2 commits into
dotnet:masterfrom
MichalStrehovsky:patch-1

Conversation

@MichalStrehovsky
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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