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

zstring_span: fix for Expects, simplify functions #850

Merged
merged 1 commit into from Aug 6, 2020

Conversation

beinhaerter
Copy link
Contributor

Fixes for a few issues recognized while answering https://stackoverflow.com/questions/56874532/using-gslzstring-view-with-c-apis.

  • s[s.size() - 1] is wrong for empty s, so Expects(s.size() > 0)
  • no hard coded '\0'but value_type{}
  • hard code empty() to return false
  • simplify as_string_span: can never be empty
  • clarify comment on ensure_z

@JordanMaples
Copy link
Contributor

Maintainers' call: The change looks good but would you mind rebasing this so I can merge it?

@JordanMaples JordanMaples added this to the 2020Q3 milestone Aug 5, 2020
@JordanMaples JordanMaples self-assigned this Aug 5, 2020
- `s[s.size() - 1]` is wrong for empty `s`, so `Expects(s.size() > 0)`
- no hard coded `'\0'`but `value_type{}`
- hard code `empty()` to return `false`
- simplify `as_string_span`: can never be `empty`
- clarify comment on `ensure_z`
@beinhaerter
Copy link
Contributor Author

@JordanMaples Did a rebase. When looking again at the code I am am (again) scratching my head. I did not change the result of empty, but is empty exactly what it should be? When a client of GSL has a basic_zstring_span s and calls if (s.empty()) what did he mean to do? Surely not getting a constant false. And probably not trying to check if the basic_zstring_span does not contain even a terminating null byte - because that is what the class is about and is checking. He probably meant to check if the underlying string is empty. So in my opinion empty should either return span_.size() > 1 or be removed. Such a behaviour change should be addressed in a later PR.

@JordanMaples JordanMaples merged commit 63379b7 into microsoft:master Aug 6, 2020
@JordanMaples
Copy link
Contributor

@beinhaerter You're right, empty in its current state doesn't really make much sense. We can definitely address empty in a later PR though. I'll merge the changes now and I'll let you know what the other maintainers think next week after our next meeting. Thanks!

@beinhaerter beinhaerter deleted the zstring_ctor branch August 6, 2020 19:57
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.

None yet

3 participants