Skip to content

C++/WinRT events don't gracefully handle invalid tokens#118

Merged
kennykerr merged 3 commits into
masterfrom
kennykerr-event
Jan 11, 2019
Merged

C++/WinRT events don't gracefully handle invalid tokens#118
kennykerr merged 3 commits into
masterfrom
kennykerr-event

Conversation

@kennykerr

Copy link
Copy Markdown
Contributor

The winrt::event<Delegate> implementation does not gracefully handle the case where its remove method is called with an invalid token value (a value not present in the array). This update brings the implementation in line with WRL, which handles this scenario.

Tests have been added to validate this scenario.

@JohnTasler

Copy link
Copy Markdown
    slim_mutex m_change;

Why is this needed? Isn't a swap within the scope of a "change"?


Refers to: src/tool/cpp/cppwinrt/strings/base_events.h:502 in 36d83b4. [](commit_id = 36d83b4, deletion_comment = False)

@kennykerr

Copy link
Copy Markdown
Contributor Author

@JohnTasler GitHub’s PR viewer is horrible. Unfortunately, if you leave a comment on a line of code outside of the PR then I cannot reply to it.

Briefly, the m_change lock ensures that add/remove are synchronized while m_swap only protects swapping the targets array. This improves concurrency because the latter happens more frequently and we don’t want it to block on the former.

@JohnTasler

Copy link
Copy Markdown
    }

Since you always access m_targets within a lock, it seems like you need one here. And, do you ever reset m_targets back to nullptr?


Refers to: src/tool/cpp/cppwinrt/strings/base_events.h:353 in 36d83b4. [](commit_id = 36d83b4, deletion_comment = False)

@JohnTasler

JohnTasler commented Jan 10, 2019

Copy link
Copy Markdown

Sorry. I wanted to grok and review the whole class. I'm using CodeFlow.

It's not a perfect integration, but works pretty well:
https://www.1eswiki.com/wiki/CodeFlow_integration_with_GitHub_Pull_Requests


In reply to: 453295948 [](ancestors = 453295948)

@Scottj1s Scottj1s left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@kennykerr kennykerr merged commit 84a5403 into master Jan 11, 2019
@kennykerr kennykerr deleted the kennykerr-event branch January 11, 2019 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants