Add nostd::make_unique

[owent]

Fixes #3096

Changes

• Add nostd::make_unique

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[netlify]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	0b23c70
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/670ce8a259ce9a0008fa8439

[marcalff]

This opens a whole new can of worms (see CI), because now data_ is a std::shared_ptr and the code wants to add an nostd::unique_ptr to it, instead of a std::unique_ptr.

How about a simpler solution without new nostd helpers, like:

data_->Add(std::unique_ptr<ResourceMetrics>(new ResourceMetrics(data)));

[owent]

Thanks, I modified the parameter type of this API but kept std::unique_ptr in the exported APIs for ABI compatibility.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (497eaf4) to head (0b23c70).
Report is 141 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3097      +/-   ##
==========================================
+ Coverage   87.12%   87.86%   +0.74%     
==========================================
  Files         200      195       -5     
  Lines        6109     5986     -123     
==========================================
- Hits         5322     5259      -63     
+ Misses        787      727      -60     

Files with missing lines	Coverage Δ
api/include/opentelemetry/nostd/unique_ptr.h	100.00% <100.00%> (ø)
...de/opentelemetry/exporters/memory/in_memory_data.h	100.00% <100.00%> (ø)
...telemetry/exporters/memory/in_memory_metric_data.h	100.00% <ø> (ø)
...entelemetry/exporters/memory/in_memory_span_data.h	100.00% <100.00%> (ø)
exporters/memory/src/in_memory_metric_data.cc	85.00% <100.00%> (ø)
...rs/memory/src/in_memory_metric_exporter_factory.cc	100.00% <100.00%> (ø)
...xporters/memory/test/in_memory_metric_data_test.cc	100.00% <100.00%> (ø)
...clude/opentelemetry/sdk/common/atomic_unique_ptr.h	100.00% <100.00%> (ø)
...include/opentelemetry/sdk/common/circular_buffer.h	100.00% <100.00%> (ø)

... and 122 files with indirect coverage changes

[lalitb]

I'm a bit puzzled—why are we reintroducing support for building with GCC 4.8 and C++11 when we've already moved on to supporting C++14 and above? Or may be I am missing something :)

[marcalff]

Instead of reintroducing nostd helpers for code in the sdk already using std::unique_ptr, I think it will be better to settle on a subset of C++ 14.

Let's call itC++13.9 ... almost as C++14, but we restrict ourselves to not use std::make_unique, so that it can also build in C++11.

There are 4 lines to fix in the entire code base, so that:

data_->Add(std::make_unique<ResourceMetrics>(data));

needs to change to

data_->Add(std::unique_ptr<ResourceMetrics>(new ResourceMetrics(data)));

and then very old toolchains will still work.

While C++11 is no longer officially supported, we should not break it if it is easy (4 lines) to avoid.

@owent would this work for you ?

[marcalff]

Previous discussion on C++11 / C++14:

#2522 (comment)

[ThomsonTan]

Yes, prefer not introducing nostd::make_unique as the C++ 14 is already the minimum requirement. And we also don't want to declare that it is compatible with std::make_unique in C++ 14 and above, like nostd::make_unique(nostd::size_t size) is not supported. So could outline the current code like @marcalff mentioned, or give it some other utility name if it is really necessary. This way makes it more flexible than polyfill a standard library method.

[owent]

Instead of reintroducing nostd helpers for code in the sdk already using std::unique_ptr, I think it will be better to settle on a subset of C++ 14.

Let's call itC++13.9 ... almost as C++14, but we restrict ourselves to not use std::make_unique, so that it can also build in C++11.

There are 4 lines to fix in the entire code base, so that:

data_->Add(std::make_unique<ResourceMetrics>(data));

needs to change to

data_->Add(std::unique_ptr<ResourceMetrics>(new ResourceMetrics(data)));

and then very old toolchains will still work.

While C++11 is no longer officially supported, we should not break it if it is easy (4 lines) to avoid.

@owent would this work for you ?

Yes, it works. But I wonder if it's necessary to keep nostd::unique_ptr<T> if we always depend std::unique_ptr ?

I raised #3098 to replace std::make_unique only. And this PR can be closed if we use #3098 .

[marcalff]

Yes, it works. But I wonder if it's necessary to keep nostd::unique_ptr<T> if we always depend std::unique_ptr ?

I raised #3098 to replace std::make_unique only. And this PR can be closed if we use #3098 .

In my understanding, nostd::unique_ptr<T> is now always implemented using std::unique_ptr internally (because we use C++ >= 11), but the prototype in the ABI only refers to nostd::unique_ptr<T>.

If we change the code in the API to replace nostd with std, the methods signatures will change, breaking the ABI.

[marcalff]

Closed in favor of alternate solution, implemented in #3098