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

Codechange: let the setting name be std::string #9314

Merged
merged 4 commits into from Jun 13, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented May 30, 2021

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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@rubidium42 rubidium42 force-pushed the string_setting_name branch from 479c7a9 to af56e73 Jun 6, 2021
{
if (strncmp(name, "company.", 8) == 0) name += 8;
static const std::string_view company_prefix = "company.";
Copy link
Member

@TrueBrain TrueBrain Jun 10, 2021

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?

Loading

Copy link
Contributor Author

@rubidium42 rubidium42 Jun 10, 2021

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.

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 10, 2021

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?

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 10, 2021

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 :)

Loading

{
size_t prefix_len = prefix.size();
if (str.size() < prefix_len) return false;
return str.compare(0, prefix_len, prefix, 0, prefix_len) == 0;
Copy link
Member

@TrueBrain TrueBrain Jun 10, 2021

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

Loading

Copy link
Contributor Author

@rubidium42 rubidium42 Jun 10, 2021

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))."

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 10, 2021

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.

Loading

Copy link
Member

@TrueBrain TrueBrain Jun 13, 2021

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.

Loading

@rubidium42 rubidium42 force-pushed the string_setting_name branch from af56e73 to 91799ce Jun 10, 2021
@rubidium42 rubidium42 merged commit bf500c3 into OpenTTD:master Jun 13, 2021
13 checks passed
Loading
@rubidium42 rubidium42 deleted the string_setting_name branch Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants