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

[N-Dekker]

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/

[N-Dekker]

@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?

[issakomi]

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.

[N-Dekker]

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

[N-Dekker]

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

[issakomi]

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.

[issakomi]

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

[issakomi]

If necessary, please create a small test project for Coverity tests. Thank you.

[N-Dekker]

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

[issakomi]

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.

[N-Dekker]

For your information, a similar warning is produced by SonarSource, https://rules.sonarsource.com/cpp/RSPEC-6032/ as mentioned by Dvir Yitzchaki at the ACCU mailing list. Dvir posted an example at https://godbolt.org/z/ETdsYT3Ph

[N-Dekker]

/azp run docs/readthedocs.org:itk

[N-Dekker]

I think this PR is ready, only docs/readthedocs.org:itk is still "Pending — Read the Docs build is in progress!"