Header guards

[PhysicsIsFun]

Hello there,

I am going through proper header handling.
I learned about the header guard technique here:

http://www.cplusplus.com/forum/articles/10627/

 //x.h

#ifndef __X_H_INCLUDED__   // if x.h hasn't been included yet...
#define __X_H_INCLUDED__   //   #define this so the compiler knows it has been included

class X { };

#endif  

Now, I am going to work that into all my self-created headers.
But what about the "pre-defined" headers of c++, e.g. <vector>?
Say my main() calls a function A(), and this function A() has <vector> in its header, but A() also calls an independent other function B() that needs <vector> on its own - then I would include <vector> twice in my main program by including the header of A().


Regards!

[tpb]

The standard headers are include guarded so it's not a problem.
BTW, you shouldn't generally start an identifier with underscores since such identifiers are reserved for use by the implementation. In particular, names starting with two underscores or names starting with an underscore followed by an uppercase letter are reserved. And at "file level scope" you shouldn't even use a name starting with an underscore followed by a lowercase letter (although you can use them locally).

[PhysicsIsFun]

Ok, but this is from an article posted on this forum, so are you saying the article about 'good practice' actually does it bad?

[PhysicsIsFun]

After reorganizing my headers, my compiler gives an error.

"Line 43 in main(): undefined reference to MCMC_hard_spheres_constant_p(int, double, double, std::vector<particle, std::allocator<particle> >, double, double, double, double)"

Here are my files:

#include <iostream>
#include <fstream>
#include "MCMC_hard_spheres_constant_p.h"

using namespace std;

int main(){
    const double PI {3.14159265359};
    const int N {400};
    const double R {0.5};  
    double L {14}; 
    const double dpos {0.05}; 
    const double dL {0.1};
    const int n {10}; 
    const double beta {1}; 
    string fcc_coord_filename {"fcc_coordinates_a=1.414214_5rows_5columns_4layers.dat"};
    const double V_0 {N*PI*(4./3)*R*R*R};  //total volume of N spheres
    double pmax=25/(8*R*R*R*beta); //TEST

    vector <particle> system_1; 

    ifstream fcc_coord; 
    fcc_coord.open(fcc_coord_filename);
    if(fcc_coord.fail()){                 
        cerr<< "Error opening file"<<endl;
        exit(1);
    }
    double x,y,z;
    while(fcc_coord >> x >> y >> z){
        system_1.push_back({x,y,z});
    }
    fcc_coord.close();

    for(size_t i=0;i<system_1.size();i++){
        system_1[i].x-=L/2;
        system_1[i].y-=L/2;
        system_1[i].z-=L/2;

        system_1[i].x/=L;
        system_1[i].y/=L;
        system_1[i].z/=L;
    }
cout << V_0/MCMC_hard_spheres_constant_p(n, /*p.at(i)*/pmax, L, system_1, dpos, dL, R, beta)<<"   "<</*p.at(i)*/pmax*beta*8*R*R*R<<endl;
}

//header file for MCMC_hard_spheres_constant_p.cpp

#ifndef __MCMC_HARD_SPHERES_CONSTANT_P_H_INCLUDED__
#define __MCMC_HARD_SPHERES_CONSTANT_P_H_INCLUDED__

#include <random>
#include <vector>
#include <fstream>
#include <cmath>
#include "hard_sphere_overlap_check.h"

double MCMC_hard_spheres_constant_p(const int n, double p, double L, std::vector <particle> pos,
                                    const double dpos, const double dL, const double R, const double beta);

#endif // __MCMC_HARD_SPHERES_CONSTANT_P_H_INCLUDED_ 

//MCMC_hard_spheres_constant_p.cpp
#include "MCMC_hard_spheres_constant_p.h"

using namespace std;

double MCMC_hard_spheres_constant_p(const int n, double p, double L, vector <particle> pos,
                                    const double dpos, const double dL, const double R, const double beta){

//very long function, I can add it, if necessary
//it calls function overlap(...) defined in hard_sphere_overlap_check.cpp.

}

//hard_sphere_overlap_check.h
#ifndef __HARD_SPHERE_OVERLAP_CHECK_H_INCLUDED__
#define __HARD_SPHERE_OVERLAP_CHECK_H_INCLUDED__

#include <cmath>
#include <vector>

struct particle{
    double x;
    double y;
    double z;
};

bool overlap(std::vector<particle> &pos, size_t u, const double R, double L);

#endif 

//hard_sphere_overlap_check.cpp
#include "hard_sphere_overlap_check.h"

using namespace std;

//this function checks wether a chosen sphere u overlaps with any other sphere
bool overlap(vector<particle> &pos, size_t u, const double R, double L){

    double rx, ry, rz;

    for(size_t j=0; j<pos.size(); j++){
        if(u==j) continue;
        //calculate shortest distance between spheres (periodic boundaries!)
        rx=abs(pos[u].x-pos[j].x);
        ry=abs(pos[u].y-pos[j].y);
        rz=abs(pos[u].z-pos[j].z);
        if(rx>0.5) rx=1-rx; //periodic boundaries
        if(ry>0.5) ry=1-ry;
        if(rz>0.5) rz=1-rz;
        if((rx*rx+ry*ry+rz*rz)*L*L < 4*R*R){
            return true;
        }
    }
    return false;
}

Before, I did not use guardings and I did not have .cpp files for my functions, instead, I had defined the functions in their .h files (bad practice^^").

[tpb]

so are you saying the article about 'good practice' actually does it bad?

The C++17 draft standard (n4713), section 5.10 paragraph 3 says:

In addition, some identifiers are reserved for use by C++ implementations and shall not be used otherwise; no diagnostic is required.
(3.1) — Each identifier that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.
(3.2) — Each identifier that begins with an underscore is reserved to the implementation for use as a name in the global namespace.

[tpb]

my compiler gives an error.

I don't get that error, although there are other problems.

In hard_sphere_overlap_check.h, get rid of the double underscores at the start and end of the include macro (and don't begin it with an underscore, either). Secondly, don't include <cmath> since it is not used in the header itself, only in the implementation, so include it there only; and include vector in the implementation, too, as well as in the header (don't assume what's in the header since that can change). You should include <stddef.h> in the header to use size_t (it's not a built-in type).

Similarly for MCMC_hard_spheres_constant_p.h. Only vector and hard_sphere_overlap_check.h are needed there. All the others should only be in the implementation file, along with vector and hard_sphere_overlap_check.h.

There's really no point in making ints and doubles const in the parameter lists of your functions. That's unusual, and certainly has nothing to do with the caller (you can't modify the caller's variable if its passed by value), so it is definitely not part of the interface.

[Peter87]

PhysicsIsFun wrote:

After reorganizing my headers, my compiler gives an error.

undefined reference is linker error. If you're using an IDE make sure that MCMC_hard_spheres_constant_p.cpp has been added to the "project".

tpb wrote:

include vector in the implementation, too, as well as in the header (don't assume what's in the header since that can change).

Isn't this a bit overdoing it? I think a header-source pair could be seen as one unit. It's probably maintained by the same person and a change to the header is expected to lead to changes in the source file.

[tpb]

Isn't this a bit overdoing it?

Although I see your point, since it costs nothing I don't think it's overdoing it at all. I think each individual file should be self-sufficient in its includes. If a file uses vector, it should explicitly include vector. I don't see a problem with that.

[PhysicsIsFun]

tpb wrote:

Similarly for MCMC_hard_spheres_constant_p.h. Only vector and hard_sphere_overlap_check.h are needed there. All the others should only be in the implementation file, along with vector and hard_sphere_overlap_check.h.

According to your logic, why would I include hard_sphere_overlap_check.h in both files, the MCMC_hard_spheres_constant_p.h and the .cpp?

Where should I define my structure <particle>? Right now, it is only in the overlap header, but the other programs need it, too. And is it correctly stored in the header or do I need to put it in .cpp as well?
If I put it in MCMC_hard_spheres_constant_p.h, the compiler is not satisfied with the redefinition of <particle>. Should I work with #ifndef here as well?
edit: guarding doesn't seem to work for the structure.

Peter87 wrote:

undefined reference is linker error. If you're using an IDE make sure that MCMC_hard_spheres_constant_p.cpp has been added to the "project".

Seems to work, thanks!

[PhysicsIsFun]

Current version:

//main.cpp
#include <iostream>
#include <fstream>
#include "MCMC_hard_spheres_constant_p.h"

using namespace std;

struct particle{
    double x;
    double y;
    double z;
};

int main(){
    const double PI {3.14159265359};
    const int N {400};
    const double R {0.5};  //diameter of hard spheres
    double L {14}; //length of the system - should be large enough for the initial system config.
    const double dpos {0.05}; //used to propose new (relative) particle position
    const double dL {0.1}; //used to propose new L (max. change of L)
    const int n {10}; //amount of MC steps
    const double beta {1}; //=1/KT
    string fcc_coord_filename {"fcc_coordinates_a=1.414214_5rows_5columns_4layers.dat"}; //name of file with stores lattice coordinates
    const double V_0 {N*PI*(4./3)*R*R*R};  //total volume of N sphere
    double pmax=25/(8*R*R*R*beta); //TEST

    vector <particle> system_1; 

    //setup systems
    ifstream fcc_coord; //read coordinates of fcc lattice sites from file
    fcc_coord.open(fcc_coord_filename);
    if(fcc_coord.fail()){                  // Error test
        cerr<< "Error opening file"<<endl;
        exit(1);
    }
    double x,y,z;
    while(fcc_coord >> x >> y >> z){
        system_1.push_back({x,y,z});
    }
    fcc_coord.close();

    for(size_t i=0;i<system_1.size();i++){
        system_1[i].x-=L/2;
        system_1[i].y-=L/2;
        system_1[i].z-=L/2;

        system_1[i].x/=L;
        system_1[i].y/=L;
        system_1[i].z/=L;
    }

    
cout << V_0/MCMC_hard_spheres_constant_p(n, /*p.at(i)*/pmax, L, system_1, dpos, dL, R, beta)<<"   "<</*p.at(i)*/pmax*beta*8*R*R*R<<endl;
}

//header file for MCMC_hard_spheres_constant_p.cpp

#ifndef MCMC_HARD_SPHERES_CONSTANT_P_H_INCLUDED
#define MCMC_HARD_SPHERES_CONSTANT_P_H_INCLUDED

#include <vector>

struct particle;

double MCMC_hard_spheres_constant_p(int n, double p, double L, std::vector <particle> pos,
                                    double dpos, double dL, double R, double beta);

#endif // __MCMC_HARD_SPHERES_CONSTANT_P_H_INCLUDED_ 

//MCMC_hard_spheres_constant_p.cpp
#include "MCMC_hard_spheres_constant_p.h"
#include "hard_sphere_overlap_check.h"

#include <fstream>
#include <cmath>
#include <random>
#include <vector>

using namespace std;

struct particle{
    double x;
    double y;
    double z;
};

double MCMC_hard_spheres_constant_p(int n, double p, double L, vector <particle> pos,
                                    double dpos, double dL, double R, double beta){

//very long function, I can add it, if necessary
//it calls function overlap(...) defined in hard_sphere_overlap_check.cpp.
}

//hard_sphere_overlap_check.h
#ifndef HARD_SPHERE_OVERLAP_CHECK_H_INCLUDED
#define HARD_SPHERE_OVERLAP_CHECK_H_INCLUDED

#include <vector>

struct particle;

bool overlap(std::vector<particle> &pos, size_t u, double R, double L);


#endif 

//hard_sphere_overlap_check.cpp
#include "hard_sphere_overlap_check.h"
#include <vector>
#include <cmath>

using namespace std;

struct particle{
    double x;
    double y;
    double z;
};

//this function checks wether a chosen sphere u overlaps with any other sphere
bool overlap(vector<particle> &pos, size_t u, double R, double L){

    double rx, ry, rz;

    for(size_t j=0; j<pos.size(); j++){
        if(u==j) continue;
        //calculate shortest distance between spheres (periodic boundaries!)
        rx=abs(pos[u].x-pos[j].x);
        ry=abs(pos[u].y-pos[j].y);
        rz=abs(pos[u].z-pos[j].z);
        if(rx>0.5) rx=1-rx; //periodic boundaries
        if(ry>0.5) ry=1-ry;
        if(rz>0.5) rz=1-rz;
        if((rx*rx+ry*ry+rz*rz)*L*L < 4*R*R){
            return true;
        }
    }
    return false;
}

I am not that happy with defining particle in every .cpp file..

[Repeater]

Do you mean defining, or declaring?

Which line do you not like writing? Tell us which line you don't like writing.

Is it this?

struct particle{
    double x;
    double y;
    double z;
};

If it's that, put that in a header and #include the header in cpp files that need to know what a particle object is.

[PhysicsIsFun]

Exactly, Repeater, these lines.

I tried, but then the compiler has a problem with the double definition, since I have it in two headers.

edit: ah, you mean put it in a header of its own!
I simply went and defined it in the headers of MCMC_hard_spheres_constant_p and hard_sphere_overlap_check, but then I got the error.

I try this idea.

[PhysicsIsFun]

Ok, I put the structure definition now in a separate header file and #included it in every .cpp file.
In my other headers, I simply declared structure particle; instead of #including particle.h, because the headers only need a declaration, no definition. This should be best, right?

[Repeater]

since I have it in two headers.

Put it in ONE header.

The following compiles fine. particle in one header only.



particle.h

#ifndef PARTICLE_H
#define PARTICLE_H

struct particle
{
    double x;
    double y;
    double z;
};

void function(particle);

#endif 

function.cpp

#include "particle.h"
#include <iostream>

void function(particle input)
{
  std::cout << input.x << '\n';
}

main.cpp

#include "particle.h"


int main()
{
  particle instance;
  instance.x = 6;
  function(instance);
}

This should be best, right?

No. What's best is putting it in one header and including that one header where needed.

[PhysicsIsFun]

Yeah, that's what I said I did in my second post :D
Or what do you mean?
I already said I put in a single header "particle.h".

But in the OTHER headers, it should be better to declare struct particle instead of including the header due to it needing less space.

//particle.h
#ifndef PARTICLE_H_INCLUDED
#define PARTICLE_H_INCLUDED

struct particle{
    double x;
    double y;
    double z;
};


#endif // PARTICLE_H_INCLUDED 

//hard_sphere_overlap_check.h
#ifndef HARD_SPHERE_OVERLAP_CHECK_H_INCLUDED
#define HARD_SPHERE_OVERLAP_CHECK_H_INCLUDED

#include <vector>

struct particle;

bool overlap(std::vector<particle> &pos, size_t u, double R, double L);


#endif 

//hard_sphere_overlap_check.cpp
#include "hard_sphere_overlap_check.h"
#include <vector>
#include <cmath>
#include "particle.h"

using namespace std;


//this function checks wether a chosen sphere u overlaps with any other sphere
bool overlap(vector<particle> &pos, size_t u, double R, double L){

    double rx, ry, rz;

    for(size_t j=0; j<pos.size(); j++){
        if(u==j) continue;
        //calculate shortest distance between spheres (periodic boundaries!)
        rx=abs(pos[u].x-pos[j].x);
        ry=abs(pos[u].y-pos[j].y);
        rz=abs(pos[u].z-pos[j].z);
        if(rx>0.5) rx=1-rx; //periodic boundaries
        if(ry>0.5) ry=1-ry;
        if(rz>0.5) rz=1-rz;
        if((rx*rx+ry*ry+rz*rz)*L*L < 4*R*R){
            return true;
        }
    }
    return false;
}

Or do you say it's better to #include "particle.h" in the other header files as well?

[Repeater]

But in the OTHER headers, it should be better to declare struct particle instead of including the header due to it needing less space.

#include "particle.h"
21 characters.

struct particle;
16 characters.

I see. You can save yourself five characters. What's the benefit, saving those five characters? Are you very short of hard drive space, such that five bytes makes a difference? You're solving problems that you don't have. You don't have circular include problems. You don't have large compilation times because you're constantly tinkering with large, widely included header files. Solve problems you have; don't solve problems you don't have.

Or do you say it's better to #include "particle.h" in the other header files as well?

Yes.

[Peter87]

Forward declarations (e.g. struct particle;) is great for avoiding circular dependencies. It can also reduce compilation times quite a bit because if you make a change to a header file all files that include that header file, either directly or indirectly through another header, needs to be recompiled. If they just forward declare it instead of including the header no recompilation is necessary. The downside is that you need to remember if something is a struct, or a class, or an enum. If it's a typedef it's not even possible.

I use it a lot in my own code for my own types but not for types from other libraries. The standard library even prohibits you from declaring anything in the std namespace. With libraries you won't run into problems with circular dependencies (because the library doesn't include your headers) and the library code does not usually change so recompilation is also not an issue.

[PhysicsIsFun]

Ok guys, I change it as you suggest. Thanks!

On another note: I just read about in-line commands.

Since my function "hard_sphere_overlap_check.cpp" is comparably short and it gets called thousands of times in my simulation, would it make sense to declare it as in-line?
My ebook suggests it's a good idea, but elsewhere I have read that modern compilers can do this sort of thing on their own.

Inline functions should be defined in .h files, not in .cpp files, right?

[keskiverto]

Inlining is optional optimization. The compiler is not obliged to inline a function.

int foo(int x)
{
  // foo code
}

int bar()
{
  // code
  y = foo(z);
  // code
}

The unoptimized version has foo's binary code as clearly separate block and the bar's binary code contains instructions of a function call.

If the foo() can be inlined, then bar() won't have a function call, but foo's code directly injected.

Obviously, if the compiler of bar cannot see foo's code (if implementation of foo is not in the same translation unit as bar's implementation), then it cannot inline.

An advanced linker might do optimizations across transaltion units. At least one C++ toolset can use profiling information from a run of the program to improve optimization on the next compilation of the program.

PhysicsIsFun wrote:

Ok, I put the structure definition now in a separate header file and #included it in every .cpp file.
In my other headers, I simply declared structure particle; instead of #including particle.h, because the headers only need a declaration, no definition. This should be best, right?

Yes, in most cases.

More on that:
https://herbsutter.com/2013/08/19/gotw-7a-solution-minimizing-compile-time-dependencies-part-1/

and on tangent:
https://www.bfilipek.com/2018/01/pimpl.html
https://herbsutter.com/gotw/_100/
https://herbsutter.com/gotw/_101/


Parallelization can occur on many forms and levels:
* Auto-vectorization. CPU can perform multiple math operations with single instruction. SIMD. The MMX/SSE*/AVX* instruction sets. Compiler can do this for you, if requested.
* Threads
* GPGPU
* Multiple processes, possibly in separate computers, for example with MPI

You can even have all of them in use in same application.

gets called thousands of times

How much processing time does that use? What fraction of all the time of the application's run? Have you profiled your program to show that the function is the biggest consumer?

[PhysicsIsFun]

Hello Keskiverto, thanks for the response.

Keskiverto wrote:

PhysicsIsFun wrote:
Ok, I put the structure definition now in a separate header file and #included it in every .cpp file.
In my other headers, I simply declared structure particle; instead of #including particle.h, because the headers only need a declaration, no definition. This should be best, right?

Yes, in most cases.

Then you are contradicting Repeater who said it would be best to #include the particle.h file in the headers (no forward declaration). I guess what's best is not so obvious then.

How much processing time does that use? What fraction of all the time of the application's run? Have you profiled your program to show that the function is the biggest consumer?

I did not profile it using some tool (don't know how), but I am pretty sure that this is the most expensive part of my simulation. I already have another thread where I asked for tipps on how to optimize the problem. If declaring it in-line will give an advantage to runtime, it might be a good idea.