Trouble changing class variables within method

[TylerLaBree]

I've implemented the option to run various sim types using the variable "mode" which is a member of the GI class. (This isn't the most descriptive name, so it will likely be changed later). It is my understanding that the best way to change the value of this variable is with a method in GI. When I attempted to change its value this way, I'm given an error. I've gotten the functionality to work using another strategy, but I don't think I'm using good coding practice (GI.h:317-318, "add-ANEOS..." branch).

ParameterFile parameter_file("init/input.txt");
template <class Ptcl> int GI<Ptcl>::mode = parameter_file.getValueOf("mode", 0 );

The error is caused by GI.h:37 on the "add-ANEOS..." branch when the line is uncommented.
//mode = parameter_file.getValueOf("mode", 0 );
Here's the error it returns upon compilation:

(base) iMac:build Tyler-EES$ make
Scanning dependencies of target sph.out
[ 50%] Building CXX object CMakeFiles/sph.out.dir/src/main.cpp.o
[100%] Linking CXX executable sph.out
Undefined symbols for architecture x86_64:
  "GI<STD::RealPtcl>::mode", referenced from:
      GI<STD::RealPtcl>::setupIC(ParticleSimulator::ParticleSystem<STD::RealPtcl>&, system_t&, ParticleSimulator::DomainInfo&) [clone .constprop.783] in main.cpp.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
make[2]: *** [sph.out] Error 1
make[1]: *** [CMakeFiles/sph.out.dir/all] Error 2
make: *** [all] Error 2

I'm not sure the cause, but it may have something to do with the structure of the code. I think it's usually customary to create only the class interface in the .h file while keeping the implementation in a corresponding .cpp file (GI.cpp in this case). Currently, when we #include GI.h in main.cpp it seems to be doing the equivalent of copying and pasting all of GI.h to that location in the code, which may be causing the issue.

Am I on to anything here?

I also notice that compile times are somewhat long, and I suspect that this "copying and pasting" may be the cause because it recompiles all the code every time. If we had corresponding .cpp files for implementation of functions and classes, it may compile quicker, because it only recompiles the .cpp files that changed. (I recognize this isn't a priority right now, but just thought I'd mention it.)

[TylerLaBree]

@NatsukiHosono @gassmoeller

[gassmoeller]

I think this issue has several layers to it that we should discuss in turn.

First, let me ask if you need the mode variable outside of the setupIC function? If that is not the case (it looks like it to me) then there is no need to make the variable a member variable of the class (no matter if static or not). const unsigned int mode = parameter_file.getValueOf("mode", 0 ); inside the setupIC function would be enough. Then you do not need to declare the member variable further up, or instantiate the static member at the bottom of the file.

Second, I think the use of a mode magic number that stands for certain setups is not well maintainable in the long run. I have seen codes that use this, and they often end up with constructs like "mode 7 is like mode 3, but with the following variation ...", they need reserved numbers for internal use, there will be conflicts when different users use the same numbers, and they need lots of documentation what the individual modes do. What about the following suggestion: Instead of having one integer "mode", create two input strings that are called "Impactor initialization method" and "Target initialization method". They both are tested for values "none", "regular sphere", and "read particles from file" and give an error if they contain something else. This would be more readable and self-explanatory.

Third, I agree with your suggestion to split header files and implementation more consistently than what is done right now. I do not think compile times are particularly bad at the moment, but this also has other benefits. If you want to separate them, do it one class at a time and open separate pull requests on separate branches for each of them. This makes debugging and reviewing the changes easier.

[TylerLaBree]

Hi Dr. Gassmoeller. I appreciate the response! I agree that mode is probably not the best name for clean and maintainable code. It started as a placeholder, but when I couldn't get it to work myself, I came to you for help. I'll be sure not to implement any "magic numbers" from now on, and change mode into something more human-readable.

In addition, the need to access mode outside of the setupIC method may be made obsolete by the addition of command-line options. I originally wanted to place result files in different directories depending on the value of mode, but I think I'll allow the user to specify the dir at runtime instead.

However, I think I would also like to be able to specify GI<Ptcl>::END_TIME (which wouldn't be const, so end_time), using an input file, so the user can specify the length of the simulation at runtime. I think I need the same functionality to be able to do this.

Just this morning, I actually found a workaround to get this to work, but I'm not sure why. I just had to declare template <class Ptcl> double GI<Ptcl>::end_time; at the bottom of the file (outside the class). This allows me to read and write to end_time in setupIC, in the main method, etc. I'm not sure why this is necessary though. Could you shed some light on this?

Finally, thanks for the feedback about the header/implementation situation. If I have time this summer, I will follow your advice when editing the files in this way.

[gassmoeller]

Hi Tyler,

In addition, the need to access mode outside of the setupIC method may be made obsolete by the addition of command-line options. I originally wanted to place result files in different directories depending on the value of mode, but I think I'll allow the user to specify the dir at runtime instead.

That sounds like a good plan.

However, I think I would also like to be able to specify GI::END_TIME (which wouldn't be const, so end_time), using an input file, so the user can specify the length of the simulation at runtime. I think I need the same functionality to be able to do this.

At the moment yes, but this is something I am not too happy about anyway. The GI class makes heavy use of static variables and functions, which is generally discouraged (it essentially means we use the class not as a class, but as a collection of functions that are independent of each other). Let's talk about that at a later time though, as changing it requires some changes to main.cpp as well.

Just this morning, I actually found a workaround to get this to work, but I'm not sure why. I just had to declare template double GI::end_time; at the bottom of the file (outside the class). This allows me to read and write to end_time in setupIC, in the main method, etc. I'm not sure why this is necessary though. Could you shed some light on this?

Yes this is the correct solution (though in theory it does not have to be at the bottom of the file, it can be anywhere outside of any other scope). The reason is that the declaration (the line inside the class GI ... scope) only tells the compiler that there might be a variable of this name, which is static (i.e. not related to any specific instance of this class). Only when you actually use (i.e. define) this variable in the line outside the class does the compiler actually create this variable. template classes/variables are only used as templates to create real objects, and that is what the additional line is for (also see the section "Defining and initializing static member variables" here: https://www.learncpp.com/cpp-tutorial/811-static-member-variables/). Now here comes the problem you mentioned in your earlier post: Since the declaration and implementation (=definition) of GI happens in the same header file we can now not include this header more than once. Otherwise the compiler would try to create two instances of the static variable, which is not allowed. If you need to include the header twice, lets go with a quick solution for now, and make the variable non-static later. What should work is to move the line template <class Ptcl> double GI<Ptcl>::end_time; into main.cpp, outside of any function. This way the static variable is only ever created once. Let me know if that works.

[TylerLaBree]

Thank you! Your feedback is incredibly helpful. Thanks for taking the time to explain this to me. I'm learning a great deal about c++ with your help. I'll move template <class Ptcl> double GI<Ptcl>::end_time; to main.cpp above the main method and read up more about static members.