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.

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

STYLE: Replace (const std::string) casts with C++17 std::string_view #4587

Conversation

N-Dekker
Copy link
Contributor

Prevented potential static analysis warnings, like Clang-Tidy:

C-style casts are discouraged; use static_cast [google-readability-casting]

And JetBrains ReSharper:

C-style cast is used instead of a C++ cast

Note that std::string_view is more lightweight than std::string.

Prevented potential static analysis warnings, like Clang-Tidy:

> C-style casts are discouraged; use static_cast [google-readability-casting]

And JetBrains ReSharper:

> C-style cast is used instead of a C++ cast

Note that `std::string_view` is more lightweight than `std::string`.
@github-actions github-actions bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) labels Apr 15, 2024
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Apr 15, 2024

By the way, I noticed that GoogleTest has a few macro's specifically for string comparison, avoiding the need to manually convert the arguments to std::string or std::string_view. Might be worth considering 😃

@dzenanz
Copy link
Member

dzenanz commented Apr 15, 2024

GoogleTest has a few macro's specifically for string comparison

Make a competing PR using those? 😄

@N-Dekker
Copy link
Contributor Author

/azp run ITK.macOS

@N-Dekker
Copy link
Contributor Author

Note that std::string_view only provides read-only to the data, so in that sense, it's similar to const std::string &.

@N-Dekker N-Dekker marked this pull request as ready for review April 15, 2024 20:46
@hjmjohnson hjmjohnson merged commit 2a1265d into InsightSoftwareConsortium:master Apr 16, 2024
14 checks passed
Comment on lines -86 to +87
ITK_TEST_EXPECT_EQUAL((const std::string) "QuadEdgeMeshEulerOperatorDeleteCenterVertexFunction",
(const std::string)deleteCenterVertex->GetNameOfClass());
ITK_TEST_EXPECT_EQUAL(std::string_view("QuadEdgeMeshEulerOperatorDeleteCenterVertexFunction"),
std::string_view(deleteCenterVertex->GetNameOfClass()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for merging, @hjmjohnson! For the record, none of the recent versions of this test (pull request #4576, #4583, #4587) properly handle the possibility of deleteCenterVertex->GetNameOfClass() accidentally returning a null. I was hoping that the std::string_view(const char*) constructor would support a null char pointer as argument, but unfortunately it does not. Neither does the originally used std::string(const char*) constructor. It's undefined behavior 🤷

If you just want to ensure that this GetNameOfClass() override has code coverage, you might as well just checks that the returned pointer is not null. For example as follows:

ITK_TEST_EXPECT_TRUE(deleteCenterVertex->GetNameOfClass());

Instead of doing the string comparison check.

Copy link
Member

Choose a reason for hiding this comment

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

Is GetNameOfClass() ever expected to return a NULL pointer? So that problem might be moot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is GetNameOfClass() ever expected to return a NULL pointer?

No, but unit tests allow us to express what we expect, and what we do not expect 😸

Obviously it's easy for anyone to write a malicious GetNameOfClass() override that does return NULL. 😸 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants