Adding support for std::string_view to sf::String

[khatharr]

Description

Revision of #2447

I went ahead and cleaned up formatting and added comments, just to get it done.

This PR is related to the issue #2445

Tasks

• Tested on Linux
• Tested on Windows
• Tested on macOS
• Tested on iOS
• Tested on Android

How to test this PR?

Test driver (same as before):

#include <SFML/Main.hpp>
#include <SFML/Graphics.hpp>
#include <SFML/Window.hpp>

#include <string_view>

int main() {
  sf::RenderWindow window(sf::VideoMode(sf::Vector2u(800, 600)), "SFML Testbed");
  window.setFramerateLimit(60);

  sf::Font font;
  bool derp = font.loadFromFile("c:/Windows/Fonts/arial.ttf");
  
  const char data[] = "derpdurr";
  const wchar_t wdata[] = L"herpboing";

  std::string str(data + 4, 4);
  std::string_view sv(data, 4);
  std::wstring wstr(wdata, 4);
  std::wstring_view wsv(wdata + 4);

  std::vector<sf::Text> texts;
  texts.emplace_back(wstr, font);
  texts.back().setPosition(sf::Vector2f(10, 0));
  texts.emplace_back(sv, font);
  texts.back().setPosition(sf::Vector2f(10, 40));
  texts.emplace_back(str, font);
  texts.back().setPosition(sf::Vector2f(10, 80));
  texts.emplace_back(wsv, font);
  texts.back().setPosition(sf::Vector2f(10, 120));
  
  for(bool run = true; run;) {
    window.clear();
    for(auto& text : texts) {
      window.draw(text);
    }
    window.display();

    sf::Event event;
    while(window.pollEvent(event)) {
      if(event.type == sf::Event::Closed) {
        run = false; break;
      }
    }
  }
}

[codecov]

Codecov Report

Merging #2451 (83779bd) into master (641cf54) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 83779bd differs from pull request most recent head 9c2c29a. Consider uploading reports for the commit 9c2c29a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2451      +/-   ##
==========================================
- Coverage   23.15%   23.14%   -0.02%     
==========================================
  Files         212      212              
  Lines       18063    18073      +10     
  Branches     4405     4406       +1     
==========================================
  Hits         4183     4183              
- Misses      13152    13162      +10     
  Partials      728      728              

Impacted Files	Coverage Δ
include/SFML/System/String.hpp	100.00% <ø> (ø)
src/SFML/System/String.cpp	13.33% <0.00%> (-0.67%)	⬇️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 641cf54...9c2c29a. Read the comment docs.

[khatharr]

Okay, I don't know what the formatter wants, then. it appears to be objecting to my empty lines and replacing them with its own empty lines. I checked the line endings and they're uniform with the rest of the file. shrug

[ChrisThrasher]

Okay, I don't know what the formatter wants, then. it appears to be objecting to my empty lines and replacing them with its own empty lines. I checked the line endings and they're uniform with the rest of the file. shrug

It's saying that you have trailing whitespace on those lines that needs to be removed. A line with on code on it should not contain 4 spaces. It should contain precisely one newline character and that is all.

You an build the format target to run the autoformatter if you don't want to manually copy its advice into your editor. It's all automated via clang-format.

[khatharr]

I've never used clang. Is this something I can/should do on github?

[ChrisThrasher]

I've never used clang. Is this something I can/should do on github?

You're better off just copying the output from the failed CI job then. If you're not on Windows, clang-format (which is different from clang) is very easy to install and use but on Windows it may be harder.

[khatharr]

Okay. I think I've got it sorted out now.
Should I push another commit, or just nuke this and submit another PR?

Actually, if clang-format is standalone then I'll look at running it from my RPi before uploading anything new.

[ChrisThrasher]

Okay. I think I've got it sorted out now. Should I push another commit, or just nuke this and submit another PR?

Please do not make another PR. That's not the right way to use GitHub. Just add another commit to this PR.

[khatharr]

Understood. Another project I worked on wanted to only pull single commits, so I wasn't sure. In the future I'll make a new branch and then figure it out from there.

…

On Wed, Mar 15, 2023, 11:05 AM Chris Thrasher ***@***.***> wrote: Okay. I think I've got it sorted out now. Should I push another commit, or just nuke this and submit another PR? Please do not make another PR. That's not the right way to use GitHub. Just add another commit to this PR. — Reply to this email directly, view it on GitHub <#2451 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACXDSQENB7SXO3RN43YU5PDW4IAINANCNFSM6AAAAAAV4EIXH4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ChrisThrasher]

You can still have single-commit PRs if you like. You can amend a commit in a branch and do a force push to update a PR without adding a 2nd commit. There are various options that are better than making a new PR every time you need to make a single change.

[ChrisThrasher]

I'd like to hold off on this PR until we can merge #2466. Currently sf::String has no tests which makes me hesitant to add features. Otherwise I think I like this.

[vittorioromeo]

I'd like to hold off on this PR until we can merge #2466. Currently sf::String has no tests which makes me hesitant to add features. Otherwise I think I like this.

@ChrisThrasher: should also wait for #2480 and #2470?

[ChrisThrasher]

Yeah I think I'd prefer it wait if anything just to avoid merge conflicts. I apologize for the delays. This PR was part of what inspired me to look into sf::String in the first place. I expect #2480 to get merged soon.

[eXpl0it3r]

This can be rebased onto master now 🙂

[khatharr]

Is that something I need to do, or can someone who knows what needs to be done take care of it?

[ChrisThrasher]

Is that something I need to do, or can someone who knows what needs to be done take care of it?

It's something that you ought to do

[khatharr]

How do I do it?

[ChrisThrasher]

How do I do it?

https://learngitbranching.js.org

This website has tutorials on Git rebasing you can learn from.

[khatharr]

Thanks!

[khatharr]

Ack, I didn't mean for that to come here.

[khatharr]

Okay, so I reverted that and then told it to update from SFML/master, then I resolved the conflicts and looked over it and I think that what I have locally is correct, but when I try to build it I get:

FAILED: src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/ImageLoader.cpp.obj C:\PROGRA~2\MIB055~1\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe /nologo /TP -DSFML_GRAPHICS_EXPORTS -DSTBI_FAILURE_USERMSG -D_CRT_SECURE_NO_WARNINGS -D_WINSOCK_DEPRECATED_NO_WARNINGS -I..\..\..\include -I..\..\..\src -I..\..\..\extlibs\headers\stb_image -I..\..\..\extlibs\headers\glad\include -I..\..\..\extlibs\headers\freetype2 /DWIN32 /D_WINDOWS /GR /EHsc /Zi /Ob0 /Od /RTC1 -MDd /WX /W4 /w14242 /w14254 /w14263 /w14265 /w14287 /we4289 /w14296 /w14311 /w14545 /w14546 /w14547 /w14549 /w14555 /w14619 /w14640 /w14826 /w14905 /w14906 /w14928 /permissive- /wd4068 /wd4505 /wd4800 -std:c++17 /showIncludes /Fosrc\SFML\Graphics\CMakeFiles\sfml-graphics.dir\ImageLoader.cpp.obj /Fdsrc\SFML\Graphics\CMakeFiles\sfml-graphics.dir\ /FS -c ..\..\..\src\SFML\Graphics\ImageLoader.cpp C:\Users\Khatharr\Desktop\SFML\extlibs\headers\stb_image\stb_image.h(2224): error C2220: the following warning is treated as an error C:\Users\Khatharr\Desktop\SFML\extlibs\headers\stb_image\stb_image.h(2224): warning C4242: 'argument': conversion from 'int' to 'short', possible loss of data C:\Users\Khatharr\Desktop\SFML\extlibs\headers\stb_image\stb_image.h(2281): warning C4242: 'argument': conversion from 'int' to 'short', possible loss of data C:\Users\Khatharr\Desktop\SFML\extlibs\headers\stb_image\stb_image.h(3425): warning C4242: '=': conversion from 'int' to 'unsigned char', possible loss of data

Should this be building cleanly?

[ChrisThrasher]

Should this be building cleanly?

Yes. Update to CMake 3.24 or newer to fix it. It's an issue with MSVC not supporting system headers until recently. You can disable WARNINGS_AS_ERRORS as a temporary fix until you can upgrade CMake. I believe the latest version of Visual Studio ships with a sufficiently new CMake so try upgrading VS.

[ChrisThrasher]

The PR now includes a ton of changes that should be here which means the rebase you did must have not quite worked.

[khatharr]

I can't find a way to get rid of the screw-up I made earlier. I have a branch called "string_view" on my repo that has what I want in it, and I'd like to get it to just take the place of my "master" on this PR, but there doesn't seem to be a way to wipe out the history of my screw ups in "master". I can revert the change I made, but it still sees that as 76 changed files, etc.

I can make a new PR to solve this quickly (I'm sorry, I know that's annoying) but I can't figure out any other way to fix this at present.

I'm going to try to push a revert and see what it does.

[khatharr]

Okay, I think I have it... Let's see how it likes this...

:( No good.

[ChrisThrasher]

You can open a new PR if that's what you need

[khatharr]

Moved to #2517