Define string constants in *.cpp files

dabonnie commented 4 years ago

Description

Per https://github.com/ros-tooling/system_metrics_collector/pull/115/files#r393231762 define all constant strings in their relevant *.cpp files.

Test Plan

Unit test for all defined constants where applicable

Release Plan

check in PR

Acceptance Criteria

[ ] Code has been implemented, reviewed and merged.
[ ] Test plan has been completed
[ ] Release plan has been completed

Once all above items are checked, this story can be moved to done

dabonnie commented 4 years ago

Given #117 this now applies to the libstatistics_collector and system_metrics_collector packages

mm318 commented 4 years ago

After some discussion, the conclusion was that no action needs to be done for this issue. Below are more details.

First to provide some background terminology:

When you give a variable a definition, that variable will have an actual place (a symbol) in the generated machine code. When you give a variable a declaration, it merely tells the compiler to expect there to be a symbol for this variable in the final linked machine code, so the compiler doesn't actually allocate space or symbol in the machine code it is generating. external linkage means that the symbol is visible across different translation units, whereas internal linkage means that the symbol is only visible within the one translation unit that it is in. A multiple definition linker error occurs when there multiple symbols with external linkage of the same name (need to confirm if a symbol with external linkage and one with internal linkage will cause this error, but my guess right now is no). An undefined symbol linker error means that there are no symbols (of either external linkage or internal linkage) found for a name that was declared and is being used.

The following are the possible (and impossible) statements in C++ that declare or define variables / symbols:

namespace Foo {
    int a;                      // definition with external linkage
    int b = 1;                  // definition with external linkage
    extern int c;               // declaration
    extern int d = 1;           // definition with external linkage (with compiler warning because the extern keyword is not expected and is unnecessary)
    const int e;                // not allowed
    const int f = 1;            // definition with internal linkage
    extern const int g;         // declaration
    extern const int h = 1;     // definition with external linkage
    constexpr int i;            // not allowed
    constexpr int j = 1;        // definition with internal linkage
    extern constexpr int k;     // not allowed
    extern constexpr int l = 1; // definition with external linkage
    static int m;               // definition with internal linkage
    static int n = 1;           // definition with internal linkage
    extern static int o;        // not allowed
    extern static int p = 1;    // not allowed
}

Things to note about constexpr. It is not possible to merely have a constexpr variable declaration, so there is no risk of having an undefined symbol linker error with constexpr variable (most relevant to this issue). However, using extern constexpr can still result in a multiple definition linker error.

Also note that these rules are completely different for class/struct member variables. I will follow up with that.

mm318 commented 4 years ago

With classes, the story is a bit different (the extern keyword is not applicable at all here, and the storage non-static member variables does not involve linkage):

class Foo {
    static char a[2];                   // declaration
    static char b[2] = "1";             // not allowed (unless b is an integral type like int, in which case this would be a declaration)
    static const char c[2];             // declaration
    static const char d[2] = "1";       // not allowed (unless d is an integral type like int, in which case this would be a declaration)
    static constexpr char e[2];         // not allowed
    static constexpr char f[2] = "1";   // declaration
};

Things to note about constexpr. Whether or not a constexpr static member variable is allowed to be initialized in-class or not is the opposite of non-constexpr static member variables. It may seem strange that member variable f is still just a declaration and not a definition, and it is.

mm318 commented 4 years ago

Summary of how to make the linker happy:

There can be any number of declarations for the same variable. Declarations can go in both .hpp or .cpp files. There must be one corresponding definition with external linkage for these declarations.
There can only be one definition with external linkage for the same variable in the project, and it should be in a .cpp file.
It is recommended to have only one written definition with internal linkage for the same variable. (Multiple definitions will give compiler error only if they're visible to the same translation unit. Once past compilation, there won't be linker errors.) While static variables conventionally only appear in .cpp files, the C++ community doesn't seem to apply the same limitation to constexpr variables (they can go in either .hpp or .cpp files).

mm318 commented 4 years ago

Closing this issue as we're choosing not to make the proposed changes.

ros-tooling / system_metrics_collector