Should it be implied that for DispIds, compile-time caching could be bad (not run-time caching)? #11809
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
Proposed change(s) ********************* 1) Last sentence of "Restrict using the dual interface option for the class interface." section should be: "... Such a description encourages late-bound clients to cache DispIds at compile time. ..." instead. Justifications for suggestion(s) ******************************** 1. Caching at compile time seems to be an issue because run time can take place a long time (maybe even years) after compile time, at which time, the DispId values may have changed. Caching at run time doesn't seem likely to be much of an issue so long as the cache is refreshed / created / re-created every time the associated type is changed (which seems should be the case in the vast majority of situations). The tone of the writing in question, appears likely to indicate that the caching is undesirable. Because of this, and what I've written earlier in this paragraph, it seems likely that the author meant to refer to compile-time caching & not run-time caching. 2. The original author meaning compile-time caching would make sense because the exported type library, available at compile time, contains the DispIds that are to be cached. See 'https://docs.microsoft.com/en-us/windows/desktop/midl/id' to help understand that the DispIds are available in the type library (which is available at compile time). 3. In the 'Avoid caching dispatch identifiers (DispIds)' section on the page, is written: "...To avoid breaking late-bound COM clients when using the class interface, apply the ClassInterfaceAttribute with the ClassInterfaceType.AutoDispatch value. This value implements a dispatch-only class interface, but omits the interface description from the type library. Without an interface description, clients are unable to cache DispIds at compile time. ..." This reference to caching DispIds at compile time, is quite likely what was meant to be referred to in the text I have suggested needs changing. In this reference above, inability to do such caching is suggested as being good in order to prevent '...breaking late-bound COM clients ...' My suggested change makes sense in connection with this, as with my suggestion applied, this 'goodness' assertion is re-iterated (although by inverting the language terms the second time). The second time, instead of saying not being able to do such caching is good, with the change, it is implied that being able to do such caching is bad.
ghost
mentioned this pull request
rpetrusha
added
the
✨ 1st-time docs contributor!
|
@AaronRobinsonMSFT and @jkoritzinsky, could you review this PR, please? |
|
Thank you for contributing to the dotnet/docs repo, @markfern. Just a note to make contributing to the repo less labor-intensive: if you're going to immediately open a PR to address an issue, it's not necessary to open a corresponding issue. |
|
Thanks for letting me know. Could be helpful to put this in the guidelines
- don't recall seeing it there.
Thanks,
Mark
…On Fri, 12 Apr 2019 16:57 Ron Petrusha, ***@***.***> wrote:
Thank you for contributing to the dotnet/docs repo, @markfern
<https://github.com/MarkFern>. Just a note to make contributing to the
repo less labor-intensive: if you're going to immediately open a PR to
address an issue, it's not necessary to open a corresponding issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11809 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AvPNKKrm2s1fL4aOfWuN8EI5fZ3o2_RYks5vgKz0gaJpZM4csM4v>
.
|
|
We'd intended to imply it in the contributor's guide with "Skip this step for small changes." But the implication isn't clear. I've opened #11818 to make it clearer. |
AaronRobinsonMSFT
approved these changes
Apr 13, 2019
|
Ah okay.
I just didn't bother reading the "Skip this step for small changes" text
which is the problem. If I had read it, I would have known I suppose (but I
was trying to save time which was the intention of the 'skip' text).
Perhaps make a step called "You don't need to create a new issue if...".
I think maybe sometimes it may be worth creating such issues as such issues
can sometimes appear as useful feedback on live documentation pages whereas
pull requests don't seem to ever appear on live documentation pages.
Thanks,
Mark
…On Fri, 12 Apr 2019 23:56 Ron Petrusha, ***@***.***> wrote:
We'd intended to imply it in the contributor's guide
<https://github.com/dotnet/docs/blob/master/CONTRIBUTING.md> with "Skip
this step for small changes." But the implication isn't clear. I'll open a
PR to make it clearer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11809 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AvPNKEg0zG8E0UKx7MxUB51QOnjiGW9Bks5vgQ8bgaJpZM4csM4v>
.
|
|
Closing and reopening to begin new build. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Proposed change(s)
Last sentence of "Restrict using the dual interface option for the class interface." section should be:
instead.
Justifications for suggestion(s):
Caching at compile time seems to be an issue because run time can take place a long time (maybe even years) after compile time, at which time, the DispId values may have changed. Caching at run time doesn't seem likely to be much of an issue so long as the cache is refreshed / created / re-created every time the associated type is changed (which seems should be the case in the vast majority of situations). The tone of the writing in question, appears likely to indicate that the caching is undesirable. Because of this, and what I've written earlier in this paragraph, it seems likely that the author meant to refer to compile-time caching & not run-time caching.
The original author meaning compile-time caching would make sense because the exported type library, available at compile time, contains the DispIds that are to be cached. See here to help understand that the DispIds are available in the type library (which is available at compile time).
In the 'Avoid caching dispatch identifiers (DispIds)' section on the page, is written:
This reference to caching DispIds at compile time, is quite likely what was meant to be referred to in the text I have suggested needs changing. In this reference above, inability to do such caching is suggested as being good in order to prevent '...breaking late-bound COM clients ...' My suggested change makes sense in connection with this, as with my suggestion applied, this 'goodness' assertion is re-iterated (although by inverting the language terms the second time). The second time, instead of saying not being able to do such caching is good, with the change, it is implied that being able to do such caching is possibly bad.
Fixes #11810