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

ENH: Add itk::Copy(const T & original), which simply returns a copy #4383

Merged
merged 1 commit into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
N-Dekker marked this conversation as resolved.
Show resolved Hide resolved
* 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)
Comment on lines +69 to +70
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@issakomi Can you please tell us if the proposed itk::Copy would indeed silence those Coverity AUTO_CAUSES_COPY warnings? Specifically, is this AllowsUsingAutoKeywordCopyingWithoutWarnings unit test indeed warning-free? And do those AUTO_CAUSES_COPY appear when the itk::Copy calls in there are removed?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@N-Dekker N-Dekker Jan 3, 2024

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@issakomi I just extended the AllowsUsingAutoKeywordCopyingWithoutWarnings to include structured bindings. Hope you can also try that out on Coverty. Specifically:

  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 itk::Copy call here suppress another AUTO_CAUSES_COPY warning? Would the AUTO_CAUSES_COPY warning occur without the itk::Copy call (when simply doing const auto [first, second] = originalPair)?

I did already observe that the itk::Copy call in the context of structured bindings suppresses such warnings from Intellisense/Linter.

Copy link
Member

Choose a reason for hiding this comment

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

And it's also harder to ignore these specific warnings now that they come from at least three different tools

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.
BTW, maybe just create a test project for Coverity tests? Any opensource project can be scanned at https://scan.coverity.com. A full ITK scan will generate a ton of defects, mostly from third party modules.

Copy link
Member

@issakomi issakomi Jan 4, 2024

Choose a reason for hiding this comment

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

I did already observe that the itk::Copy call in the context of structured bindings suppresses such warnings from Intellisense/Linter.

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;
}

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@N-Dekker N-Dekker Jan 5, 2024

Choose a reason for hiding this comment

The 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 GetReference (just for the tests). Because I realized that those warnings appear specifically when there is a reference at the right hand side (rhs). And that's exactly when itk::Copy(rhs) should be useful.

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 😃.

Copy link
Member

@issakomi issakomi Jan 6, 2024

Choose a reason for hiding this comment

The 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:

  • download Coverity archive, unpack in home directory
  • export PATH=$HOME/cov-analysis-linux64-2023.6.2/bin:$PATH
  • configure something like mypoject/coverity-build and cd mypoject/coverity-build
  • cov-build --dir cov-int make -j6
  • tar cf cov-int.tar cov-int
  • gzip cov-int.tar
  • upload cov-int.tar.gz to scan portal (AFAIK, the name must always be cov-int)
  • wait for result of the scan

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));
}