Skip to content

Conversation

@jackgerrits
Copy link
Member

@jackgerrits jackgerrits commented Aug 23, 2021

  • Since checks will be required to pass they should be scoped back. I suggest we just start with the decided coding guideline of braces required for every block
  • Will not pass until the checks are fixed in master

@jackgerrits jackgerrits marked this pull request as draft August 23, 2021 20:06
@jackgerrits jackgerrits marked this pull request as ready for review September 23, 2021 16:27
csl = std::move(ec.l.cs);
else
ld = std::move(ec.l.multi);
ld = ec.l.multi;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess we don't foresee any changes where this would make a difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

This resolves the lint of moving a trivially copyable type having no effect. If this object becomes non-trivial where the move would be necessary then this CI will tell us because of the performance-unnecessary-copy-initialization check.

Copy link
Collaborator

@peterychang peterychang left a comment

Choose a reason for hiding this comment

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

one nit: If you're gonna remove the std::move calls, maybe add a comment somewhere noting that ec.l.cs contains a vector so std::move is necessary, whereas ec.l.multi is a POD type so its unnecessary

@jackgerrits
Copy link
Member Author

That's kind of the point in making this an automated check though - we can enforce that it is done right in case we forget to check the type before copying/moving

@jackgerrits
Copy link
Member Author

Updated to allow for moving trivially copyable types to avoid this confusion.

@jackgerrits jackgerrits enabled auto-merge (squash) September 23, 2021 20:12
@jackgerrits jackgerrits merged commit 1833d26 into VowpalWabbit:master Sep 23, 2021
@jackgerrits jackgerrits deleted the jagerrit/clang-tidy branch September 23, 2021 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants