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

Apply clang-tidy fixes #384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Apply clang-tidy fixes #384

wants to merge 1 commit into from

Conversation

Spartan322
Copy link
Member

Move fixed point LUT generator scripts to misc/scripts

@Spartan322 Spartan322 added bug Something isn't working enhancement New feature or request labels Mar 12, 2025
@Spartan322 Spartan322 marked this pull request as draft March 12, 2025 20:25
@Spartan322 Spartan322 force-pushed the apply/clang-tidy branch 2 times, most recently from 662a0ca to edf1990 Compare March 12, 2025 20:45
Hop311
Hop311 reviewed Mar 12, 2025
View reviewed changes
src/openvic-simulation/defines/AIDefines.cpp Outdated
min_fleets {},
max_fleets {},
time_before_disband {} {}
AIDefines::AIDefines() : fleet_size {}, min_fleets {}, max_fleets {} {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use size_t PROPERTY(var_name, 0) to initialise these in the header and replace the constructor with AIDefines() = default; (or remove it altogether)? Same for other defines structs

Copy link
Member Author

@Spartan322 Spartan322 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I missed these on running clang-tidy.

= default is deceptive because if all the members are trivially constructible and aren't set, the type becomes trivially constructible which while great for performance, in most scenarios also introduces UB if you forget to initialize the value much like an uninitialized int. As a result I'm a bit undecided if we should use = default for cases where we wouldn't benefit from trivial constructibility already.

src/openvic-simulation/interface/GFXObject.cpp
Actor::Actor() : model_file {}, scale { 1 }, idle_animation {}, move_animation {}, attack_animation {} {}
Actor::Actor() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this (and other similar constructors in this file and maybe others) be removed, either replace with = default; in the header or just gone altogether

Copy link
Member Author

@Spartan322 Spartan322 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't change anything (as of now) to be default here, and while it can't really happen as is, we don't want Actor to be trivial constructible which = default can do wherever possible. This PR is not intended to change the calling semantics, just cleanup things according to clang-tidy.

src/openvic-simulation/modifier/ModifierEffectCache.cpp
Comment on lines +11 to +17
ModifierEffectCache::building_type_effects_t::building_type_effects_t() {}
ModifierEffectCache::good_effects_t::good_effects_t() {}
ModifierEffectCache::unit_type_effects_t::unit_type_effects_t() {}
ModifierEffectCache::regiment_type_effects_t::regiment_type_effects_t() {}
ModifierEffectCache::ship_type_effects_t::ship_type_effects_t() {}
ModifierEffectCache::unit_terrain_effects_t::unit_terrain_effects_t() {}
ModifierEffectCache::strata_effects_t::strata_effects_t() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be moved to the header as default or just removed altogether?

src/openvic-simulation/modifier/ModifierEffectCache.cpp Outdated
navy_base_effects {},
ship_type_effects { nullptr },
unit_terrain_effects { nullptr },
ship_type_effects { nullptr }, unit_terrain_effects { nullptr },
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting here seems inconsistent, why are these on the same line? Also I've tried to avoid "tab + 2 spaces" formatting for constructor lists, instead adding 2 spaces before the : so that all the later lines only need a tab

src/openvic-simulation/scripts/ConditionalWeight.hpp
@@ -28,7 +29,7 @@ namespace OpenVic {
struct ConditionalWeight {

private:
fixed_point_t PROPERTY(base);
fixed_point_t PROPERTY(base, fixed_point_t::_0());
Copy link
Contributor

Choose a reason for hiding this comment

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

Across many files in this PR, fixed_point_ts are unnecessarily initialised to 0. It's not the end of the world, but it would be cleaner to avoid it where possible

Copy link
Member Author

@Spartan322 Spartan322 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

fixed_point_t always initialized to 0 because someone set the default constructor to always initialize the value to 0, fixed points are thus always default initialized when declared, truthfully this specific case is pointless but its just mimicking the behavior that was in the source file

src/openvic-simulation/scripts/Script.hpp

protected:
virtual bool _parse_script(ast::NodeCPtr root, _Context... context) = 0;

public:
Script() : _root { nullptr } {}
Script() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not = default?

src/openvic-simulation/types/fixed_point/FixedPointLUT_2_16_EXP_e.hpp
Comment on lines -27 to +34
5174916558532155392
5174916558532153344
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that this changed, I guess we've found an example of why we don't use floats and platform specific implementations of stuff like exp. Not that big a deal here tho as this is by far the least likely value to be used in any exponential calculations we do, but still worth bearing in mind for any future re-generations of this file

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to get a consistent value we can use for this that won't change on each run of the script, I have literally no idea how we'd achieve that with python tho.

@Spartan322 Spartan322 force-pushed the apply/clang-tidy branch 2 times, most recently from 39e2fcf to b2fee04 Compare March 12, 2025 23:24
@Spartan322 Spartan322 mentioned this pull request Mar 12, 2025
Draft
Base automatically changed from add/pre-commit to master March 13, 2025 12:39
@Spartan322 Spartan322 marked this pull request as ready for review March 13, 2025 12:40
@Spartan322
Apply clang-tidy fixes
56c1951 
Move fixed point LUT generator scripts to misc/scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hop311 Hop311 Hop311 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Spartan322 @Hop311