Cleanup project system code#24570
Conversation
…ectReference This field is documented as being written and read from any thread, but in practice all uses are guarded by an AssertIsForeground(). Thus we can get rid of the helper methods that are trying to "help" by locking before accessing the fields, making it really hard to track all the real uses of it.
|
And @Pilchie or @jinujoseph, as code reviews happen and CI integrates continuously, does this even need an ask mode template? |
|
I should clarify: I wouldn't want this in master, as further perf work improvements in dev15.7.x would benefit from this. |
It turns out we had some tests, but the tests were disabled. This was because the tests weren't working properly anyways: they were calling into UpdateProjectBinPath which only updated some (but not all) of the project state. That was an internal helper method that shouldn't be used by tests. Updating the tests to use SetBinOutputPathAndRelatedData works better.
This was some legacy code that tried to verify that the references we have from the project system match up to what DTE and other sources say. This was debug-only, and the actual asserts were commented out. This is deadweight at this point, so delete it.
b7c2d5f to
6256f9a
Compare
| project2.SetBinOutputPathAndRelatedData(@"c:\project2.dll"); | ||
|
|
||
| // since this is known to be the output path of project1, the metadata reference is converted to a project reference | ||
| project2.OnImportAdded(@"c:\project1.dll", "project1"); |
There was a problem hiding this comment.
so we no longer treat p2p reference and metadata reference differently? any reference that points to same path to project becomes p2p reference?
There was a problem hiding this comment.
what happen if order is revered, reference is added first, and then project is added? will existing reference turns to p2p reference?
There was a problem hiding this comment.
@heejaechang Yes, we've been doing that for awhile now. This is just fixing the tests to actually test that.
And yes, either order works.
| Assert.Empty(project2.GetCurrentProjectReferences()); | ||
| Assert.Single(project2.GetCurrentMetadataReferences().Where(r => r.FilePath == @"c:\project1.dll")); | ||
|
|
||
| project2.Disconnect(); |
There was a problem hiding this comment.
here we probably want to create project1 again and add it back? and see whether projec2 gets that as p2p reference?
There was a problem hiding this comment.
I think that's already covered in AddingReferenceToProjectMetadataPromotesToProjectReference and AddingProjectReferenceAndUpdateReferenceBinPath tests effectively?
|
ah nevermind , this is targeted for 15.7 , we are good |
|
@jinujoseph Doesn't matter, we could put this into 15.6. From the unit test perspective this is touching, our code already worked fine. |
| // okay, two has different set of dlls referenced. check special Microsoft.VisualBasic case. | ||
| if (delta != 1) | ||
| { | ||
| //// Contract.Requires(false, "different set of references!!!"); |
There was a problem hiding this comment.
lol... debug only code... with commented out asserts...
Commit at-a-time review recommended: as I was investigating crash reports last week I did a little cleanup so I could actually understand what the code was doing. I also fixed up tests so they actually were testing the product correctly. There is no functional change here.
Since this is really mostly a test-changes-only my gut is the ask mode template doesn't apply here. (If it does, most of the questions don't make sense.)