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

EnC - Update CustomAttributes table instead of just always adding to it #53507

Merged
merged 37 commits into from
Jun 16, 2021

Conversation

davidwengier
Copy link
Contributor

Fixes #52816

This doesn't do anything for deletes yet, need to decide if we will, and how. Previously we didn't care about changes that were only visible through reflection, but now that reflection caches are cleared that maybe isn't fair. Though re-orders then probably need to be changed too? Need to discuss.

@davidwengier
Copy link
Contributor Author

@tmat given dotnet/runtime#53066 I wonder if this change in behaviour should be based on the runtime capabilities, so that downlevel frameworks aren't affected? I don't think its an enourmous issue since the issue already exists in those frameworks, so its probably not causing issues due to reflection caching, but the most cautious approach would be to only change behaviour for .NET 6 and up.

@davidwengier
Copy link
Contributor Author

Apologies @tmat that you had to read all of that, the new code with binary search is much simpler, much easier to understand, and was much easier to write.

code: EditAndContinueOperation.Default);
}

void AddLogEntryOrDelete(int rowId, EntityHandle parent, bool add)
Copy link
Member

Choose a reason for hiding this comment

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

add

Isn't this rather "update" then "add"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming on this one was hard. It either adds an EncLog entry, or it creates a "delete" custom attribute and adds it to the list for later emitting.

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@davidwengier
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@davidwengier
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@davidwengier davidwengier enabled auto-merge (squash) June 16, 2021 08:08
@davidwengier davidwengier merged commit 9b0752c into dotnet:main Jun 16, 2021
@ghost ghost added this to the Next milestone Jun 16, 2021
@davidwengier davidwengier deleted the EnCCustomAttributes branch June 16, 2021 19:41
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnC update on method with custom attribute duplicates the attribute
5 participants