R.5 (scoped objects): Do not warn on a const `unique_ptr<T[]>`

[N-Dekker]

unique_ptr<T[]> is being used in practice to declare a local dynamically allocated buffer, for example:

const auto local_buffer = std::make_unique_for_overwrite<int[]>(size);

Especially when the unique_ptr is declared "const", a warning that it is not moved, copied, reassigned or reset does not appear suitable.

---

For the record, here is a complete code example, using such a local Unique_pointer (not included with the proposed commit):

int get_median_value(const std::list<int>& integers)
{
  const auto size = integers.size();
  const auto local_buffer = std::make_unique_for_overwrite<int[]>(size);

  std::copy_n(begin(integers), size, local_buffer.get());
  std::nth_element(local_buffer.get(), local_buffer.get() + size/2, local_buffer.get() + size);
  
  return local_buffer[size/2];
}

[N-Dekker]

Just for clarification, this pull request addresses the enforcement rule, which says:

(Simple) Warn if a local Unique_pointer or Shared_pointer is not moved, copied, reassigned or reset before its lifetime ends.

[Xeverous]

I think the way this rule is worded covers much greater set of situations than it should. One such situation has just been presented with unbounded arrays.

Another: what if you need to access some information and the only way to obtain it is to call a function which returns a unique pointer? Basically: obtain a handle, read stuff, let the handle die.

[hsutter]

Editors call: We agree, thanks. Could you please make two changes to your PR:

1. Change the enforcement rule to just add the word "non-const":

Warn if a local non-const Unique_pointer [...]

2. Please add the succinct explanation and your example in a new "Exception: ... Example: ..." in the body of the Guideline/

Thanks!

[N-Dekker]

Editors call: We agree, thanks.

@hsutter Hi Herb, thank you very much for your encouragement! I have recently been sprinkling around a lot of local const unique_ptr<T[]> variables (e.g., https://github.com/InsightSoftwareConsortium/ITK/commit/15d09d6c756b46b4d1c296d8bb13b85f82654ef9), replacing lots of manual new[]s and delete[]s, and I would very much like these changes to be in accordance with the C++ Core Guidelines!

1. Change the enforcement rule to just add the word "non-const":

Warn if a local non-const Unique_pointer [...]

Sorry, I think I overemphasized the "const" aspect, when I submitted the PR! The original warning still appears relevant for a local const unique_ptr<T> variable, initialized by make_unique or make_unique_for_overwrite. For example, the following code should still trigger a warning, as the file stream should be declared as a local object, not a heap object, in this case:

void write_hello()
{
    const auto const_ptr = std::make_unique<std::ofstream>("hello.txt"); // Bad! Please warn me! 
    (*const_ptr) << "Hello!\n";
}

So I would rather not add the term non-const to this enforcement rule. I hope you agree.

2. Please add the succinct explanation and your example in a new "Exception: ... Example: ..." in the body of the Guideline/

Sure, I'd be glad to add the get_median_value example to the proposed text itself. I'll prepare an amend + force-push. Coming soon...

[cubbimew]

sprinkling around a lot

Sounds like an opportunity to privately resurrect std::dynarray

[N-Dekker]

Sounds like an opportunity to privately resurrect std::dynarray

@cubbimew Thanks for the suggestion Sergey, but personally I'd rather have it resurrected publicly 😃 Until then, I think we should be able to use local const unique_ptr<T[]> variables without a warning.

[bgloyer]

This is a good change. Another exception would be using large objects that you don't want local:

const auto huge = make_unique<MassiveLegacyObject>(3);

Basing the enforement around 'const unique_ptr' seems to be a good general case. In practice, I don't think this is a common problem with unique_ptr's. It is more likely to happen with raw new (like is shown in the example) with developers comming from other languages.

[N-Dekker]

This is a good change. Another exception would be using large objects that you don't want local:

Thanks @bgloyer I got your point. An object might be too large to be placed on the stack. However, I'm still in favor of having this particular pull request specifically about scoped unbounded arrays, as it appears a very common use case for local unique_ptrs. (Much more common than the use case for such a large scoped object, as far as I have seen.) We already have an agreement about adding an exception for scoped unbounded arrays, as Herb Sutter wrote at https://github.com/isocpp/CppCoreGuidelines/pull/1969#issuecomment-1255555334 I think it would be more difficult to get an agreement on an exception for "large" objects, especially because it's hard to specify in a generic way when exactly an object would be too large to be placed on the stack. I would be in favor of deferring that to another PR (or issue).

[hsutter]

Editors call: Looks good, thank you!