-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Scale back [Serializable] on CoreCLR types #11765
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,6 @@ namespace System.Diagnostics.Tracing | |
/// <summary> | ||
/// Exception that is thrown when an error occurs during EventSource operation. | ||
/// </summary> | ||
#if !ES_BUILD_PCL | ||
[Serializable] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think that this should stay, but under different ifdef. @brianrob ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that the EventSourceException is Serializable because we require all Exceptions to be serializable on the Desktop runtime to support Exceptions traveling across AppDomain boundaries. EventSource is unusual, in that at least at one time we tried to keep the source code the same on Desktop and CoreCLR (however because shiproom would wanted only changes that were in response to a particular request to change the desktop, the code bases are NOT in sync today). Still it may still be useful to keep open the option of a 'bulk update' (I think it is still true that we COULD simply copy the contents to the desktop and it would be 'correct'. Removing [Serializable] for the desktop build would break that. I looked and we don't have a #ifdef that means' Desktop. The closest thing we have is 'not coreCLR or Project N or not Portable' #if !CORECLR && !ES_BUILD_PN && !ES_BUILD_PCL This is my recommendation. Simply surround the [serialable] with this instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple of things to be aware of:
@dotnet-bot test Ubuntu x64 corefx_baseline There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I've added @vancem 's suggested ifdef along with !CORERT. @brianrob , I've been testing this against CoreFX and modified tests appropriately so that it works. That change was dotnet/corefx#20035. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. Thanks. |
||
#endif | ||
public class EventSourceException : Exception | ||
{ | ||
/// <summary> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is no longer applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed