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

Added an overload taking string literals to BasicStringRef #79

Closed
wants to merge 1 commit into from

Conversation

marton78
Copy link

This patch allows constructing BasicStringRef from a string literal, the size of which is known at compile time. Note that I had to change the constructor taking a const char* to take a const & to const char*, in order to disable the decaying of string literals to pointers.

@TiagoRabello
Copy link

I think you should consider the comments made at N3849 about a similar option encountered while specifying std::basic_string_view.

The relevant comment is at the section Avoid strlen("string literal") where they state some problems of this constructor when interacting with the common pattern of storing strings in fixed-size arrays of char.

@marton78
Copy link
Author

Thanks, good point.

  1. I know of the strlen optimization, however, I assume it is rendered impossible by the lazy calculation of BasicStringRef::size_ on first use. I'd be surprised if the compiler still remembered the array's length after the array has decayed to a pointer. That is, I guess that the length would have to be calculated in BasicStringBuf's constructor, where this information is still available to the compiler.
  2. The example described in N3849 would work by deleting the overload taking non-const char arrays, wouldn't it?

@vitaut
Copy link
Contributor

vitaut commented Oct 29, 2014

Thanks a lot for the patch, @marton78. I tend to agree with the comments in the Avoid strlen("string literal") section of N3849 kindly pointed to by @TiagoRabello.

Ideally the call to strlen should be optimized away by the compiler, but in the current implementation of BasicStringRef it is not done, because the length is calculated in the size method rather than in the constructor. I think the best solution is to follow the basic_string_view approach and move size computation to the constructor:

constexpr BasicStringRef(const Char *s)
  : data_(s), size_(std::char_traits<Char>::length(s)) {}

A quick test shows that the call to strlen is optimized away for string literals in this case. For example, this code

#include "format.h"

void test(fmt::StringRef s) {}

int main() {
  void (*volatile f)(fmt::StringRef s) = test;
  f("some string");
}

is compiled to

.LC0:
    .string "some string"
    ...
main:
    ...
    movl    $.LC0, %edi
    movl    $11, %esi; 11 is the length of "some string" computed at compile time
    movq    $_Z4testN3fmt14BasicStringRefIcEE, 8(%rsp)
    movq    8(%rsp), %rax
    call    *%rax

Then the additional overload for const Char (&) [N] is not necessary.

@marton78
Copy link
Author

+1

Or, you could change the constructor as such:

BasicStringRef(const Char *s, std::size_t n = std::char_traits<Char>::length(s))
  : data_(s), size_(n) {}

in case you don't want to lose the ability to specify sizes explicitly.

@vitaut
Copy link
Contributor

vitaut commented Oct 30, 2014

Done in 5c4b667. Unfortunately referring to parameter s in the default value didn't work, so I ended up with two overloads, one with size parameter and one without.
Cheers!

@vitaut vitaut closed this Oct 30, 2014
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.

3 participants