Skip to content
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

Replace TryDequeue with Count == 0 check #3543

Closed
wants to merge 1 commit into from

Conversation

anders9ustafsson
Copy link

@anders9ustafsson anders9ustafsson commented Nov 10, 2022

This PR addresses a limitation in .NET Standard 2.0, where the method Queue.TryDequeue is not implemented. I have refactored the associated code to instead check if the queue is empty (Queue.Count == 0), which should mimic the behavior of the TryDequeue method.

This fix is only relevant in case it is decided to target .NET Standard 2.0, see discussion in #3503 .

@CADBIMDeveloper
Copy link
Contributor

Hi @anders9ustafsson , maybe it's better to check term.Count property instead of try-catch. Seems, it exists in .Net standard?

@anders9ustafsson
Copy link
Author

@CADBIMDeveloper Personally, I would say that the changes I propose in the PR are straightforward modifications of the original code, and thus preferable.

Howeer, if you are concerned with the usage of try-catch, don't hesitate to provide an alternative solution to the issue. Perhaps in a separate PR?

@CADBIMDeveloper
Copy link
Contributor

@anders9ustafsson I was just interested in motivation of using try-catch here. I thought I missed something about queue implementation in .Net and it was not safe to check Count property here for some reason I was not aware of.

@anders9ustafsson
Copy link
Author

@CADBIMDeveloper I will give some thought to your suggestion, you are probably right that it is a less intrusive way to use Queue.Count instead.

@anders9ustafsson anders9ustafsson changed the title For .NET Standard 2.0 compatibility, use Dequeue instead of TryDequeue Replace TryDequeue with Count == 0 check Nov 11, 2022
@anders9ustafsson
Copy link
Author

@CADBIMDeveloper In the end, I found your suggestion more appealing, so I have updated the pull request according to your recommendation. Many thanks!

@lperron
Copy link
Collaborator

lperron commented Jun 29, 2023

Closing the PR. We have a working solution

@lperron lperron closed this Jun 29, 2023
@Mizux Mizux self-requested a review June 29, 2023 17:21
@Mizux Mizux added Feature Request Missing Feature/Wrapper Lang: .NET .Net wrapper issue labels Jun 29, 2023
@Mizux Mizux added this to the v9.7 milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Missing Feature/Wrapper Lang: .NET .Net wrapper issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants