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
Codechange: let the setting name be std::string #9314
Conversation
479c7a9
to
af56e73
Compare
{ | ||
if (strncmp(name, "company.", 8) == 0) name += 8; | ||
static const std::string_view company_prefix = "company."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a string be stored in a string_view
? Where is the real string stored?
Shouldn't this just be a std::string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. string_view is like span but for string-like things, and in this case it is initialized with a C-string instead of converting the C-string to a std::string and then to a std::string_view. The advantage of using string_view here is that information of the length that is available at compile time does not get lost, compared to using a const char *
, which makes later size checks/comparisons cheaper. And that calls to StrStartsWith
do not need to (implicitly) convert data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes it seems you and I are not talking the same language ;) You do realise I reviewed enough of your PRs to know what std::string_view
does right? Not what I was asking about ;)
What I am asking, is that string_view
points to (but does not contain) a string. So is it safe to assign a value to string_view
like this, or do we need to store it in a proper variable first? For std::span
I have been told it is UB, so is it a similar problem with string_view
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer my own question: "it depends". https://en.cppreference.com/w/cpp/string/basic_string_view gives a nice example of good and bad usage, which can be best seen here: https://godbolt.org/z/c6YcG1Tzj . Annoyingly, GCC doesn't complain about this at all. Clang does, so that is something at least :)
So from my understanding, your usage is fine, as the c-string is in the .data
, and as such remains valid. So the view around it remains valid too. If it would be anything that requires any runtime setup, it will not be valid.
Such a thin line to walk .. parts of C++ that still buzz my ears :)
{ | ||
size_t prefix_len = prefix.size(); | ||
if (str.size() < prefix_len) return false; | ||
return str.compare(0, prefix_len, prefix, 0, prefix_len) == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am staring at this, and think: sure? I really have no clue if this is doing the right thing :D
We really need unit-tests for these kind of things .. :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even better, use library quality code such as https://en.cppreference.com/w/cpp/string/basic_string_view/starts_with
If the comment is about the compare, cppreference says "constexpr int compare(size_type pos1, size_type count1, basic_string_view v, size_type pos2, size_type count2) const;
(3)
...
3) Equivalent to substr(pos1, count1).compare(v.substr(pos2, count2))
."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the comment is not about the compare. The comment is that I have no clue if this is doing the right thing under all circumstance, as that is near impossible to figure out based on the code. For that, unit-tests would be very welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, as mentioned, reviewing this by code was really difficult to estimate if this would do the right thing. So I made a mini-unit-text:
https://godbolt.org/z/bxErvPY3o
It seems to be doing what I expect, but let me know if I forgot any test-cases.
…ds with a given other string
af56e73
to
91799ce
Compare
Motivation / Problem
Conversion of C-string to std::string is not completed yet. And since we're heavily refactoring settings, lets tackle one there too.
Description
Introduce helper functions for determining a string starts or ends with a given prefix/suffix.
Introduce helper function for passing std::string as SetDParamStr for error messages.
Rewrite some internal code to make use of those.
Sort of feature: when doing something like 'set engine_renew on', instead of it saying 'engine_renew = on' it will now mention the fully qualified name, i.e. 'company.engine_renew'.
Limitations
Left script and gamelog code alone... something with rabbit holes ;)
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.