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

'strnlen' limits the supported platforms #315

Closed
vervaekejonathan opened this issue Aug 8, 2016 · 8 comments
Closed

'strnlen' limits the supported platforms #315

vervaekejonathan opened this issue Aug 8, 2016 · 8 comments
Assignees

Comments

@vervaekejonathan
Copy link

vervaekejonathan commented Aug 8, 2016

strnlen (in string_span.h) is not available on all platforms (like arm-none-eabi).
To provide more supported platforms, i suggest to use an own implementation.

@neilmacintosh neilmacintosh self-assigned this Aug 9, 2016
@neilmacintosh
Copy link
Contributor

Fair enough. If you have specific compiler+platform combinations where an alternate implementation is needed, you could submit a PR that makes the necessary change for those platforms.

@rianquinn
Copy link
Contributor

It appears to be on all platforms that use cygwin and probably should be a generic replacement. Does adding the following to gsl/string_span sound good?

std::size_t
strnlen(cosnt char *str, std::size_t n)
{
    const char *start = str;

    while (n-- > 0 && *str)
        str++;

    return str - start;
}

@neilmacintosh
Copy link
Contributor

@rianquinn perhaps we should just replace the call to strnlen() with a call to std::char_traits<CharType>::length()? That should work everywhere and should have the same level of performance optimizations as strnlen would have (on platforms that support it).

I'd be happy to look at a PR with those changes....;-)

@rianquinn
Copy link
Contributor

std::char_traits<CharType>::length() does not have a "n" version. It appears that the only places strnlen is being used is for "C" style strings, so strlen would work the same as std::char_traits<CharType>::length(). We could use it, but would lose the additional safety compared to implementing our own strnlen

@neilmacintosh
Copy link
Contributor

of course! whoops, a "thinko" on my part.

In that case, yes, you could add a strnlen equivalent, but I would suggest a couple of small details.

First, call it something other than strnlen, put it in the gsl::details namespace and qualify a call to it. Second, make it a passthrough to strnlen on platforms that support strnlen() so we don't block possible optimizations they have for that function.

Finally, I think rather than doing the pointer arithmetic in your suggested version, couldn't you just use a span?

@rianquinn
Copy link
Contributor

I was thinking that span could be used as well... sort of. the provided n refers to the max string size and not the max size of the char array so the bounds check that span is providing is useless. The advantage of using span IMO is simply to contain the arithmetic to one spot. That being said... under the hood, strnlen is doing nothing of the sort. The irony of using the C library to implement the GSL I guess.

Question: How do you want to determine what platforms support this and which don't? The only test case I have is __CYGWIN__. Someone else had a similar issue for a different platform that I cannot personally test.

Also... what do you mean by "qualify a call"... sorry looks like my knowledge of C++ lingo could use some work :)

@neilmacintosh
Copy link
Contributor

Yes, using a span at least ensures that you never go past n characters looking for a terminator. The nature of the API is that there is no way to know if the provided pointer is long enough. Sad, but necessary for legacy interop.

I think the easiest thing is to define a "feature" macro for the scope of that file, something like GSL_PLATFORM_HAS_STRNLEN, and only define it for now if you see CYGWIN. That way if other people are also missing strnlen, they can offer PRs that expand the range of platforms for which we use our implementation.

By "qualify the call" I just meant call it with the namespace, to ensure the correct function is used, like so: details::string_length(p, n).

@neilmacintosh
Copy link
Contributor

Thanks @vervaekejonathan for the original report and to @rianquinn for fixing this with a PR.

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

No branches or pull requests

3 participants