Fortran module in GRHydro_InterfacesM.h is nonsensical

[Roland Haas]

since 1.) the module is stored in a .h file 2.) the .h file is included at the top of .F90 files 3.) the routines for which modules are declared are mostly C routines that do not live in a module

as a result should one actually "use" the module in eg con2prim then one would get a link time error looking for the non-existent fortran module routine

I probably also creates all kinds of race conditions to have the same module defined in more than one source file, it also carefully circumvents the Cactus module tracking mechanism.

It is highly confusing and misleading to have the module present when in fact it cannot be used.

Keyword: GRHydro

[Erik Schnetter]

It is not legal in Fortran to define the same module twice. The situation is the same as defining two routines with the same name -- this is a fundamental "don't". I am raising the severity of this. I also consider this a blocker for the release.

I understand that people strive to structure Fortran code in a manner similar to C code, but unfortunately that doesn't work. Instead of defining stand-alone subroutines, and then declaring interfaces (and repeating much code), it is far superior to define subroutines INSIDE a module, and then using that module when the subroutine is to be called. Include files and interfaces should not be necessary at all for this.

The drawback of this is that Cactus cannot call subroutines that are defined in a module, it only knows how to call stand-alone subroutines (however, this can be changed). That is, all scheduled or externally visible routines need to be stand-alone routines.

[Frank Löffler]

Do we have a working and tested remedy for this?

[Roland Haas]

nothing tested, one solution is to remove the module altogether. Another to remove the module part of the file and have the #include inside of each subroutine that uses the interfaces.

[Frank Löffler]

I agree that something needs to be done, but unless someone really puts in a lot of time for testing on various machines I think it might be best to defer this for after the release. That file has been there for quite a while, and apparently nothing really serious happened so far. What d'you think?

[Roland Haas]

In particular we have duplicate modulde definition in Con2PrimInterfacesM.h and Con2PrimInterfacesAM.h which prevents (depending on some race condition) compilation of the code.

I do not think this should be left alone instead we should fix it. Even if the "fix" is removal of the module altogether. It's not useful anyway since it tries to define a module for subroutines that are implemented in C, I don't even understand how it ever compiled.

[Frank Löffler]

I would be very happy if you could provide a working solution, but we would need it soon and then we can decide about the release. It would need to get tested on all machines after all. It's now so late in the release process that this makes me really uncomfortable. However bad the current situation is, it seems to work on all machines we test on.

[Frank Löffler]

interface-2013_05.patch seems to remove the interface, but also contains changes I think are unrelated (src/GRHydro_Con2PrimAM.F90).

[Roland Haas]

yes the release branch one replicates the changes in rev 517 on trunk which do not apply to 2013_05 since 517 depends on 511 which is not yet in 2013_05. The later is the one that should restore being able to run the magnetized collapse with trunk/release branches. I am as of yet not sure though if that is really the case yet. The changes to src/GRHydro_Con2PrimAM.F90 commute with the change in module location, however one needs to remove use of the include file, so interface-2013_05.patch is really two serial changes: 1.) do the equivalent of r517 in trunk, 2.) remove .h file in favour of proper Fortran files.

[Frank Löffler]

I assume you have tested this on one machine at least with gcc and the intel compiler? If so, please apply. We'll need to get this tested as soon as possible.

[Roland Haas]

Tested on my workstation with gcc and on bethe with intel13. So two machines and the two compilers but never on the same machine with both compilers.

I am somewhat unhappy to see trunk and release diverge already because of 517, but don't quite see a way to avoid it (well I could slim down the interfaces-2013_05.patch file and really only remove the #include I guess thus having fewer changed lines).

[Frank Löffler]

We are only 4 days from complete freeze, and this is still not in, creating a real problem. There is hardly any time left to fix this and have it properly tested on all machines. If it's not committed by tomorrow and we manage to test it in time, I think we should better leave it out (especially since there is currently no report of an actual breakage), and deal with it in trunk.

[Roland Haas]

Oh right, it does say "please apply". Applied the trunk patch as rev 520, applied a "interface only" patch as rev 521 of 2013_05. I'll worry about Con2PrimAM.F90 getting out of sync with Con2PrimM.F90 at another time (occasionally I get the feeling that GRHydro's code could be vastly improved by a rm -rf * :-) ).