-
-
Notifications
You must be signed in to change notification settings - Fork 726
ENH: Add itk::Copy(const T & original), which simply returns a copy
#4383
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /*========================================================================= | ||
| * | ||
| * Copyright NumFOCUS | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0.txt | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| *=========================================================================*/ | ||
|
|
||
| #ifndef itkCopy_h | ||
| #define itkCopy_h | ||
|
|
||
| namespace itk | ||
| { | ||
|
|
||
| /** Returns a copy of its argument. Primarily used to make the act of copying (typically involving a copy-constructor | ||
| * call) explicit, and to avoid warnings like: | ||
| * | ||
| * - Microsoft Visual Studio 2022, IntelliSense Code Linter [lnt-accidental-copy] "`auto` doesn't deduce references. A | ||
| * possibly unintended copy is being made" (https://learn.microsoft.com/en-us/cpp/ide/lnt-accidental-copy) | ||
| * - Microsoft Visual Studio 2022, Warning C26820, "This is a potentially expensive copy operation. Consider using a | ||
| * reference unless a copy is required (p.9)" (https://learn.microsoft.com/en-us/cpp/code-quality/c26820?view=msvc-170) | ||
| * - Synopsys Coverity 2023.6.2, "CID 331225: Performance inefficiencies (AUTO_CAUSES_COPY)", "Using the `auto` | ||
| * keyword without an `&` causes the copy of an object...". | ||
| * - SonarSource static code analysis, "Unnecessary expensive copy should be avoided when using `auto` as a placeholder | ||
| * type", "Avoid this unnecessary copy by using a "const" reference. (cpp:S6032)", | ||
| * https://rules.sonarsource.com/cpp/RSPEC-6032/ | ||
| * | ||
| * Example: | ||
| \code | ||
| const auto possiblyUnintendedCopy = image->GetRequestedRegion(); // Warning! | ||
| const auto intendedCopy = Copy(image->GetRequestedRegion()); // OK, no warning :-) | ||
| const auto [index, size] = Copy(image->GetRequestedRegion()); // OK, no warning :-) | ||
| \endcode | ||
| * | ||
| * \note In general, it is up to the programmer to choose between doing a copy and using a reference, of course. `Copy` | ||
| should only be used when doing a copy is preferred over using a reference. | ||
| */ | ||
| template <typename T> | ||
| [[nodiscard]] constexpr T | ||
| Copy(const T & original) | ||
| { | ||
| // May call the copy-constructor of `T`. | ||
| return original; | ||
| } | ||
|
|
||
| } // namespace itk | ||
|
|
||
|
|
||
| #endif // itkCopy_h | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| /*========================================================================= | ||
| * | ||
| * Copyright NumFOCUS | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0.txt | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| *=========================================================================*/ | ||
|
|
||
| // First include the header file to be tested: | ||
| #include "itkCopy.h" | ||
| #include <gtest/gtest.h> | ||
|
|
||
| #include <array> | ||
| #include <cstdint> // For intmax_t. | ||
| #include <random> | ||
| #include <utility> // For pair. | ||
| #include <vector> | ||
|
|
||
|
|
||
| // Check compile-time evaluation: | ||
| static_assert(itk::Copy(0) == 0); | ||
| static_assert(itk::Copy(1) == 1); | ||
|
|
||
| namespace | ||
| { | ||
| // Returns a (const) reference to the specified argument. | ||
| template <typename T> | ||
| const T & | ||
| GetReference(const T & arg) | ||
| { | ||
| return arg; | ||
| } | ||
| } // namespace | ||
|
|
||
|
|
||
| // Checks that an object returned by `itk::Copy(original)` compares equal to the original, even though it has a | ||
| // different address. | ||
| TEST(Copy, EqualToTheOriginalButHasDifferentAddress) | ||
| { | ||
| const auto check = [](const auto & original) { | ||
| auto && copy = itk::Copy(original); | ||
|
|
||
| // The returned `copy` has an equal value... | ||
| EXPECT_EQ(copy, original); | ||
|
|
||
| // ... but a different address! | ||
| EXPECT_NE(©, &original); | ||
| }; | ||
|
|
||
| std::default_random_engine randomEngine{}; | ||
| const auto getRandomNumber = [&randomEngine] { return std::uniform_int_distribution<>{}(randomEngine); }; | ||
|
|
||
| check(getRandomNumber()); | ||
| check(std::vector<int>(3, getRandomNumber())); | ||
| check(std::array<int, 3>{ getRandomNumber(), getRandomNumber(), getRandomNumber() }); | ||
| } | ||
|
|
||
|
|
||
| // Tests that `itk::Copy` allows storing a copy of a value in a variable declared by `auto`, without a warning. | ||
| TEST(Copy, AllowsUsingAutoKeywordCopyingWithoutWarnings) | ||
|
Comment on lines
+69
to
+70
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @issakomi Can you please tell us if the proposed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will try. BTW, I don't run a Coverity scan for the entire ITK. I scan my applications from time to time and some headers from ITK are also scanned if they are enabled somehow. I'll try to reproduce it. IMHO we could just ignore the warnings.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @issakomi For a specific application, I think those auto-causes-copy warnings can usually be ignored safely. But I think there is a policy for ITK to address warnings as much as possible. (Please correct me if I'm wrong!) And it's also harder to ignore these specific warnings now that they come from at least three different tools (Intellisense/Linter, MSVC compiler and Coverity).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @issakomi I just extended the auto originalPair =
std::make_pair(std::vector<std::intmax_t>{ 1, 2, 3, 4 }, std::array<std::intmax_t, 4>{ 1, 2, 3, 4 });
const auto [first, second] = itk::Copy(originalPair);Does the I did already observe that the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What I mean is that AUTO_CAUSES_COPY can be skipped if the object is small and trivial to copy. I have no experience with IntelliSense Code Linter. Coverity promises to suggest 'auto &' only if possible. The new Copy function is a way to silence the warning, IMHO, it's the same thing as ignoring it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The code below does not generate any Coverity warnings: template <typename T>
[[nodiscard]] constexpr T
Copy(const T & original)
{
// May call the copy-constructor of `T`.
return original;
}{
std::vector<std::intmax_t> originalVector{ 1, 2, 3, 4 };
const auto copyOfVector = Copy(originalVector);
std::cout << copyOfVector[0] << std::endl;
std::array<std::intmax_t, 4> originalArray{ 1, 2, 3, 4 };
const auto copyOfArray = Copy(originalArray);
std::cout << copyOfArray[0] << std::endl;
auto originalPair = std::make_pair(std::vector<std::intmax_t>{ 1, 2, 3, 4 }, std::array<std::intmax_t, 4>{ 1, 2, 3, 4 });
const auto [first, second] = Copy(originalPair);
const auto copyOfPair = Copy(originalPair);
std::cout << first[0] << std::endl;
std::cout << copyOfPair.first[0] << std::endl;
}
{
std::vector<std::intmax_t> originalVector{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
const auto copyOfVector = Copy(originalVector);
std::cout << copyOfVector[0] << std::endl;
std::array<std::intmax_t, 10> originalArray{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
const auto copyOfArray = Copy(originalArray);
std::cout << copyOfArray[0] << std::endl;
auto originalPair = std::make_pair(std::vector<std::intmax_t>{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }, std::array<std::intmax_t, 10>{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
const auto [first, second] = Copy(originalPair);
const auto copyOfPair = Copy(originalPair);
std::cout << first[0] << std::endl;
std::cout << copyOfPair.first[0] << std::endl;
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If necessary, please create a small test project for Coverity tests. Thank you.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for doing those Coverity tests, @issakomi In the mean time, I did further improve the unit tests. Especially by introducing a local function Thanks for suggesting to do my own Coverity tests, I think I'll do so later. But I think the MSVC warnings are quite similar, and I already have MSVC installed 😃.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for the suggestion of a special repository for Coverity tests is that I have to put a code somewhere in main.cpp of my app and upload to Coverity scan (scans are limited). It is rather easy (github login can be used for scan.coverity.com), but a project have to be approved. Short example on Linux:
There are many options for automation, of course. There is also an option of "modeling" to suppress false-positives, but it seems to be a little over-complicated. Probably commercial customers can do scan locally, not sure. |
||
| { | ||
| std::vector<std::intmax_t> originalVector{ 1, 2, 3, 4 }; | ||
|
|
||
| // Without `itk::Copy`, the statement `const auto copyOfVector = GetReference(originalVector);` might trigger a | ||
| // warning, saying something like "`auto` doesn't deduce references. A possibly unintended copy is being made" (MSVC | ||
| // 2022 IntelliSense Code Linter). | ||
| const auto copyOfVector = itk::Copy(GetReference(originalVector)); | ||
|
|
||
| // Clear the original (not the "copy"). | ||
| originalVector = {}; | ||
|
|
||
| // In this case, it is essential that the "copy" is not just a reference to the original, because the value of the | ||
| // original object is modified after that copy took place. This modification should not affect the copy. | ||
| ASSERT_NE(originalVector, copyOfVector); | ||
|
|
||
| // A similar test for the equivalent `std::array`: | ||
| std::array<std::intmax_t, 4> originalArray{ 1, 2, 3, 4 }; | ||
|
|
||
| // MSVC 2022 Warning C26820 ("This is a potentially expensive copy operation. Consider using a reference unless a copy | ||
| // is required.") is not raised for types whose size isn't more than twice the platform-dependent pointer size, | ||
| // according to https://learn.microsoft.com/en-us/cpp/code-quality/c26820?view=msvc-170 | ||
| static_assert( | ||
| sizeof(originalArray) > 2 * sizeof(void *), | ||
| "The array must be big enough to _possibly_ trigger compiler warnings about the expensiveness of copying!"); | ||
|
|
||
| const auto copyOfArray = itk::Copy(GetReference(originalArray)); | ||
|
|
||
| // Reset the elements of the original array (not the "copy"). | ||
| originalArray = {}; | ||
|
|
||
| // The original and the copy should be different now, because the copy is not reset. | ||
| ASSERT_NE(originalArray, copyOfArray); | ||
|
|
||
| // Sanity check: the copy of the vector and the copy of the array still have the same elements. | ||
| EXPECT_TRUE(std::equal(copyOfVector.cbegin(), copyOfVector.cend(), copyOfArray.cbegin(), copyOfArray.cend())); | ||
|
|
||
| // The following demonstates an `itk::Copy` use case when having structured bindings: | ||
| auto originalPair = | ||
| std::make_pair(std::vector<std::intmax_t>{ 1, 2, 3, 4 }, std::array<std::intmax_t, 4>{ 1, 2, 3, 4 }); | ||
| const auto [first, second] = itk::Copy(GetReference(originalPair)); | ||
| const auto copyOfPair = itk::Copy(GetReference(originalPair)); | ||
| originalPair = {}; | ||
| ASSERT_NE(originalPair, copyOfPair); | ||
|
|
||
| EXPECT_EQ(copyOfPair, std::make_pair(first, second)); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.