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

Add StringView and Span #384

Merged
merged 4 commits into from
Aug 23, 2020
Merged

Add StringView and Span #384

merged 4 commits into from
Aug 23, 2020

Conversation

fmatthew5876
Copy link
Contributor

@fmatthew5876 fmatthew5876 commented Aug 19, 2020

I'm pulling these out of the other refactor PRs so we can review concerns about how to move forward with these 3rd party implementations and also so I can use this as a base for Player PR's depending on these.

If the String refactor will take a long time to review, I would rather this gets reviewed and in sooner.

Getting this in also means we can compile some of EasyRPG/Player#2280 against current master and review it sooner.

template <typename C, typename T, typename A>
void ToString(const std::basic_string<C,T,A>&) = delete;

// We use macros instead of template to enable implicit string -> string_view conversion here.
Copy link
Member

Choose a reason for hiding this comment

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

Can you give an example what you get with these macros that looks much worse without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the macro I can write this:

std::string x;
Substr(x, 2, 3);

Without it, I have to write this because C++ templates aren't smart enough to figure out that basic_string<C,T,A> should be implicitly convertable to BasicStringView<C,T>;

Maybe C++17 deduction guides can help with this?

std::string x;
Substr(ToStringView(x), 2, 3);

Converting string -> string_view is normal and doesn't cost in performance, so it should be allowed freely. Going the other way means making a copy and thus should be flagged.

std::string y;
StringView x;
x = y; // Ok
y = x; // Error
y = ToString(x); // Ok, you meant to make a copy here.

https://github.com/martinmoene/string-view-lite

* Add char_type hack for fmtlib to work in Player
* Add helper free functions
* Add install rules in lcf/third_party/string_view.hpp
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Adding these classes is uncontroversial, the discussion topic is more about naming and conventions as this can't be changed after.

Because this actually implements standard types how about naming them after standard types? Imo this makes them feel more "natural" here as other devs are used to these names. lcf::string_view, lcf::span etc.

I agree with Utils::-removal but these StartsWith, EndsWith, Substr are imo unnecessary. When you check Player code they appear only 4+2+1 times.

For these few cases something like ToStringView(s).starts_with is good enough.

I accept the ToString/ToStringView functions, could they be named to_string/to_string_view or is that confusing because of std::to_string. That string_view -> string is explicit is good.

The confusion could be reduced by replacing every std::to_string with lcf::to_string, if this is feasible (templates?).

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Aug 20, 2020

Adding these classes is uncontroversial, the discussion topic is more about naming and conventions as this can't be changed after.

Because this actually implements standard types how about naming them after standard types? Imo this makes them feel more "natural" here as other devs are used to these names. lcf::string_view, lcf::span etc.

Since they exist in our namespace, I wanted to be consistent with naming them like our other types. Other libraries do this too, such as llvm::StringRef, folly::StringPiece, gsl::string_span.

Granted, all of those were invented before the final form of std::string_view was standardized.

I'm more leaning towards consistency with the rest of lcf naming style, but I don't care that much so if you feel strongly about renaming them to snake_case let me know and I'll change it.

I agree with Utils::-removal but these StartsWith, EndsWith, Substr are imo unnecessary. When you check Player code they appear only 4+2+1 times.

For these few cases something like ToStringView(s).starts_with is good enough.

I could see us later adding other string algorithms which don't exist as member functions of StringView. In which case it would be nice just to have a set of free functions somewhere I can just call on any string types instead of having to check if one is a member function or not.

That being said, we aren't there yet and if such library is needed it would go in it's own "string_utils.h" or such header. So I can drop these for now.

I accept the ToString/ToStringView functions, could they be named to_string/to_string_view or is that confusing because of std::to_string. That string_view -> string is explicit is good.

There is some headaches here.

For the fmtlib hack you need to define fmt::basic_string_view to_string_view(T) function in the same namespace as the type. fmtlib has it's own string view type that it uses internally.

So I'd rather avoid conflicts with that name. Also these names look like other lcf functions are are easily greppable.

All this mess goes away when we have C++17 std::string_view.

The confusion could be reduced by replacing every std::to_string with lcf::to_string, if this is feasible (templates?).

In player code, I made a new using StringView = lcf::StringView so that in player I don't need to say lcf:: somewhere and I have a header to add the fmtlib hacks. It also allows us to move this out of lcf later if we want to do and not have to make a huge diff in player.

So using lcf::to_string() would look wrong there.

Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Imo this can now stay as is. Will think about it again tomorrow.

@Ghabry Ghabry merged commit 5b8664f into EasyRPG:master Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants