Skip to content

Conversation

wisniewskij
Copy link

Summary

I uncommented - performance-* clang-tidy check glob. This check encourages good performance practices. After that I solved CSA errors generated by the code.

Copy link
Member

@MatiPl01 MatiPl01 left a comment

Choose a reason for hiding this comment

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

Looks very well. I left just 2 minor suggestions you may want to refer to. Good job overall!

Comment on lines 35 to +36
RegistryEntry{
std::move(animationsVector),
buildAnimationToIndexMap(animationsVector)});
animationsVector, buildAnimationToIndexMap(animationsVector)});
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should keep std::move here and just remove the const modifier from the animationsVector declaration:

const auto animationsVector =
      buildAnimationsVector(rt, shadowNode, animationNames, newAnimations);

This std::move call was added purposely but the the const modifier near the animationsVector prevented it from working as expected.

Copy link
Author

Choose a reason for hiding this comment

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

What I don't get here is why do we move the animationsVector first and then try to use it straight away. Isn't it in indeterminate state after that?

Comment on lines 6 to 7
DelayedItem<TValue>::DelayedItem(const double timestamp, const TValue value)
: timestamp(timestamp), value(value) {}
: timestamp(timestamp), value(std::move(value)) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DelayedItem<TValue>::DelayedItem(const double timestamp, const TValue value)
: timestamp(timestamp), value(value) {}
: timestamp(timestamp), value(std::move(value)) {}
DelayedItem<TValue>::DelayedItem(const double timestamp, TValue value)
: timestamp(timestamp), value(std::move(value)) {}

Good change. I think you also should remove const from the value parameter declaration.

Copy link
Author

Choose a reason for hiding this comment

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

Done. I changed timestamp to not-const as well because that's how it's declared in .h.

@wisniewskij wisniewskij self-assigned this Oct 21, 2025
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.

2 participants