Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

Add std namespace prefix in C++ code #397

Merged
merged 11 commits into from
Dec 15, 2020
Merged

Add std namespace prefix in C++ code #397

merged 11 commits into from
Dec 15, 2020

Conversation

beenje
Copy link
Contributor

@beenje beenje commented Dec 10, 2020

Allow to compile with TANGO_USE_USING_NAMESPACE=OFF in cppTango.

Add std:: prefix to:

  • cout
  • cerr
  • endl
  • ends
  • vector
  • string
  • bad_alloc
  • numeric_limits
  • max
  • auto_ptr

Should std::auto_ptr be replaced with unique_pointer instead?

@ajoubertza
Copy link
Member

Hi @beenje

Welcome to PyTango! Thanks for the PR. It seems like a good idea. Looking at the related tickets, were you trying to compile PyTango on MacOS? That would be nice.

Should std::auto_ptr be replaced with unique_pointer instead?

I've never noticed that before, but yes, it looks like the intention is to use unique_pointer everywhere except ext/defs.h, so please fix it.

ajoubertza
ajoubertza reviewed Dec 11, 2020
View reviewed changes
ext/exception.cpp Outdated
@@ -58,7 +58,7 @@ namespace Tango

void sequencePyDevError_2_DevErrorList(PyObject *value, Tango::DevErrorList &del)
{
long len = max((int)PySequence_Size(value), 0);
long len = std::max((int)PySequence_Size(value), 0);
Copy link
Member

Choose a reason for hiding this comment

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

I see the AppVeyor build is failing here - the Visual Studio compiler doesn't like this. Any idea why? Can we leave this one out, or do we need an #ifdef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an old answer but from https://stackoverflow.com/a/2789509, it looks like we need to use #define NOMINMAX for windows.
I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a new commit that should fix the build on windows.

@beenje
Fix build on windows
ac564fa 
Prevent macro invocation (max macro defined in windows.h).
See https://stackoverflow.com/a/2789509
@beenje
Copy link
Contributor Author

beenje commented Dec 14, 2020

I tried to replace the std::auto_ptr with unique_ptr but the build fails.

    /tmp/pip-req-build-f94gz_85/ext/group.cpp:200:41:   required from here
    /opt/conda/envs/tango/include/boost/python/converter/as_to_python_function.hpp:40:33: error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = Tango::Group; _Dp = std::default_delete<Tango::Group>]'

I kept auto_ptr in ext/group.cpp to test but it still fails with unique_ptr in ext/server/device_class.cpp:

    /tmp/pip-req-build-uppobmhw/ext/server/device_class.cpp:470:97:   required from here
    /opt/conda/envs/tango/include/boost/python/converter/implicit.hpp:37:9: error: no matching function for call to 'std::unique_ptr<CppDeviceClass>::unique_ptr(std::unique_ptr<CppDeviceClassWrap>&)'
       37 |         new (storage) Target(get_source());
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    In file included from /opt/conda/envs/tango/x86_64-conda-linux-gnu/include/c++/9.3.0/memory:80,
                     from /opt/conda/envs/tango/include/boost/function/function_base.hpp:16,
                     from /opt/conda/envs/tango/include/boost/function/detail/prologue.hpp:17,
                     from /opt/conda/envs/tango/include/boost/function/function_template.hpp:13,
                     from /opt/conda/envs/tango/include/boost/function/detail/maybe_include.hpp:15,
                     from /opt/conda/envs/tango/include/boost/function/function0.hpp:11,
                     from /opt/conda/envs/tango/include/boost/python/errors.hpp:13,
                     from /opt/conda/envs/tango/include/boost/python/handle.hpp:11,
                     from /opt/conda/envs/tango/include/boost/python/args_fwd.hpp:10,
                     from /opt/conda/envs/tango/include/boost/python/args.hpp:10,
                     from /opt/conda/envs/tango/include/boost/python.hpp:11,
                     from /tmp/pip-req-build-uppobmhw/ext/precompiled_header.hpp:20,
                     from /tmp/pip-req-build-uppobmhw/ext/server/device_class.cpp:12:
    /opt/conda/envs/tango/x86_64-conda-linux-gnu/include/c++/9.3.0/bits/unique_ptr.h:281:2: note: candidate: 'template<class _Up, class> std::unique_ptr<_Tp, _Dp>::unique_ptr(std::auto_ptr<_Up>&&)'
      281 |  unique_ptr(auto_ptr<_Up>&& __u) noexcept;
          |  ^~~~~~~~~~
    /opt/conda/envs/tango/x86_64-conda-linux-gnu/include/c++/9.3.0/bits/unique_ptr.h:281:2: note:   template argument deduction/substitution failed:
    In file included from /opt/conda/envs/tango/include/boost/python/implicit.hpp:10,
                     from /opt/conda/envs/tango/include/boost/python.hpp:33,
                     from /tmp/pip-req-build-uppobmhw/ext/precompiled_header.hpp:20,
                     from /tmp/pip-req-build-uppobmhw/ext/server/device_class.cpp:12:
    /opt/conda/envs/tango/include/boost/python/converter/implicit.hpp:37:9: note:   'std::unique_ptr<CppDeviceClassWrap>' is not derived from 'std::auto_ptr<T>'
       37 |         new (storage) Target(get_source());

Any idea?

@ajoubertza
Copy link
Member

I tried to replace the std::auto_ptr with unique_ptr but the build fails.
...
Any idea?

No, I'm not sure why. It looks like the boost library is still using auto_ptr for those conversions. Maybe that's why those bits of the PyTango code don't use the unique_ptr typedef. My C++ is very rusty, so I had a look here - the two types are not 100% compatible.

I'm happy if we just leave the std::auto_ptrs as they are. Changing to std::unique_ptr is outside the scope of this PR.

ajoubertza
ajoubertza approved these changes Dec 14, 2020
View reviewed changes
Copy link
Member

@ajoubertza ajoubertza left a comment

Choose a reason for hiding this comment

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

Looks good.

ext/exception.cpp
@@ -58,7 +58,7 @@ namespace Tango

void sequencePyDevError_2_DevErrorList(PyObject *value, Tango::DevErrorList &del)
{
long len = std::max((int)PySequence_Size(value), 0);
long len = (std::max)((int)PySequence_Size(value), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I prefer this approach to messing with the include files.

@ajoubertza ajoubertza changed the title Add std namespace prefix Add std namespace prefix in C++ code Dec 14, 2020
@ajoubertza
Copy link
Member

@beenje Is there anything else you still want to add to this PR, or can I merge it?

@beenje
Copy link
Contributor Author

beenje commented Dec 14, 2020

If we don't touch the auto_ptr, I don't have anything else to add. It should be good to merge as is.

@ajoubertza ajoubertza merged commit e93e5df into tango-controls:develop Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ajoubertza ajoubertza ajoubertza approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@beenje @ajoubertza