Multiple definition of constexpr symbols when used in different compilation units

[dev-zero]

Multiple definition of constexpr symbols when used in different compilation units

[alazzaro]

I don't have time to look at this problem now, however an easy fix is to make all functions in the header as static. @dev-zero could you try that?
BTW, we don't have a test for this interface, I will open a new issue...

[dev-zero]

Does not work. When building a file including that header, the compiler expects a definition:

In file included from test_a.cc:1:0:
src/dbcsr.h:38:17: warning: ‘void c_dbcsr_release(void**)’ used but never defined
     static void c_dbcsr_release(void** matrix);
                 ^~~~~~~~~~~~~~~
In file included from test_b.cc:1:0:
src/dbcsr.h:38:17: warning: ‘void c_dbcsr_release(void**)’ used but never defined
     static void c_dbcsr_release(void** matrix);

[dev-zero]

ok, the following works (and is probably what you meant):

diff --git a/src/dbcsr.h b/src/dbcsr.h                                                                                                                                                                       
index 6ad84d7c..27d76a3d 100644
--- a/src/dbcsr.h
+++ b/src/dbcsr.h
@@ -11,7 +11,7 @@ extern "C" {
     
     void c_dbcsr_finalize_lib_aux(MPI_Fint* fcomm);
     
-    void c_dbcsr_finalize_lib(MPI_Comm comm)
+    static void c_dbcsr_finalize_lib(MPI_Comm comm)
     {
         MPI_Fint fcomm = MPI_Comm_c2f(comm);
         c_dbcsr_finalize_lib_aux(&fcomm);
@@ -20,7 +20,7 @@ extern "C" {
     void c_dbcsr_distribution_new_aux(void** dist, MPI_Fint* fcomm, int* row_dist, int row_dist_size,
                                       int* col_dist, int col_dist_size);
     
-    void c_dbcsr_distribution_new(void** dist, MPI_Comm comm, int* row_dist, int row_dist_size,
+    static void c_dbcsr_distribution_new(void** dist, MPI_Comm comm, int* row_dist, int row_dist_size,
                                               int* col_dist, int col_dist_size)
     {
         MPI_Fint fcomm = MPI_Comm_c2f(comm);
@@ -50,17 +50,17 @@ extern "C" {
 
 
 namespace dbcsr {
-    constexpr auto& init_lib = c_dbcsr_init_lib;
-    constexpr auto& finalize_lib = c_dbcsr_finalize_lib;
-    constexpr auto& distribution_new = c_dbcsr_distribution_new;
-    constexpr auto& distribution_release = c_dbcsr_distribution_release;
-    constexpr auto& create_new_d = c_dbcsr_create_new_d;
-    constexpr auto& finalize = c_dbcsr_finalize;
-    constexpr auto& release = c_dbcsr_release;
-    constexpr auto& print = c_dbcsr_print;
-    constexpr auto& get_stored_coordinates = c_dbcsr_get_stored_coordinates;
-    constexpr auto& put_block_d = c_dbcsr_put_block_d;
-    constexpr auto& multiply_d = c_dbcsr_multiply_d;
+    static constexpr auto& init_lib = c_dbcsr_init_lib;
+    static constexpr auto& finalize_lib = c_dbcsr_finalize_lib;
+    static constexpr auto& distribution_new = c_dbcsr_distribution_new;
+    static constexpr auto& distribution_release = c_dbcsr_distribution_release;
+    static constexpr auto& create_new_d = c_dbcsr_create_new_d;
+    static constexpr auto& finalize = c_dbcsr_finalize;
+    static constexpr auto& release = c_dbcsr_release;
+    static constexpr auto& print = c_dbcsr_print;
+    static constexpr auto& get_stored_coordinates = c_dbcsr_get_stored_coordinates;
+    static constexpr auto& put_block_d = c_dbcsr_put_block_d;
+    static constexpr auto& multiply_d = c_dbcsr_multiply_d;
 }

 # endif

Two questions:

• Wouldn't it be cleaner to not export those _aux functions in the first place and add the implementation of the c_dbcsr_finalize_lib and c_dbcsr_distribution_new in a separate dbcsr.c file?
• Does it really make sense to simply alias the function names into the dbcsr namespace? The API doesn't become magically object oriented C++ with that. If you want those aliases, I would recommend to use a different namespace like c_dbcsr to keep the dbcsr namespace clean for a proper C++ API .

What do you think?

[jkn93]

I'll submit a small extension to my intermediate header. The namespace contains
wrapper-funcs that'll ease the access to the fortran-routines by using references.
The sample 'dbcsr_example_5.cpp' uses those wrappers.
This header should also work with plain c-code.
Feel free to modify...

[dithillobothrium]

• Does it really make sense to simply alias the function names into the dbcsr namespace? The API doesn't become magically object oriented C++ with that. If you want those aliases, I would recommend to use a different namespace like c_dbcsr to keep the dbcsr namespace clean for a proper C++ API

I think namespace is needed just to shorten the names, otherwise it is not needed.

[dev-zero]

ok then, what about using a different namespace like c_dbcsr to keep the dbcsr namespace empty for a truly OO API for DBCSR?

[dithillobothrium]

OO API will be a class with static functions? If it is so then there is no too much difference between namespace and class.

[dev-zero]

no idea how the OO API could look like, but I would imagine that something like this might be nice:

dbcsr::Matrix m("mymat", ...);

m.get_stored_coordinates(0, 1, ...);
std::cout << m << std::endl;

m.finalize();

[dithillobothrium]

Ah. Then probably we dont need the copies of the functions in the namespace. Just C-interface and OO API like that.

[alazzaro]

Guys, this part is on my to-do list. Let's keep the interface as it is. BTW, I would prefer to use static. I must admit the "inline" is artificial... In both cases, explicit inline and static allow the declaration of functions inside the headers.

[dev-zero]

@alazzaro you mean the inline as proposed in #64? Did you see that some of the calls are also reducing parameters? For example:

dbcsr/src/dbcsr_c.h

Lines 67 to 70 in 66cee2a

	void multiply(char transa, char transb, double alpha, dm_dbcsr& c_matrix_a, dm_dbcsr& c_matrix_b,
	double beta, dm_dbcsr& c_matrix_c){
	c_dbcsr_multiply_d(transa,transb,alpha,&c_matrix_a,&c_matrix_b,beta,&c_matrix_c,nullptr);
	}

[alazzaro]

Sorry, I'm referring to the current version on develop, which I think is the topic on this issue. The extended in the PR #64 has to carefully reviewed... I propose to use static as your description in #65 (comment)

[alazzaro]

OK to drop the C++ namespace, then we have to update the example to use C functions. I will fix it in the next days (and add a test for the interface). Do we have the C enable interface on Travis?

[dev-zero]

C enable interface is on by default on Travis, but not building the examples. Will update the example today since I have the drop already prepared in PR #82

[alazzaro]

Well, I can move the example to the test directory and making a test out of it (Friday).