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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

khatharr
Copy link

Description

This is a modification to sf::String to add support for conversion from std::string_view and related derivations of std::basic_string_view, similar to the existing conversions within sf::String.

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?

This is the test program I used, although a better one is probably appropriate. (I've never worked with uint32_t characters, so I wouldn't know where to begin with that.)

#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;
      }
    }
  }
}

@eXpl0it3r eXpl0it3r added this to Backlog in SFML 3.0.0 via automation Mar 14, 2023
@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Mar 14, 2023
@eXpl0it3r eXpl0it3r moved this from Backlog to C++17 Changes in SFML 3.0.0 Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #2447 (d80a03c) into master (b6c54ac) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2447      +/-   ##
==========================================
- Coverage   23.16%   23.15%   -0.02%     
==========================================
  Files         212      212              
  Lines       18065    18077      +12     
  Branches     4403     4404       +1     
==========================================
  Hits         4185     4185              
- Misses      13168    13439     +271     
+ Partials      712      453     -259     
Impacted Files Coverage Δ
include/SFML/System/String.hpp 100.00% <ø> (ø)
src/SFML/System/String.cpp 13.20% <0.00%> (-0.80%) ⬇️

... and 53 files with indirect coverage changes


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 b6c54ac...d80a03c. Read the comment docs.

@ChrisThrasher
Copy link
Member

@vittorioromeo What do you think of this?

@khatharr
Copy link
Author

This probably shouldn't be pulled yet. I didn't write the doxy for the overloads in the header. This is just for consideration, and I can address issues and write the comments later.

@ChrisThrasher
Copy link
Member

ChrisThrasher commented Mar 14, 2023

This probably shouldn't be pulled yet. I didn't write the doxy for the overloads in the header. This is just for consideration, and I can address issues and write the comments later.

Okay good to know. I was about to comment on the missing docs. We can focus on the core feature first before I ask you to fix formatting and docs and all that minutia. By the way ignore the code coverage failures. That's not your fault.

@khatharr
Copy link
Author

Yeah. I was just looking at the details. The formatter didn't like the constructor initializers being on a new line. I'll fix that.

@khatharr khatharr closed this by deleting the head repository Mar 15, 2023
@eXpl0it3r
Copy link
Member

eXpl0it3r commented Mar 15, 2023

Recommend using branches in git for another time, so you have better control how to handle updates and don't need to close and re-create PRs 🙂

@khatharr
Copy link
Author

khatharr commented Mar 15, 2023 via email

@eXpl0it3r eXpl0it3r moved this from C++17 Changes to Done in SFML 3.0.0 Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
SFML 3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants