Skip to content

Refactor and cleanups - #907

Merged
stv0g merged 11 commits into
masterfrom
refactor-cleanups
May 9, 2025
Merged

Refactor and cleanups#907
stv0g merged 11 commits into
masterfrom
refactor-cleanups

Conversation

@stv0g

@stv0gstv0g commented May 8, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@stv0g
stv0g requested a review from pjungkampMay 8, 2025 20:01
@stv0gstv0g self-assigned this May 8, 2025
@stv0g
stv0g requested a review from n-eiling as a code ownerMay 8, 2025 20:01
n-eiling
n-eiling previously approved these changes May 8, 2025
View reviewed changes

@n-eilingn-eiling left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm. Please don't mix style and logic changes in one PR. Makes it unnecessarily hard to review.

@stv0g
stv0gforce-pushed the refactor-cleanups branch from c6b9ebc to 390d249CompareMay 9, 2025 06:13
stv0g added 2 commits May 9, 2025 08:15
@stv0g
chore: Rework SignalData union
41df98c
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix: Typo
e176c58
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
stv0gforce-pushed the refactor-cleanups branch from 390d249 to 412472cCompareMay 9, 2025 06:15
@stv0g
fix(nix): Add criterion dependency for testing
2fc42b7
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
stv0gforce-pushed the refactor-cleanups branch 2 times, most recently from 3b4b181 to 0deed1cCompareMay 9, 2025 09:15
@pjungkamp

pjungkamp commented May 9, 2025

Copy link
Copy Markdown
Contributor

Regarding the virtual/override/final keyword annotation style. I'd like to stick to the CppCoreGuidelines and use exactly one of these for any virtual function. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override

@stv0g

stv0g commented May 9, 2025

Copy link
Copy Markdown
ContributorAuthor

Lgtm. Please don't mix style and logic changes in one PR. Makes it unnecessarily hard to review.

Noted, I will split them in the future. I think only the SignalData changes were functional.

@stv0g
stv0gforce-pushed the refactor-cleanups branch 2 times, most recently from 4c287e3 to d7a2848CompareMay 9, 2025 12:17
pjungkamp
pjungkamp requested changes May 9, 2025
View reviewed changes
Comment threadinclude/villas/node_compat.hpp

@pjungkamppjungkampMay 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This still has both virtual and override on the same function.

@pjungkamppjungkampMay 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now it's missing override for _read and _write.

stv0g added 8 commits May 9, 2025 14:31
@stv0g
fix(style): Use override keyword in nodes
9e1f341
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix(style): Use override keyword in formats
f4883d7
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix(style): Use override keyword in hooks
b9f29cf
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix(style): Use override keyword in API requests
f542350
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix: Correct usage of Node::check()
c70eb04
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix: Remove wrong struct keyword from NodeCompatType
c735415
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix(style): Harmonize comment-style
2fdb1cd
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
fix(cppcheck): Fix cppcheck performance warning
7194eb1
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
@stv0g
stv0gforce-pushed the refactor-cleanups branch from d7a2848 to 7194eb1CompareMay 9, 2025 12:31
pjungkamp
pjungkamp requested changes May 9, 2025
View reviewed changes
Comment threadinclude/villas/node_compat.hpp

@pjungkamppjungkampMay 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now it's missing override for _read and _write.

@stv0g
stv0g merged commit 063f8b3 into masterMay 9, 2025
@stv0g
stv0g deleted the refactor-cleanups branch May 9, 2025 12:45
@pjungkamppjungkamp mentioned this pull request Jun 4, 2025
Merged
7 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pjungkamppjungkamppjungkamp approved these changes
@n-eilingn-eilingn-eiling left review comments

Assignees

@stv0gstv0g

Labels

maintenance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stv0g@pjungkamp@n-eiling