Skip to content

Commit

Permalink
ENH: Add itk::Copy(const T & original), which simply returns a copy
Browse files Browse the repository at this point in the history
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/
  • Loading branch information
N-Dekker authored and dzenanz committed Jan 12, 2024
1 parent c8ec91c commit b2dacec
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 0 deletions.
4 changes: 4 additions & 0 deletions .github/workflows/itk_dict.txt
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Conners
ConvertAnitkImageTovtkImageData
ConvertRGBvtkImageDataToAnitkImage
Cortesi's
Coverity
Creade
CreateListOfSamplesWithIDs
Crofton's
Expand Down Expand Up @@ -94,6 +95,7 @@ Hilden
Hiraki
Hirschberg
Hostname
Intelli
IOregion
ITKIOMeshGifti
ITKImageToIplImage
Expand Down Expand Up @@ -198,6 +200,7 @@ Spall
Sprengel
Stegun
Stiehl
Synopsys
Syst
Tajbakhsh
Talos
Expand Down Expand Up @@ -587,6 +590,7 @@ lin
lina
lins
lm
lnt
loc
localizer
lockings
Expand Down
59 changes: 59 additions & 0 deletions Modules/Core/Common/include/itkCopy.h
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
1 change: 1 addition & 0 deletions Modules/Core/Common/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,7 @@ set(ITKCommonGTests
itkBuildInformationGTest.cxx
itkConnectedImageNeighborhoodShapeGTest.cxx
itkConstantBoundaryImageNeighborhoodPixelAccessPolicyGTest.cxx
itkCopyGTest.cxx
itkDerefGTest.cxx
itkExceptionObjectGTest.cxx
itkFixedArrayGTest.cxx
Expand Down
116 changes: 116 additions & 0 deletions Modules/Core/Common/test/itkCopyGTest.cxx
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(&copy, &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)
{
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));
}

0 comments on commit b2dacec

Please sign in to comment.