Support reading embedded source #16
Conversation
nguerrera
force-pushed
the
embed-source-in-pdb
branch
from
9761362
to
76c3617
Compare
nguerrera
changed the title
nguerrera
force-pushed
the
embed-source-in-pdb
branch
3 times, most recently
from
73964d7
to
7697b0c
Compare
nguerrera
changed the title
nguerrera
force-pushed
the
embed-source-in-pdb
branch
from
7697b0c
to
477dfe2
Compare
|
@tmat Ready for review. Updated to match spec change and added tests. |
| // https://github.com/dotnet/corefx/issues/8004 tracks adding that API to corefx and | ||
| // this should be updated to use that when it's fixed. In the meantime, we are forced | ||
| // to make an extra copy here. | ||
| Array.Copy(reader.ReadBytes(count), source, count); |
There was a problem hiding this comment.
Could we just add that one API now? Seems like making an extra copy here would be pretty significant hit.
There was a problem hiding this comment.
Sure. I'll send a PR.
There was a problem hiding this comment.
The feature is in corefx master, but corefx build is currently blocked.
nguerrera
force-pushed
the
embed-source-in-pdb
branch
from
477dfe2
to
05826c9
Compare
|
I have updated DSRN to the latest build. We can now add tests. |
nguerrera
force-pushed
the
embed-source-in-pdb
branch
from
05826c9
to
fad6211
Compare
|
@tmat I've updated to match latest Roslyn PR, but DSRN still doesn't have my changes. Tests are still asserting NativeNotYetImplemented(). :( |
nguerrera
force-pushed
the
embed-source-in-pdb
branch
from
2ee9308
to
95a9055
Compare
nguerrera
force-pushed
the
embed-source-in-pdb
branch
from
42d5a51
to
4e10c0d
Compare
|
@tmat false alarm (forgot to restore and got old DSRN). There is an issue in the native PDBs I'm generating so I'm backing that out from initial Roslyn PR. I've disabled the native test. |
| @@ -37,6 +37,9 @@ | |||
| <ItemGroup> | |||
| <Compile Include="PdbConverterTests.cs" /> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
| <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" /> | |||
| </ItemGroup> | |||
There was a problem hiding this comment.
Test explorer adds this. I'm tired of reverting it. OK to leave it?
|
@tmat Please review. Can we merge without the no-copy optimization (blocked on corefx build) and native support (pulling from Roslyn PR). |
|
|
||
| namespace Microsoft.DiaSymReader.PortablePdb.UnitTests | ||
| { | ||
| using System.Linq; |
There was a problem hiding this comment.
Are these intentionally here, or could they be move up?
There was a problem hiding this comment.
SymTestHelpers copied from elsewhere, causes ctrl + . to put more usings next to it. I'll move System.Linq.
|
👍 (a couple of minor suggestions) |
|
Applied all the suggestions. |
No description provided.