-
Notifications
You must be signed in to change notification settings - Fork 1.6k
2 Mistakes? In GetEndComSlot & GetStartComSlot? #2292
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Possible corrections are as follows: - In remarks section of documentation for Marshal.GetEndComSlot, the following possible correction is made: "... If the class interface is auto-dispatch, this method always returns -1 to indicate that the dispatch-only interface does not expose a v-table to managed clients. ..." - In parameter descriptions of documentation for Marshal.GetStartComSlot, the following possible correction is made: "<param name="t">A type that represents an interface or class.</param>" Justifications for suggestions: - Potential corrections align remarks section & parameter descriptions of the two methods: Marshal.GetEndComSlot & Marshal.GetStartComSlot. - I've written code where the parameter in question has been a class type for the Marshal.GetStartComSlot method, and it worked without problems. - The remarks section for Marshal.GetStartComSlot says that it returns a v-table number for an interface or a class. It logically follows that the one parameter that the static method accepts can be either an interface or a class. - Changing 'auto-dual' to 'auto-dispatch' seems to be correct when reading documentation at: https://docs.microsoft.com/en-us/dotnet/framework/interop/com-callable-wrapper#avoid-caching-dispatch-identifiers-dispids. - I've written code that runs Marshal.GetEndComSlot on an System.Collections.ArrayList type as well as on a System.Object type. The documentation for System.Object indicates that it is an auto-dual class ("[System.Runtime.InteropServices.ClassInterface(System.Runtime.InteropServices.ClassInterfaceType.AutoDual)]" is written in class definition). Running GetEndComSlot on this type does not return -1 which is what the previous documentation for the method indicated would be the case. Contrastedly, running the method on an ArrayList type returns -1. The documentation at 'https://docs.microsoft.com/en-us/previous-versions/dotnet/netframework-4.0/4fcadw4a(v=vs.100)#avoid-caching-dispatch-identifiers-dispids' indicates that if the ClassInterface attribute is not defined, its value should be the default auto-dispatch. The ArrayList class definition in the documentation doesn't define the attribute and so should be auto-dispatch (from my thinking). Therefore, according to the previous documentation for the GetEndComSlot, running the method on an ArrayList type probably should return a genuine positive-integer com slot, but as just said, it returns -1 (the number that is supposed to be returned by auto-dual classes according to previous documentation.) It seems the documentation for GetEndComSlot has got auto-dual mixed up with auto-dispatch. - The previous documentation for GetEndComSlot has the text: 'If the class interface is auto-dual, this method always returns -1 to indicate that the dispatch-only interface does not expose a v-table to managed clients.' I've suggested this text should be changed. The first part of the sentence refers to auto-dual classes, but the second part, which it seems is supposed to either also refer to auto-dual classes or an aspect of them, suddenly comments on dispatch-only interfaces which is not a proper reference to auto-dual classes or a direct aspect of them. Instead dispatch-only interfaces are connected to auto-dispatch classes which are not auto-dual classes - it looks like the first part of the sentence likely was supposed to refer to auto-dispatch classes instead of auto-dual classes.
@AaronRobinsonMSFT, @jkoritzinsky can one of you review the changes done here? |
Friendly ping @AaronRobinsonMSFT, @jkoritzinsky |
AaronRobinsonMSFT
approved these changes
Jun 8, 2019
mairaw
approved these changes
Aug 6, 2019
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.
Thank you @markfern. Sorry for the delay on merging this PR. I've removed my review comment and I'll apply it in a separate PR.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
✨ 1st-time dotnet-api-docs contributor!
Indicates PRs from new contributors to the dotnet-api-docs repository
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Possible corrections are as follows:
Justifications for suggestions