From 942ca36db72546672c0054d2e0a7dacefe9ae376 Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Wed, 3 Jan 2024 18:22:38 +0100 Subject: [PATCH] ENH: Add `itk::Copy(const T & original)`, which simply returns a copy Intended to be used to make the act of copying 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", https://rules.sonarsource.com/cpp/RSPEC-6032/ --- .github/workflows/itk_dict.txt | 4 + Modules/Core/Common/include/itkCopy.h | 59 +++++++++++ Modules/Core/Common/test/CMakeLists.txt | 1 + Modules/Core/Common/test/itkCopyGTest.cxx | 116 ++++++++++++++++++++++ 4 files changed, 180 insertions(+) create mode 100644 Modules/Core/Common/include/itkCopy.h create mode 100644 Modules/Core/Common/test/itkCopyGTest.cxx diff --git a/.github/workflows/itk_dict.txt b/.github/workflows/itk_dict.txt index e5628ab6f69..5a69dae6770 100644 --- a/.github/workflows/itk_dict.txt +++ b/.github/workflows/itk_dict.txt @@ -37,6 +37,7 @@ Conners ConvertAnitkImageTovtkImageData ConvertRGBvtkImageDataToAnitkImage Cortesi's +Coverity Creade CreateListOfSamplesWithIDs Crofton's @@ -94,6 +95,7 @@ Hilden Hiraki Hirschberg Hostname +Intelli IOregion ITKIOMeshGifti ITKImageToIplImage @@ -198,6 +200,7 @@ Spall Sprengel Stegun Stiehl +Synopsys Syst Tajbakhsh Talos @@ -587,6 +590,7 @@ lin lina lins lm +lnt loc localizer lockings diff --git a/Modules/Core/Common/include/itkCopy.h b/Modules/Core/Common/include/itkCopy.h new file mode 100644 index 00000000000..ff7dcc1aadc --- /dev/null +++ b/Modules/Core/Common/include/itkCopy.h @@ -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 +[[nodiscard]] constexpr T +Copy(const T & original) +{ + // May call the copy-constructor of `T`. + return original; +} + +} // namespace itk + + +#endif // itkCopy_h diff --git a/Modules/Core/Common/test/CMakeLists.txt b/Modules/Core/Common/test/CMakeLists.txt index e3dd04c8c04..0ce48a5db76 100644 --- a/Modules/Core/Common/test/CMakeLists.txt +++ b/Modules/Core/Common/test/CMakeLists.txt @@ -1718,6 +1718,7 @@ set(ITKCommonGTests itkBuildInformationGTest.cxx itkConnectedImageNeighborhoodShapeGTest.cxx itkConstantBoundaryImageNeighborhoodPixelAccessPolicyGTest.cxx + itkCopyGTest.cxx itkDerefGTest.cxx itkExceptionObjectGTest.cxx itkFixedArrayGTest.cxx diff --git a/Modules/Core/Common/test/itkCopyGTest.cxx b/Modules/Core/Common/test/itkCopyGTest.cxx new file mode 100644 index 00000000000..d5be421afe7 --- /dev/null +++ b/Modules/Core/Common/test/itkCopyGTest.cxx @@ -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 + +#include +#include // For intmax_t. +#include +#include // For pair. +#include + + +// 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 +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(3, getRandomNumber())); + check(std::array{ 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) +{ + std::vector 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 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{ 1, 2, 3, 4 }, std::array{ 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)); +}