Use c++11 smart pointer #42

Manually merged
obache merged 2 commits from feat/replace-smart-pointer-type into master 4 months ago
obache commented 4 months ago
Collaborator

C++11 is allowed,

  • no need to use boost for shared_ptr
  • use unique_ptr instead of deprecated auto_ptr
C++11 is allowed, * no need to use boost for `shared_ptr` * use `unique_ptr` instead of deprecated `auto_ptr`
obache added 2 commits 4 months ago
obache force-pushed feat/replace-smart-pointer-type from 26c0d5cda6 to 0d168f7ecd 4 months ago
obache merged commit 0d168f7ecd into master manually 4 months ago
Owner

It looks good. It was necessary to add CXX_FEATURES to make sure c++11 is selected on some old distros where gcc supports c++11 but not as default choice. I have pushed a commit on top of yours for that.
I will wait for @SlavekB to test building in some old distros before merging the PR.

It looks good. It was necessary to add CXX_FEATURES to make sure c++11 is selected on some old distros where gcc supports c++11 but not as default choice. I have pushed a commit on top of yours for that. I will wait for @SlavekB to test building in some old distros before merging the PR.
Owner

I will wait for @SlavekB to test building in some old distros before merging the PR.

Duh!! I mistakenly merged already :-(

@SlavekB I don't expect problems but before I backport this to R14.0.x could you do a build test on Xenial?

> I will wait for @SlavekB to test building in some old distros before merging the PR. Duh!! I mistakenly merged already :-( @SlavekB I don't expect problems but before I backport this to R14.0.x could you do a build test on Xenial?
MicheleC requested review from SlavekB 4 months ago
SlavekB approved these changes 4 months ago
SlavekB left a comment
Owner

I did the test on Xenial, added one more commit for ensure at least C++11 and made a backport to r14.0.x branch.

I did the test on Xenial, added one more commit for ensure at least C++11 and made a backport to r14.0.x branch.
SlavekB deleted branch feat/replace-smart-pointer-type 4 months ago
SlavekB added this to the R14.0.12 release milestone 4 months ago
Owner

I did the test on Xenial, added one more commit for ensure at least C++11 and made a backport to r14.0.x branch.

Thanks! That is what I meant for originally, because there could have been a chance of missing CXX_FEATURES somewhere else that could be detected by building. Unfortunately, due to something I have to fix on my side, I pushed to master instead of the branch :-(.

> I did the test on Xenial, added one more commit for ensure at least C++11 and made a backport to r14.0.x branch. Thanks! That is what I meant for originally, because there could have been a chance of missing CXX_FEATURES somewhere else that could be detected by building. Unfortunately, due to something I have to fix on my side, I pushed to master instead of the branch :-(.
Poster
Collaborator

From TDE/tde#73, I'm using nullptr in this PR, but is it not allowed by default yet?

From TDE/tde#73, I'm using `nullptr` in this PR, but is it not allowed by default yet?
Owner

c++11 is allowed. We just need to add CXX_FEATURES in cmake files to make sure c++11 standard is selected by the compiler if it is not the default. This only affects old distros like Ubuntu Xenial, where gcc supports c++11 fully but does not set it as default.
So feel free to use any c++11 feature you wish :-)

c++11 is allowed. We just need to add CXX_FEATURES in cmake files to make sure c++11 standard is selected by the compiler if it is not the default. This only affects old distros like Ubuntu Xenial, where gcc supports c++11 fully but does not set it as default. So feel free to use any c++11 feature you wish :-)
Poster
Collaborator

CMAKE_CXX_STANDARD is set as 11 globally? or mixed std usage is troublesome.

`CMAKE_CXX_STANDARD` is set as `11` globally? or mixed `std` usage is troublesome.
Owner

The CMAKE_CXX_STANDARD setting is a way that we do not want to use. The setting in this way causes to enforce the standard, regardless of whether the C++ compiler has the desired standard as a default and above all, regardless of whether it has as a default newer standard!

Therefore, we chose the way of setting the required C++ features, for example cxx_nullptr, cxx_range_for,… when CMake decides what version of C++ standard is necessary and whether it is necessary to add the desired build option or not.

If we already want to focus on move forward in connection with TDE/tde#73, it will be appropriate to set TDE_CXX_FEATURES at the level of common TDE CMake module, instead of setting cxx features individually for libraries or applications. However, in this regard, we probably should first complete the conversion automake => cmake for the remaining modules.

The `CMAKE_CXX_STANDARD` setting is a way that we do not want to use. The setting in this way causes to enforce the standard, regardless of whether the C++ compiler has the desired standard as a default and above all, regardless of whether it has as a default newer standard! Therefore, we chose the way of setting the required C++ features, for example `cxx_nullptr`, `cxx_range_for`,… when CMake decides what version of C++ standard is necessary and whether it is necessary to add the desired build option or not. If we already want to focus on move forward in connection with TDE/tde#73, it will be appropriate to set `TDE_CXX_FEATURES` at the level of common TDE CMake module, instead of setting cxx features individually for libraries or applications. However, in this regard, we probably should first complete the conversion automake => cmake for the remaining modules.
Owner

f we already want to focus on move forward in connection with TDE/tde#73, it will be appropriate to set TDE_CXX_FEATURES at the level of common TDE CMake module, instead of setting cxx features individually for libraries or applications. However, in this regard, we probably should first complete the conversion automake => cmake for the remaining modules.

Completing conversion of automake to cmake first would definitely be good, but I think that will take a while. Instead I can see c++11 features being used more and more quickly, so I think we should go ahead and set CXX_FEATURES at common level in TDE.

> f we already want to focus on move forward in connection with TDE/tde#73, it will be appropriate to set TDE_CXX_FEATURES at the level of common TDE CMake module, instead of setting cxx features individually for libraries or applications. However, in this regard, we probably should first complete the conversion automake => cmake for the remaining modules. Completing conversion of automake to cmake first would definitely be good, but I think that will take a while. Instead I can see c++11 features being used more and more quickly, so I think we should go ahead and set CXX_FEATURES at common level in TDE.
Owner

CXX_FEATURES have been enabled at TDE level now. There is no longer any need to set them for individual modules or libraries.

CXX_FEATURES have been enabled at TDE level now. There is no longer any need to set them for individual modules or libraries.

Reviewers

SlavekB approved these changes 4 months ago
The pull request has been manually merged as 0d168f7ecd.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.