Skip to content

Add StringPiece::substr method#2722

Open
elliotgoodrich wants to merge 1 commit intoninja-build:masterfrom
elliotgoodrich:small-string-utils
Open

Add StringPiece::substr method#2722
elliotgoodrich wants to merge 1 commit intoninja-build:masterfrom
elliotgoodrich:small-string-utils

Conversation

@elliotgoodrich
Copy link
Copy Markdown
Contributor

Add StringPiece::substr with almost identical behavior as
std::string_view::substr. The only difference is what happens when
pos > size(), which is StringPiece::substr will call Fatal and
std::string_view::substr will throw or terminate (depending on whether
exceptions are enabled).

In ninja, where exceptions are disabled, these methods are basically identical and it should be no harder to migrate StringPiece to std::string_view after this.

operator== was moved to be a free function so that we can write both

StringPiece("abc") == "abc";
"abc" == StringPiece("abc");

as currently we can't write the second line because we can't implicitly convert "abc" to a StringPiece.

This changes is needed for future changes to reduce the number of std::string copies for performance reasons.

@elliotgoodrich
Copy link
Copy Markdown
Contributor Author

This has been pulled out of #2706

@digit-google
Copy link
Copy Markdown
Contributor

Thanks, this looks good to me, but can you add some unit-tests for it for good measure? No need to test the Fatal() case in substr() here.

Add `StringPiece::substr` with almost identical behavior as
`std::string_view::substr`.   The only difference is what happens when
`pos > size()`, which is `StringPiece::substr` will call `Fatal` and
`std::string_view::substr` will throw or terminate (depending on whether
exceptions are enabled).

In ninja, where exceptions are disabled, these methods are basically
identical and it should be no harder to migrate `StringPiece` to
`std::string_view` after this.

`operator==` was moved to be a free function so that we can write both

```cpp
StringPiece("abc") == "abc";
"abc" == StringPiece("abc");
```

as currently we can't write the second line because we can't implicitly
convert `"abc"` to a `StringPiece`.

This changes is needed for future changes to reduce the number of
`std::string` copies for performance reasons.
@elliotgoodrich
Copy link
Copy Markdown
Contributor Author

Thanks, this looks good to me, but can you add some unit-tests for it for good measure? No need to test the Fatal() case in substr() here.

No problem @digit-google - done!

Copy link
Copy Markdown
Contributor

@digit-google digit-google left a comment

Choose a reason for hiding this comment

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

Thank you, @jhasse can you take a look!

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.

2 participants