Skip to content

Better move semantics & fix clang-tidy warnings#3606

Merged
theohax merged 1 commit intodevelopfrom
yyy-clang-tidy-improvements
Dec 18, 2021
Merged

Better move semantics & fix clang-tidy warnings#3606
theohax merged 1 commit intodevelopfrom
yyy-clang-tidy-improvements

Conversation

@theohax
Copy link
Copy Markdown
Contributor

@theohax theohax commented Dec 14, 2021

This is a PR born from the work that I've been doing lately in the networking areas -- things that did not belong to any specific PR, but generally fixing a few clang-tidy warnings and better usage of move semantics.

For a more thorough explanation of why some pass-by-ref is replaced with pass-by-value (and subsequent std::moves), check out this comment, I think it explains it pretty well, but also feel free to bring up any thought you have here, I am open to discuss about it. Thanks!

@theohax theohax added enhancement quality improvements This item indicates the need for or supplies changes that improve maintainability non-functional change labels Dec 14, 2021
@thsfs
Copy link
Copy Markdown
Contributor

thsfs commented Dec 17, 2021

There too many changes for a single commit, I'd suggest grouping them per change type.

}));
}

// TODO: investigate clang-tidy warning about recursive call chain
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.

I wouldn't recommend adding a TODO comment for something that comes from outside the source code (a tool), since the warning will eventually come up when running clang-tidy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's more a note for myself to be honest, I intend to investigate and fix those two very soon so it's easier to see the todo rather than look up the warning again. But I agree with your advice generally.

@theohax
Copy link
Copy Markdown
Contributor Author

theohax commented Dec 18, 2021

There too many changes for a single commit, I'd suggest grouping them per change type.

They are really (all changes) fixing cland-tidy warnings, so the commit message isn't misguiding at all. It could maybe be broken down into more specific warning types, I'll keep that in mind for next time.

@theohax theohax merged commit e573cd7 into develop Dec 18, 2021
@theohax theohax deleted the yyy-clang-tidy-improvements branch December 18, 2021 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement non-functional change quality improvements This item indicates the need for or supplies changes that improve maintainability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants