Auto port System.Threading ACL comments #4147
Conversation
carlossanlop
added
new-content
|
I'll delegate this review to @kouvel |
There was a problem hiding this comment.
@carlossanlop much appreciated for the port. What sources were used for the port? I get a feeling that I'm comparing against a different source (I'm looking at .NET Framework 4.8 docs on docs.microsoft.com). At the moment I'm not sure that all of these changes are accurate, there are some significant differences, but generally even though most of the changes appear to be accurate I'm not sure now if the sources for the docs used were/are accurate of if I'm using the wrong sources to compare, etc.
| ]]></format> | ||
| </remarks> | ||
| <exception cref="T:System.ArgumentNullException">.NET Framework only: The <paramref name="name" /> length is beyond MAX_PATH (260 characters).</exception> | ||
| <exception cref="T:System.ArgumentOutOfRangeException">The <paramref name="mode" /> enum value was out of legal range.</exception> |
There was a problem hiding this comment.
Hmm I see an ArgumentException thrown by .NET Framework, ArgumentOutOfRangeException would make sense in this case but I'm wondering now if I'm seeing different docs on which this port was based (I'm currently looking at .NET Framework docs for the equivalent APIs).
There was a problem hiding this comment.
You're right, I should indicate another exception for Framework. I'll add it to the suggestion from the previous comment. https://referencesource.microsoft.com/#mscorlib/system/threading/eventwaithandle.cs,136
There was a problem hiding this comment.
Hmm there are many inconsistencies between in-code docs, tests, and .NET Framework docs. Filed dotnet/runtime#35519 to fix those, I think following the XML docs is good for this PR.
| </remarks> | ||
| <exception cref="T:System.ArgumentNullException">.NET Framework only: The <paramref name="name" /> length is beyond MAX_PATH (260 characters).</exception> | ||
| <exception cref="T:System.ArgumentOutOfRangeException">The <paramref name="mode" /> enum value was out of legal range.</exception> | ||
| <exception cref="T:System.IO.DirectoryNotFoundException">Could not find a part of the path specified in <paramref name="name" />.</exception> |
There was a problem hiding this comment.
I don't see this exception being raised by this API, the name parameter has nothing to do with directories on Windows, and on Unixes it's an implementation detail and probably should not result in such an exception
There was a problem hiding this comment.
Unit test verifying this exception added here: https://github.com/dotnet/runtime/blob/25d316cefafc4c3884f38acf7bdef971d2b9fe4b/src/libraries/System.Threading.AccessControl/tests/EventWaitHandleAclTests.cs#L70-L84
There was a problem hiding this comment.
I see thanks, I didn't know about that. If it can throw this exception for EventWaitHandle then it would throw in similar cases for the other types as well. This is fine for now, I'll look into fixing issues, filed issue dotnet/runtime#35518.
@kouvel These are the new types and methods I introduced in System.Threading back in November. They are not direct ports from Framework, there are some modifications. PRs and source files:
Were you looking at the old Framework code? The new code differs a bit. Would you mind taking a look again? |
Co-Authored-By: Koundinya Veluri <[email protected]>
Co-Authored-By: Koundinya Veluri <[email protected]>
Automatically ported the triple slash comments in the new ACL types and methods in System.Threading.
Ported with DocsPortingTool.