Core/enable cxx17

[roigcarlo]

DO NOT MERGE

Draft of changes for C++17. Wip. This PR is to address some problems observed in #9916 mostly regarding deprecation of features towards C++20.

List of current changes:

• All unitary_function and binary_function instances have been removed from the code.

• The return_type from the functions above has been replaced with decltype() when necessary

• Current instances of code that already used decltype() using an object have been replaced with the a std::declval of that object to avoid creating an instance:

  decltype(TDataType)
  

  Changes to:

  decltype(std::declval<TDataType>())
  

  This is specially important because of Dofs in model_part.h:

  typedef PointerVectorSet<DofType,
          SetIdentityFunction<DofType>,
          std::less<SetIdentityFunction<DofType>>::result_type,
          std::equal_to<SetIdentityFunction<DofType>>::result_type,
          DofType* > DofsArrayType;
  

  Changes to:

  typedef PointerVectorSet<DofType,
          SetIdentityFunction<DofType>,
          std::less<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
          std::equal_to<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
          DofType* > DofsArrayType;
  

  which has undermined some inconsistencies with DofType in several core classes (For instance scheme.h):

  /// DoF iterator type definition
  typedef typename PointerVectorSet<TDofType, IndexedObject>::iterator DofIterator;
  /// DoF constant iterator type definition
  typedef typename PointerVectorSet<TDofType, IndexedObject>::const_iterator DofConstantIterator;
  

  Should be defined as:

  /// DoF iterator type definition
  typedef typename PointerVectorSet<TDofType, 
          std::less<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
          std::equal_to<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
          DofType* >::iterator DofIterator;
  /// DoF constant iterator type definition
  typedef typename PointerVectorSet<TDofType, 
          std::less<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
          std::equal_to<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
          DofType* >::const_iterator DofConstantIterator;
  

  Otherwise it forces the PointerVectorSet of some classes to work with DofType::Pointer instead of DofType* Which could potentially have an impact in performance. Also there seems to be an inconsistency on whether to use IndexedObject or SetIdentityFunction for the Key/Hash of the PointerVectorSet<DofType>. I asume IndexedObject is the correct approach since otherwise the sorting function fails ( seems that only RomApplication was sorting dofs...)

[matekelemen]

typedef PointerVectorSet<DofType,
     SetIdentityFunction<DofType>,
     std::less<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
     std::equal_to<decltype(std::declval<IndexedObject>()(std::declval<DofType>()))>,
     DofType* > DofsArrayType;

IndexObject::operator() is a template and (as far as I can tell) has no specializations, so why not use the member alias it uses for its return type?

typedef PointerVectorSet<DofType,
        SetIdentityFunction<DofType>,
        std::less<IndexObject::IndexType>,
        std::equal_to<IndexObject::IndexType>,
        DofType* > DofsArrayType;

[roigcarlo]

Ok :+1: I still prefer the other because it helps to send the idea that the type should be deducible, at least until we remove the SetIdentityFunction that we use in other instantiations, but I agree that using the return type directly is clearer.

Let me do some more work on this in the meantime, as it seems that It still has problems. As I commented above there were several miss usages of this class where the Dof itself was being used as a key ( and I am still not sure if I broke the BS or the Test with the changes). Once I have a final working version I will replace all of the possible decltype

[roigcarlo]

@loumalouomega or @pooyan-dadvand

Any idea about this error:

/home/roigcarlo/Kratos/kratos/processes/from_json_check_result_process.cpp:584:81: error: no matching function for call to ‘distance(Kratos::Parameters::const_iterator, Kratos::Parameters::const_iterator)’
  584 |     auto size_parameters =[](const Parameters& rParameters){return std::distance(rParameters.begin(), rParameters.end());};
      |                                                                    ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/12.1.0/string:46,
                 from /home/roigcarlo/Kratos/kratos/containers/model.h:18,
                 from /home/roigcarlo/Kratos/kratos/processes/from_json_check_result_process.cpp:18:
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h:146:5: note: candidate: ‘template<class _InputIterator> constexpr typename std::iterator_traits< <template-parameter-1-1> >::difference_type std::distance(_InputIterator, _InputIterator)’
  146 |     distance(_InputIterator __first, _InputIterator __last)
      |     ^~~~~~~~
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h:146:5: note:   template argument deduction/substitution failed:
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h: In substitution of ‘template<class _InputIterator> constexpr typename std::iterator_traits< <template-parameter-1-1> >::difference_type std::distance(_InputIterator, _InputIterator) [with _InputIterator = Kratos::Parameters::const_iterator_adaptor]’:
/home/roigcarlo/Kratos/kratos/processes/from_json_check_result_process.cpp:584:81:   required from here
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h:146:5: error: no type named ‘difference_type’ in ‘struct std::iterator_traits<Kratos::Parameters::const_iterator_adaptor>’

I added those to the definition of both const_iterator and iterator in the Parameters class:

using iterator_category = std::forward_iterator_tag;
using difference_type   = std::ptrdiff_t;
using value_type        = Parameters;
using pointer           = Parameters*;
using reference         = Parameters&;

[matekelemen]

Just a guess, but the pointer and reference member aliases of const_iterator_adaptor probably need to inherit the constness:

using pointer = const Parameters*;
using reference = const Parameters&;

[roigcarlo]

@matekelemen mm unfortunately no :S, but thx for the suggestion

[loumalouomega]

@loumalouomega or @pooyan-dadvand

Any idea about this error:

/home/roigcarlo/Kratos/kratos/processes/from_json_check_result_process.cpp:584:81: error: no matching function for call to ‘distance(Kratos::Parameters::const_iterator, Kratos::Parameters::const_iterator)’
  584 |     auto size_parameters =[](const Parameters& rParameters){return std::distance(rParameters.begin(), rParameters.end());};
      |                                                                    ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/12.1.0/string:46,
                 from /home/roigcarlo/Kratos/kratos/containers/model.h:18,
                 from /home/roigcarlo/Kratos/kratos/processes/from_json_check_result_process.cpp:18:
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h:146:5: note: candidate: ‘template<class _InputIterator> constexpr typename std::iterator_traits< <template-parameter-1-1> >::difference_type std::distance(_InputIterator, _InputIterator)’
  146 |     distance(_InputIterator __first, _InputIterator __last)
      |     ^~~~~~~~
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h:146:5: note:   template argument deduction/substitution failed:
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h: In substitution of ‘template<class _InputIterator> constexpr typename std::iterator_traits< <template-parameter-1-1> >::difference_type std::distance(_InputIterator, _InputIterator) [with _InputIterator = Kratos::Parameters::const_iterator_adaptor]’:
/home/roigcarlo/Kratos/kratos/processes/from_json_check_result_process.cpp:584:81:   required from here
/usr/include/c++/12.1.0/bits/stl_iterator_base_funcs.h:146:5: error: no type named ‘difference_type’ in ‘struct std::iterator_traits<Kratos::Parameters::const_iterator_adaptor>’

I added those to the definition of both const_iterator and iterator in the Parameters class:

using iterator_category = std::forward_iterator_tag;
using difference_type   = std::ptrdiff_t;
using value_type        = Parameters;
using pointer           = Parameters*;
using reference         = Parameters&;

I would need to compile myself

[pooyan-dadvand]

Are you sure that the definition of the value_iterator is not in conflict with others?

Meanwhile, to me, the error seems to come from some cross includes

[loumalouomega]

Meanwhile, to me, the error seems to come from some cross includes

I also thought that, I tried different configurations and I didn't get it work...

[matekelemen]

@roigcarlo ah found it, your aliases are private, so iterator_traits can't find them.

[roigcarlo]

I would preffer to have everything compiled in the same standard (as long as its possible). Is there any reason to leave it in C++11?

[philbucher]

Yes, compatibility with external solvers that use C++11 I wouldn't want to debug incompatibilities because of different standards 😅

In principle I would love to use 17 there as well but @pooyan-dadvand and I decided to leave it 11

[roigcarlo]

@philbucher Done

[philbucher]

Thanks!

[roigcarlo]

@pooyan-dadvand @philbucher seems that the only problem is the LockObject move constructor in intel and we are ready to go.

Do we have any alternative to this? Sorry to ask but I did not follow the development of neither the Lock object and the chunk closely so I am a little bit blind on what is used for.

[philbucher]

Afaik @pooyan-dadvand fixed that in a recent PR, but not sure if this is merged

[philbucher]

Should be fixed with #9943

[roigcarlo]

@philbucher Please take a look at the last changes (the ones related with the default MPIDatacomms). I am never 100% that I am not forcing a communicator that cannot be used with the cosim app.

[philbucher]

@philbucher Please take a look at the last changes (the ones related with the default MPIDatacomms). I am never 100% that I am not forcing a communicator that cannot be used with the cosim app.

Coincidentally I fixed the utility properly in #10110

In a nutshell, don't hardcode communicators to world. EIther take it from a ModelPart or, if it is a utility, then make it an input argument ~EDIT for testing using world or another communicator explicitly is ok~ EDITEDIT for testing use KM.Testing.GetDefaultDataCommunicator() in python and Testing::GetDefaultDataCommunicator() in C++ if you need a DataComm. We introduced this to make it clear that this is intended for testing

[roigcarlo]

@philbucher I just move your commits here and adapted the rest (nothing should be using world now, its either the default or the one from the mpda in the process)

If there is no further issues, this should be ready to merge.

[roigcarlo]

Mmm seems that after the last changes a new deprecation appeared, any idea CheckSerializationForSimpleType. It looks like an important check to me, why is used in core comms if its deprecated Oo?

[philbucher]

Iirc this is a sort of security check added by Jordi Basically the data exchange is done via serialization even though it can be done directly (i.e. for simple types)

However deprecation warnings should be disabled in the CI, maybe the compiler has changed

=> you can disable the deprecation warnings, I am pretty sure they were disabled when I added the Intel compiler And also they are disabled for the other compilers

[RiccardoRossi]

well if it follows a suboptimal path we should discover why...

On Fri, Aug 12, 2022, 20:35 Philipp Bucher @.***> wrote:

Iirc this is a sort of security check added by Jordi Basically the data exchange is done via serialization even though it can be done directly (i.e. for simple types)

However deprecation warnings should be disabled in the CI, maybe the compiler has changed

=> you can disable the deprecation warnings, I am pretty sure they were disabled when I added the Intel compiler And also they are disabled for the other compilers

— Reply to this email directly, view it on GitHub https://github.com/KratosMultiphysics/Kratos/pull/9937#issuecomment-1213404048, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5PWENGOMZU2X5CS7DNIJDVY2KPLANCNFSM5XKUXCZA . You are receiving this because your review was requested.Message ID: @.***>

[loumalouomega]

Are we C++17 already?

[philbucher]

Yes 🎉