C/C++ bindings const and std::complex about arpack-ng HOT 32 CLOSED

I don't see any reason why it would not be.
it is great to see changes on a 30+ yo project, I won't be the one pushing back on new stuff. I will only be the guardian of the ABI/API!

from arpack-ng.

Const is the best 👍

from arpack-ng.

An enum is the best option, and for this one IMO it's worth making converter functions. Not sure we want to force C++11 on the user though (regarding scoped enum vs C-style enum).

from arpack-ng.

You're kinder than me.

#if __cplusplus <= 199711L
#error Upgrade your compiler it's 2018
#endif

On a serious note, could we also just use the C bindings for the pre C++11 users if they included *.hpp and issue a warning that it's a C++11 interface. I'm not 100% sure if the reinterpret_cast<> is stable pre-C++11.

from arpack-ng.

The goal of the branch would be to continue the dev before merging in master?

from arpack-ng.

@sylvestre I've got all the information and a plan to go ahead and implement the C++ bindings with std::complex. Before I start coding this, are you able to give me an indication on the likelihood of this being merged in?

from arpack-ng.

@10110111 @fghoussen
You might be interested by this proposition.

Sure, const would be good!

from arpack-ng.

Indeed, I'm all for const-correctness. Actually I was thinking of proposing the change after my previous fix was accepted.

As for _Complex vs std::complex, they are layout-compatible, so I suppose simply changing the header to use std::complex in the C++ version should work without extra work in the Fortran side of the binding.

But to accept std::string instead of const char* you'd have to wrap the original Fortran function into an adapter function. To me it doesn't look too useful to be worth it, especially since it may involve useless dynamic allocation for very short (1-2 char) strings, which are typical for bmat, howmny and which parameters.

from arpack-ng.

I think SSO will avoid heap allocation here but if I can't convince you then keeping it as char const* is ok (I really dislike pointers though). A safer approach would be a strongly typed enum and convert that to char[] so the user really can't make an error with the casing. This of course means adding in some converter functions to the C++ bindings.

For now though, if I changed the C and C++ bindings to be const correct and the C++ bindings to use std::complex<T> are you available to review this PR? Then the examples can be updated to show off the new bindings.

from arpack-ng.

I'll assume c++11?

from arpack-ng.

Not sure about c++11 ...
Users of fortran libs have usually a dependency to use old and deprecated compilers...
However, if this is making your life way easier, please go ahead (and add a checks in the configure and cmake)

from arpack-ng.

I agree 100% : const would be a good thing... In an "ideal" world (which is not always "real" world) ! :D

The iso_c_binding is an (late) evolution (2003 or 2008) of the specifications of the Fortran language to help type conversions and interconnections with C (not C++ !...). Bindings in C and C++ are not handled the same way (in particular, decoration at link time): the difference between arpack.h and arpack.hpp is that you ("only") need extern in arpack.h, but, you need extern "C" in arpack.hpp.

The big picture tells you : from C++, if you want to benefit from iso_c_binding, the idea is to "get back to C" (this means you need to convert C++ types to C types if they are not handled natively... And this is the case for complex - as far as I have seen, people do that [conversions] as iso_c_binding is already tedious/painful enough to get to work).

Now, you can see this in different manners:

1. Why std::complex does not provide cast operator into C types (float/double _Complex): the best solution to me would be add/implement that in std::complex. This would solve so many problems (not only in arpack-ng !)...

2. If const is supported by C (?), you may add a few const in arpack.h if this compiles (?). I don't remember if C supports const, and even if so, I believe it may not mean exactly the same thing (I know/remember there are tricks/traps on the topic...)

3. You can move the existing arpack.hpp to arpack_c_to_cpp.hpp. Then add a pure C++ arpack.hpp (with C++ string, complex, ...). But then you need to provide C++ implementation of these wrap-up functions provided by arpack.hpp: you need to implement (= conversion of C++ types to C types) that in a arpack.cpp. So you need to compile that in an extra library (can't be handled by Fortran) which is a cumbersome tedious and not-great extra work... (this is why people "get back to C" as far as I have seen)

I am not the one who decide: the maintainer does ! I'am not the maintainer.

You guys decide what you like most ! If you prototype some code on a branch, I can have a look and give you my opinion if you ask for it, no problem ! :D

A the time I did that I was looking for a "clean but simple" solution: this is why I didn't get into that.

Franck

from arpack-ng.

Also, at the time, I had no time/need to provide iso_c_binding for parpack !... If you feel the courage to do it (tedious), feel free to do so. Would be a good thing.

from arpack-ng.

My plan is to have the C++ bindings call the C bindings to avoid dealing directly with FORTRAN. Since C++11, the reinterpret_cast<> seems to guarantee the compatibility of C and C++ complex types, if I understood you correctly, that addresses point 1.

For 2. I don't know if const decorations change the function signature when you interface with FORTRAN. I was under the impression that const is part of the type system and on a binary level it means almost nothing. This needs to be checked but C supports const in any case (http://en.cppreference.com/w/c/language/const) - some pitfalls are listed there. Again, I'm not sure how this will effect the FORTRAN binding.

For 3. I should be able to keep this header only - so no extra library except for arpack.h.

I'll get something together on the weekend and we can discuss from there.

from arpack-ng.

@dbeurle

For 2. I don't know if const decorations change the function signature when you interface with FORTRAN. I was under the impression that const is part of the type system and on a binary level it means almost nothing.

Functions are not decorated in C. Moreover, extern "C" functions are not decorated in C++ either. So const or non-const, or whatever at all, doesn't affect the symbols. And yes, on the binary level const doesn't change anything — it's simply an aid of compile-time detection of some logical errors.

from arpack-ng.

@sylvestre

Not sure about c++11 ...
Users of fortran libs have usually a dependency to use old and deprecated compilers...

I propose to check #if __cplusplus < 201103L to detect pre-C++11 compilers, and refrain from saying enum class for them, otherwise do use the scoped enums. This will both give strong scoping for C++11 and some support for C++03.

Something like

#if __cplusplus < 201103L
namespace BMAT
{
enum
#else
enum class
#endif
     BMAT
{
    StandardEigenvalueProblem,
    GeneralizedEigenvalueProblem
};
#if __cplusplus < 201103L
}
#endif

This way the user code will look identicaly for C++03 and C++11/higher, like BMAT::StandardEigenvalueProblem, and global namespace won't be polluted by the enumerators.

from arpack-ng.

On a serious note, could we also just use the C bindings for the pre C++11 users if they included

Doesn't sound good to me, since C bindings will have C-only types included like _Complex, which are not standard C++.

I'm not 100% sure if the reinterpret_cast<> is stable pre-C++11.

Yeah, we might partially check that our assumption about the layout is valid with something like

namespace detail
{
typedef char ComplexCheck[1-2*!(sizeof(std::complex<double>)==2*sizeof(double))];
}

If size is wrong, this'll not compile. If the size is correct, I don't think there's much possibility of having something other than two double elements inside, in which case reinterpret_cast should be defined.

from arpack-ng.

@dbeurle

Since C++11, the reinterpret_cast<> seems to guarantee the compatibility of C and C++ complex types, if I understood you correctly, that addresses point 1.

No, cast is not a conversion. This is not safe...

@10110111

Functions are not decorated in C.

Yes.

Moreover, extern "C" functions are not decorated in C++ either.

You don't want C++ to decore these functions => you need to use extern "C" (= no decoration).

So const or non-const, or whatever at all, doesn't affect the symbols

Yes, symbols are not affected but this is not the question !... The problem is connected to the fact that const may not (exactly) mean the same thing in C and C++: problems could occur at coding and / or compilation time (not link time). This is a semantic problem.

@dbeurle

For 3. I should be able to keep this header only - so no extra library except for arpack.h.

Could be a solution, then you need:

1. in arpack.hpp: suppress everything + include arpack.h

2. in arpack.hpp: implement all the (numerous...) C++ wrappers using C functions. Something like:

template<class Type> saupd (... /*use C++ string or complex with const here*/) {
 ... // Conversion C++ -> C types.
 if (std::is_same(T, float)) ssaupd_c(... /*use C types here*/ )
 if (std::is_same(T, double)) dsaupd_c(... /*use C types here*/ )
}

3. in icb_arpack_cpp.cpp: propagate arpack.hpp changes, replace ssaupd_c(...) by saupd_c(...)

You need string and complex type for the C++ wrapper signature: adding a dependency on C++11 is not needed for that.

from arpack-ng.

@fghoussen

No, cast is not a conversion. This is not safe...

Cast is a conversion (of pointer in this case). Regarding std::complex, C++11 Standard does say in 26.4/4:

If z is an lvalue expression of type cv std::complex<T> then:

• the expression reinterpret_cast<cv T(&)[2]>(z) shall be well-formed,

• reinterpret_cast<cv T(&)[2]>(z)[0] shall designate the real part of z, and

• reinterpret_cast<cv T(&)[2]>(z)[1] shall designate the imaginary part of z.
  Moreover, if a is an expression of type cv std::complex<T>* and the expression a[i] is well-defined for an integer expression i, then:

• reinterpret_cast<cv T*>(a)[2*i] shall designate the real part of a[i], and

• reinterpret_cast<cv T*>(a)[2*i + 1] shall designate the imaginary part of a[i].

And C99 as well as C11 say in 6.2.5/13:

Each complex type has the same representation and alignment requirements as an array type containing exactly two elements of the corresponding real type; the first element is equal to the real part, and the second element to the imaginary part, of the complex number.

Now about your

Yes, symbols are not affected but this is not the question !... The problem is connected to the fact that const may not (exactly) mean the same thing in C and C++: problems could occur at coding and / or compilation time (not link time). This is a semantic problem.

const does mean exactly the same for function parameters: promise of the function not to alter the referenced values. There are some special cases like possibility to introduce a temporary when passing an object by const reference in C++, but there're no references in C, so it's not a problem.

from arpack-ng.

@10110111
Thanks for looking up those clauses. I think C++11 references the C99 standard which is nice. I think there are also some gotchas with const in C but at any rate the proposed usage of const shouldn't be a concern.

@fghoussen

template saupd (... /use C++ string or complex with const here/) {
... // Conversion C++ -> C types.
if (std::is_same(T, float)) ssaupd_c(... /use C types here/ )
if (std::is_same(T, double)) dsaupd_c(... /use C types here/ )
}
in icb_arpack_cpp.cpp: propagate arpack.hpp changes, replace ssaupd_c(...) by saupd_c(...)

One could probably just use function overloading here to work around the s/d kludge in the C bindings for the missing type overloads.

As a side note, std::is_same is C++11. I appreciate the irony though.

from arpack-ng.

You may use specialized templates to avoid C++11 dependency. (the less dependencies, the better - personal opinion)

from arpack-ng.

By the way, if somebody wants to have iso_c_binding for parpack, I began the code months ago... But failed on the "correct type" to attribute to MPI_Communicator. If somebody knows about this, let me know...

from arpack-ng.

I think that the MPI_Communicator type is not consistent across MPI implementations. I think it's either a struct or an int. I'm not interested in parallel eigenvalue solvers but I think a WIP branch would be useful to not waste the work you've already done on it.

from arpack-ng.

Really ?!... Nothing works: code and cmake/autotools tests fail ! This is no use at all !

Anyway, if this can be of interest to anybody (and maintainers) I can PR this on a dedicated branch (named icb_parpack ?) on opencollab/arpack-ng that still must be created ! Also, I will probably can not help more as I am already in a dead end here... Sylvestre, what do you think ?

from arpack-ng.

A godbolted std::complex and C _Complex type conversion example is here https://godbolt.org/g/yCPeSg. What's interesting is the assembly generated for each method is the same using both g++ and clang as one would expect, including the member order as specified in the standard...no surprises.

@10110111 Do you think that the reinterpret_cast in the c_reinterpret_test function is safe if the underlying order is guaranteed by the previous standard clauses? It seems like this should work now if I interface the calls from C++ to C with reinterpret_cast on the pointer passed as an argument (essentially what these tests have done).

If so..I have all the pieces needed to start work on writing the C++ bindings to use std::complex re-based on the commit from Franck.

from arpack-ng.

Do you think that the reinterpret_cast in the c_reinterpret_test function is safe if the underlying order is guaranteed by the previous standard clauses?

Yes. Mostly, reinterpret_cast is restricted by the strict aliasing rules, which in this case do permit accessing the value (see the bullet about aggregate or union type in 3.10/10).

from arpack-ng.

@sylvestre

The goal of the branch would be to continue the dev before merging in master?

Yes. Before merging in master, and, if somebody can finish this (will not be me ! I did everything I could and stands in a dead end). Sylvestre, this is your decision ! :D

Do you think that the reinterpret_cast ...

There is no place for style here. Harass the std consortium to get std::complex::c_cplx : this is the best solution for everybody (not just a fix for arpack). Casting will possibly (probably ?) turn the whole thing into a coredump disneyland... Why don't you do like the 7 billlions other people on Earth ? Can't get that... The assembly is maybe the same today (gcc/clang) but what about tomorrow ? What about intel and pgi compilers ? Debugging this kind of stuffs is really a nightmare and no-fun...

Cast the way you like at your side (these will be your bugs), but, do not change function signatures in headers (let others safe).

from arpack-ng.

@sylvestre

Sylvestre, this is your decision ! :D

I got the cumbersome icb to work finally for parpack: code is OK... CMake is OK, autotools is still KO (not so used to it). If someone can help on autotools here : https://github.com/fghoussen/arpack-ng/commits/icb_parpack, I could PR in opencollab....

from arpack-ng.

Oh men ! Seems cmake is KO on ubuntu : I didn't see that as I am running debian/testing... Anyway, still some work to do here...

from arpack-ng.

@fghoussen Perhaps move this to a different issue please?

from arpack-ng.

Sure

from arpack-ng.

The work is mostly done on this now. I'll close this off and open up separate issues and write tests as they come up. Thanks everyone!

from arpack-ng.