Skip to content

Modernize C++ tests #5386

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.

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

Closed
wants to merge 1 commit into from
Closed

Modernize C++ tests #5386

wants to merge 1 commit into from

Conversation

cyyever
Copy link
Contributor

@cyyever cyyever commented Sep 29, 2024

Description

Modernize test code.

@cyyever cyyever changed the title Modernize code Modernize C++ tests Sep 29, 2024
@cyyever cyyever marked this pull request as draft September 29, 2024 15:48
@rwgk
Copy link
Collaborator

rwgk commented Sep 29, 2024

I'd rather not make these changes.

E.g.

@@ -92,8 +93,8 @@ TEST_SUBMODULE(class_, m) {
     // test_inheritance
     class Pet {
     public:
-        Pet(const std::string &name, const std::string &species)
-            : m_name(name), m_species(species) {}
+        Pet(std::string name, std::string species)
+            : m_name(std::move(name)), m_species(std::move(species)) {}
         std::string name() const { return m_name; }
         std::string species() const { return m_species; }

That a loss of readability IMO. const in the signature is clear. Scattering std::move() around in the implementation is just clutter in comparison.

Is this a big deal one way or another? — I don't think so. Better leave as is. Better invest time in something that matters more.

@cyyever cyyever closed this Sep 30, 2024
@cyyever cyyever deleted the fix branch September 30, 2024 02:07
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