close
Skip to content

Make 'Color' constants 'constexpr' and add tests#1937

Merged
vittorioromeo merged 1 commit into
masterfrom
feature/make_color_constants_constexpr
Feb 8, 2022
Merged

Make 'Color' constants 'constexpr' and add tests#1937
vittorioromeo merged 1 commit into
masterfrom
feature/make_color_constants_constexpr

Conversation

@vittorioromeo

Copy link
Copy Markdown
Member

Description

Follow-up to #1904, using @Bromeon's suggestion to make constants constexpr, and also added some tests.

Tasks

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

How to test this PR?

CI.

@vittorioromeo

Copy link
Copy Markdown
Member Author

@Bromeon: this is what I meant... I remembered trying this, but VS2017 is broken :/

Comment thread include/SFML/Graphics/Color.inl
@vittorioromeo

Copy link
Copy Markdown
Member Author

@Bromeon @binary1248 @eXpl0it3r: how do you feel about dropping VS2017 support? It is clearly broken...

@vittorioromeo

Copy link
Copy Markdown
Member Author

@Bromeon @binary1248 @eXpl0it3r: how do you feel about dropping VS2017 support? It is clearly broken...

Ping :)

@Bromeon

Bromeon commented Jan 12, 2022

Copy link
Copy Markdown
Member

Given that 2017 lies now half a decade in the past, I think it's OK to drop support for VS 2017, especially if it struggles to be C++17 compatible.

It's of course a question if this can be worked around somehow (with reasonable effort).

@eXpl0it3r

Copy link
Copy Markdown
Member

Well we knew going in, that we'll have to abandon some compilers. So that's fine for me. Wondering if we can easily do some branch filtering in Build Bot or drop VS 2017 for 2.6.x as well 🤔

@eXpl0it3r

Copy link
Copy Markdown
Member

The windows-2016 image is deprecated and will be removed in March 2022 anyways, so GitHub Actions couldn't easily cover VS 2017, see https://github.blog/changelog/2021-10-19-github-actions-the-windows-2016-runner-image-will-be-removed-from-github-hosted-runners-on-march-15-2022/

@vittorioromeo vittorioromeo force-pushed the feature/make_color_constants_constexpr branch from 93d777d to b199000 Compare January 27, 2022 02:28
@vittorioromeo

Copy link
Copy Markdown
Member Author

@eXpl0it3r: it seems CI still tries to compile against VS2017. Is there a PR for the removal of this compiler?

@eXpl0it3r

Copy link
Copy Markdown
Member

No, I didn't do anything. If we all agree, I can create one.

@ChrisThrasher

ChrisThrasher commented Feb 5, 2022

Copy link
Copy Markdown
Member

Is this PR unblocked now that VS 2017 has been removed?

@eXpl0it3r eXpl0it3r force-pushed the feature/make_color_constants_constexpr branch from b199000 to 7d278bc Compare February 7, 2022 10:11
@vittorioromeo vittorioromeo requested a review from Bromeon February 8, 2022 22:49
@vittorioromeo

Copy link
Copy Markdown
Member Author

CI passes now. @Bromeon @eXpl0it3r happy to approve?

@vittorioromeo vittorioromeo merged commit 0e41954 into master Feb 8, 2022
@binary1248 binary1248 deleted the feature/make_color_constants_constexpr branch February 18, 2022 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants