Skip to content

Conversation

gnawme
Copy link
Contributor

@gnawme gnawme commented Jan 24, 2023

Thanks a lot for making a contribution to SFML! 🙂

Before you create the pull request, we ask you to check the follow boxes. (For small changes not everything needs to ticked, but the more the better!)

  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?
  • If you have additional steps which need to be performed list them as tasks!

Description

Please describe your pull request.

This PR is related to the issue #2328

Tasks

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

How to test this PR?

Enabled test suite, all tests pass.

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #2377 (a64fad2) into master (ca5ca65) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2377   +/-   ##
=======================================
  Coverage   14.40%   14.40%           
=======================================
  Files         212      212           
  Lines       17898    17897    -1     
  Branches     4397     4397           
=======================================
  Hits         2578     2578           
+ Misses      15146    15144    -2     
- Partials      174      175    +1     
Impacted Files Coverage Δ
include/SFML/Graphics/RenderTarget.hpp 0.00% <ø> (ø)
include/SFML/Network/Socket.hpp 50.00% <ø> (ø)
include/SFML/Network/UdpSocket.hpp 0.00% <ø> (ø)
include/SFML/Window/Joystick.hpp 0.00% <ø> (ø)
src/SFML/Window/Win32/JoystickImpl.cpp 0.71% <0.00%> (ø)
src/SFML/Audio/SoundStream.cpp 0.00% <0.00%> (ø)
src/SFML/Audio/SoundRecorder.cpp 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

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

@ChrisThrasher ChrisThrasher added this to the 3.0 milestone Jan 24, 2023
@ChrisThrasher ChrisThrasher linked an issue Jan 24, 2023 that may be closed by this pull request
@gnawme gnawme force-pushed the feature/use-constexpr-for-enums branch from 4b5e427 to f4a763e Compare January 24, 2023 16:51
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

You removed the API documentation everywhere. Can you add it back?

@gnawme
Copy link
Contributor Author

gnawme commented Jan 24, 2023

You removed the API documentation everywhere. Can you add it back?

I did what? How?

@ChrisThrasher
Copy link
Member

You removed the API documentation everywhere. Can you add it back?

I did what? How?

Screenshot 2023-01-24 at 12 40 22 PM

If you look at the diff, you'll see that you removed some documentation comments.

@gnawme gnawme force-pushed the feature/use-constexpr-for-enums branch from 32d5f1c to 21d6ccc Compare January 24, 2023 21:56
@ChrisThrasher
Copy link
Member

ChrisThrasher commented Jan 24, 2023

Screenshot 2023-01-24 at 3 07 46 PM

There are more issues with signed/unsigned comparisons. If there's a clean fix then I'm cool with us making that. Otherwise let's just make those constants ints like they were before so we can keep this PR neatly contained to solving just a single problem.

Addressed review comments


Addressed review comments


Changed NOLINTBEGIN/END to NOLINTNEXTLINE


Addressed CI complaint


Fixed BSD CI issues
@gnawme gnawme force-pushed the feature/use-constexpr-for-enums branch from 21d6ccc to a64fad2 Compare January 24, 2023 22:14
@ChrisThrasher ChrisThrasher merged commit 4c8b770 into SFML:master Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Replace anonymous enums with constexpr variables.
5 participants