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.

Sign up for GitHub

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

Jump to bottom

Refactor and disallow type conversion operators #6373

Closed
wants to merge 17 commits into from
Closed

Refactor and disallow type conversion operators #6373

wants to merge 17 commits into from

Conversation

bunnybot
Copy link

NordfrieseMirrored from Codeberg
Created on Sat Feb 24 13:37:22 CET 2024 by Benedikt Straub (Nordfriese)

Type of change
Refactoring

Issue(s) closed
Implicit type conversions are bad. This branch replaces all custom conversion operators with explicit functions, and adds a codecheck rule against type conversion operators.

Possible regressions
Validity and value of Coords, Vision/VisibleState, MessageID, and FileRead::Pos/FileWrite::Pos.

@bunnybot bunnybot added this to the v1.3 milestone Feb 24, 2024
@bunnybot bunnybot self-assigned this Feb 24, 2024
@bunnybot
Copy link
Author

Assigned to Nordfriese

@bunnybot bunnybot added codecheck Compiler warnings, clang-tidy, code style checks, linters, … cleanup & refactoring Improving our code quality labels Feb 24, 2024
@bunnybot bunnybot added the ci:success CI checks succeeded label Apr 2, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Apr 11, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Apr 20, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Apr 21, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Apr 30, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 12, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 18, 2024
@bunnybot bunnybot added ci:fail CI checks failed and removed ci:success CI checks succeeded labels May 22, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:fail CI checks failed labels May 27, 2024
bunnybot
bunnybot commented Jun 1, 2024
View reviewed changes
Copy link
Author

@bunnybot bunnybot left a comment

Choose a reason for hiding this comment

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

matthiaklMirrored from Codeberg
On Sat Jun 01 10:28:04 CEST 2024, matthiakl commented:

LGTM. I don't see any behavior changes.
Some comments are obsolete now.

src/io/fileread.h Outdated
@@ -34,10 +34,10 @@
class FileRead : public StreamRead {
public:
struct Pos {
Pos(size_t const p = 0) : pos(p) { // NOLINT allow implicit conversion
explicit Pos(size_t const p = 0) : pos(p) { // NOLINT allow implicit conversion
Copy link
Author

Choose a reason for hiding this comment

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

matthiaklMirrored from Codeberg
On Sat Jun 01 10:28:04 CEST 2024, matthiakl wrote:

- Pos(size_t const p = 0) : pos(p) {  // NOLINT allow implicit conversion
+ explicit Pos(size_t const p = 0) : pos(p) {

No implicit conversion allowed.

src/io/filewrite.h Outdated
@@ -30,11 +30,11 @@ class FileSystem;
class FileWrite : public StreamWrite {
public:
struct Pos {
Pos(size_t const p = 0) : pos(p) { // NOLINT allow implicit conversion
explicit Pos(size_t const p = 0) : pos(p) { // NOLINT allow implicit conversion
Copy link
Author

Choose a reason for hiding this comment

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

matthiaklMirrored from Codeberg
On Sat Jun 01 10:29:32 CEST 2024, matthiakl wrote:

- Pos(size_t const p = 0) : pos(p) {  // NOLINT allow implicit conversion
+ explicit Pos(size_t const p = 0) : pos(p) {

No implicit conversion allowed.

@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Jun 1, 2024
bunnybot
bunnybot commented Jun 1, 2024
View reviewed changes
Copy link
Author

@bunnybot bunnybot left a comment

Choose a reason for hiding this comment

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

matthiaklMirrored from Codeberg
On Sat Jun 01 22:29:57 CEST 2024, matthiakl approved this pull request:

<@>bunnybot merge

@bunnybot bunnybot added the review:approve The pull request has been approved. label Jun 1, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels Jun 1, 2024
@Noordfrees
Copy link
Member

Noordfrees commented Jun 2, 2024 •
edited

comment deleted

@bunnybot bunnybot closed this Jun 2, 2024
@bunnybot bunnybot deleted the mirror/Nordfriese/widelands/forbid-type-conversion-operators branch June 2, 2024 08:37
Noordfrees added a commit to Noordfrees/widelands that referenced this pull request Jun 2, 2024
@Noordfrees
Refactor and disallow type conversion operators (GH widelands#6373 / CB
29f9acd 
widelands#4736)

Co-authored-by: Benedikt Straub <[email protected]>
Co-committed-by: Benedikt Straub <[email protected]>
@bunnybot
Copy link
Author

bunnybot commented Jun 2, 2024 •
edited

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

deleted comment

@bunnybot
Copy link
Author

bunnybot commented Jun 2, 2024

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

NoordfreesMirrored from GitHub
On Sun Jun 02 10:35:38 CEST 2024, Noordfrees wrote:

NordfrieseMirrored from Codeberg
On Sun Jun 02 10:23:02 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Thanks for the review :)

<@>bunnybot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bunnybot bunnybot

Labels
ci:success CI checks succeeded cleanup & refactoring Improving our code quality codecheck Compiler warnings, clang-tidy, code style checks, linters, … review:approve The pull request has been approved.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bunnybot @Noordfrees