Comments (32)
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:
-
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 !)...
-
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...)
-
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.
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.
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.
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...
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.
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:
-
in arpack.hpp: suppress everything + include arpack.h
-
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*/ )
}
- 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.
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 cvstd::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 ofz
, and
reinterpret_cast<cv T(&)[2]>(z)[1]
shall designate the imaginary part ofz
.
Moreover, ifa
is an expression of type cvstd::complex<T>*
and the expressiona[i]
is well-defined for an integer expressioni
, then:
reinterpret_cast<cv T*>(a)[2*i]
shall designate the real part ofa[i]
, and
reinterpret_cast<cv T*>(a)[2*i + 1]
shall designate the imaginary part ofa[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.
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 thec_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.
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, 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.
Related Issues (20)
- DSAUPD() fails with ARPACK 3.9.0, but not with ARPACK 3.8.0 HOT 40
- ARPACk 3.9.0 and mingw with clang HOT 16
- Initial vector generation is broken in 3.9.0, causing rare failures HOT 14
- Different RNG seeding strategies in complex vs real versions of parallel getv0() HOT 5
- issue46_tst requires mpiexec HOT 6
- About the parpacklib compile problem. HOT 2
- Make seems to be ineffective HOT 1
- When I run massive online analysis, I get an error "Failed to load implementation from: com.github.fommil.netlib.NativeSystemARPACK". HOT 3
- Creating CI job for windows using vcpkg (github action)? HOT 2
- RFE: please start making github releases HOT 2
- About arpackmm test suite HOT 2
- About pyarpack test suite HOT 4
- MATRIX_MARKET tests failure with parallel make -jN check HOT 5
- Test issue401_tst fails HOT 1
- Example BAND cnbdr2.f
- Segmentation Fault, mkl_sparse_d_mv, real symmetric lanczos HOT 4
- libparpack built without using FFLAGS when using autotools HOT 2
- Tests fail to build with LTO HOT 1
- Fails to build with ILP64 on Alpine Linux x86_64 since arpack 3.9.0 HOT 8
- Using ifx fortran compiler from Intel 2024 oneapi gives errors during configure step HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from arpack-ng.