-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
ArgumentOutOfRangeException: use corresponding default message for all overloads #89846
Conversation
src/libraries/System.Private.CoreLib/src/System/ArgumentOutOfRangeException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/ArgumentOutOfRangeException.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
: this(paramName, null, message) {} | ||
|
||
public ArgumentOutOfRangeException(string? message, Exception? innerException) |
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.
This is missing the same treatment.
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.
This is consistent with other *Exception classes. I think this should be changed in another PR targeting all Exception derived classes
https://github.com/OwnageIsMagic/runtime/blob/3f624ff6fc8e347efde954744879c075efb1411b/src/libraries/System.Private.CoreLib/src/System/ArgumentException.cs#L44
https://github.com/OwnageIsMagic/runtime/blob/3f624ff6fc8e347efde954744879c075efb1411b/src/libraries/System.Private.CoreLib/src/System/MissingMethodException.cs#L30
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.
But it's inconsistent with itself. There's no reason a null message to the different ctors should use a different message. Either we change them all on this type or we don't change any on this type.
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.
What about other classes (string, Exception) ctor? Another PR?
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.
If there are other types where this would make sense, we can use this PR to do so.
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.
So there is no point constraining this to CoreLib.
This will affect all exception classes CoreLib (ls -1 **/*Exception.cs | wc -l # -> 100
) and some in other libs
i.e.
public DataException(string? s) : base(s) | |
{ | |
HResult = HResults.Data; | |
} | |
public DataException(string? s, Exception? innerException) : base(s, innerException) { } |
Lines 20 to 31 in 3b21b90
public IdentityNotMappedException(string? message) | |
: base(message) | |
{ | |
} | |
public IdentityNotMappedException(string? message, Exception? inner) | |
: base(message, inner) | |
{ | |
} | |
internal IdentityNotMappedException(string? message, IdentityReferenceCollection? unmappedIdentities) | |
: this(message) |
117 classes, but not every Exception out here have custom message
Are you ok with proposed pattern with constructor delegation?
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.
Are you ok with proposed pattern with constructor delegation?
I'd prefer we just add ?? SR.Whatever
where each message is passed along to the base.
Delegating from ctor to ctor unnecessarily keeps all larger constructors rooted and prevents them from being trimmed away when the smaller one is being used. It's also a lot more churn.
@stephentoub test timeout on arm seems unrelated |
@stephentoub ping ? |
Reverted original patch and rebased on fresh main #90505 |
Closes #89783