zaphoyd / websocketpp

C++ websocket client/server library
http://www.zaphoyd.com/websocketpp
Other
7.08k stars 1.98k forks source link

internal linkage variables in uri.hpp #1146

Open lenard-mosys opened 1 month ago

lenard-mosys commented 1 month ago

Hi, I just spotted the following

https://github.com/zaphoyd/websocketpp/blob/1b11fd301531e6df35a6107c1e8665b1e77a2d8e/websocketpp/uri.hpp#L42-L47

As for answering the question stated in the comment:

Not sure what the original code was, but could have been something like extern uint16_t const uri_default_port = 80, which leads to linking errors if multiple translation units include the header, as there can only be one definition of the single entity uri_default port.

static uint16_t const uri_default_port or just uint16_t const uri_default_port (const implies static here, don't ask why) makes each translation unit have their own uri_default_port variable.

Some usual workarounds of the original problem:

  1. From C++17, you can have inline variables (I believe that's not an option), these generate weak symbols for the variables:

    inline uint16_t const uri_default_port = 80;
    inline uint16_t const uri_default_secure_port = 443;
  2. You can emulate C++17 inline variables with static members of class templates, then you can assign them to namespace-scope static references (boost itself uses this pattern at places). This is very cumbersome, but works:

    
    template <typename = void>
    struct uri_default_ports {
    static const uint16_t insecure;
    static const uint16_t secure;
    };

template const uint16_t uri_default_ports::insecure = 80;

template const uint16_t uri_default_ports::secure = 443;

static const uint16_t& uri_default_port = uri_default_ports<>::insecure; static const uint16_t& uri_default_secure_port = uri_default_ports<>::secure;


3. You can use the "enum hack", the trade-off is that you can't take the address of these, which is potentially breaking. And specifying the underlying type is not available before C++11, if you care about that.
```cpp

enum uri_default_ports {
    uri_default_port = 80,
    uri_default_secure_port = 443
};
  1. Leave as is. Use of these variables in inline functions (as you already do) might open the door of ODR-violations, if the address of these variables are taken (they get odr-used), as you have distinct entities in distinct translation units for these variables. I don't think you actually have this issue, but your user might. In any case, I don't think a typical ODR-violation with these variables would manifest in an actual bug.