Search code examples
c++singletonextern

Should immutable global objects be declared as 'const my_result_t BLAH;' or 'extern const my_result_t BLAH;'?


First, some motivating background info; I'm experimenting with the idea of representing error-codes (returned from functions) as super-lightweight human-readable-strings rather than enum-integers, something like this:

#include <string.h>

/** Ultra-lightweight type for returning the success/error status from a function. */
class my_result_t
{
public:
   /** Default constructor, creates a my_result_t indicating success */
   my_result_t() : _errorString(NULL) {/* empty */}

   /** Constructor for returning an error-result */
   my_result_t(const char * s) : _errorString(s) {/* empty */}

   /** Returns true iff the result was "success" */
   bool IsOK() const {return (_errorString == NULL);}

   /** Returns true iff the result was an error of some type */
   bool IsError() const {return (_errorString != NULL);}

   /** Return a human-readable description of the result */
   const char * GetDescription() const {return _errorString ? _errorString : "Success";}

   /** Returns true iff the two objects are equivalent */
   bool operator ==(const my_result_t & rhs) const
   {
      return _errorString ? ((rhs._errorString)&&(strcmp(_errorString, rhs._errorString) == 0)) : (rhs._errorString == NULL);
   }

   /** Returns true iff the two objects are not equivalent */
   bool operator !=(const my_result_t & rhs) const {return !(*this==rhs);}

private:
   const char * _errorString;
};

my_result_t SomeFunction()
{
   FILE * fp = fopen("some_file.txt", "r");
   if (fp)
   {
      fclose(fp);
      return my_result_t();  // success!
   }
   else return my_result_t("File not Found");
}

int main(int, char **)
{
   printf("SomeFunction returned [%s]\n", SomeFunction().GetDescription());
   return 0;
}

... the idea being that instead of having to maintain a centralized registry of "official" error codes somewhere, any function could simply return a string describing their particular error-condition in a human-readable fashion. Since sizeof(my_result_t)==sizeof(const char *), this shouldn't be significantly less efficient than the traditional return-an-integer-error-code convention that e.g. POSIX likes to use. (I do have to be careful not to return pointers-to-temporary-char-buffers, of course)

... all that works reasonably well; my question concerns the subsequent refinement, which is the creation of some global my_result_t definitions for certain common error-types, e.g.:

const my_result_t RESULT_OUT_OF_MEMORY("Out of Memory");
const my_result_t RESULT_ACCESS_DENIED("Access Denied");  
const my_result_t RESULT_BAD_ARGUMENT("Bad Argument");  
const my_result_t RESULT_FILE_NOT_FOUND("File not Found");  
[...]

... that way the author of SomeFunction() could just return RESULT_FILE_NOT_FOUND; rather than being required to enter a custom error-string and risk typos, inconsistency with other function's result-strings for the same type of error, etc.

My question is, what is the most runtime-efficient way to declare these common/global result-codes?

One way to do it would be to make them 'singleton objects', like this:

// my_result_t.h  
extern const my_result_t RESULT_OUT_OF_MEMORY("Out of Memory");
extern const my_result_t RESULT_ACCESS_DENIED("Access Denied");  
extern const my_result_t RESULT_BAD_ARGUMENT("Bad Argument");  
extern const my_result_t RESULT_FILE_NOT_FOUND("File not Found");  

// my_result_t.cpp
const my_result_t RESULT_OUT_OF_MEMORY("Out of Memory");
const my_result_t RESULT_ACCESS_DENIED("Access Denied");  
const my_result_t RESULT_BAD_ARGUMENT("Bad Argument");  
const my_result_t RESULT_FILE_NOT_FOUND("File not Found");

... or the other way, is to simply put the following in a central header file:

// my_result_t.h  
const my_result_t RESULT_OUT_OF_MEMORY("Out of Memory");
const my_result_t RESULT_ACCESS_DENIED("Access Denied");  
const my_result_t RESULT_BAD_ARGUMENT("Bad Argument");  
const my_result_t RESULT_FILE_NOT_FOUND("File not Found");  

... this latter way seems to work fine, but I'm not 100% confident it's kosher to do that; for one thing, it means that e.g. RESULT_OUT_OF_MEMORY is a separate object in every translation unit, which seems like it might put stress on the linker to de-duplicate, or even invoke conceivably undefined behavior (I'm not sure how the One Definition Rule applies here). On the other hand, using the "extern" approach means that the actual contents of the my_result_t objects are only available to optimizer when compiling my_result_t.cpp, and not when compiling any other functions that reference these objects, which means the optimizer might not be able do inlining-optimizations.

Is one approach better than the other, from the perspective of correctness, but also helping the optimizer to make the code as efficient as possible?


Solution

  • Const namespace scope variables should have default internal linkage, so it is correct to write const my_result_t RESULT_XXX; in the header:

    The const qualifier used on a declaration of a non-local non-volatile non-template non-inline variable that is not declared extern gives it internal linkage. This is different from C where const file scope variables have external linkage.

    Also, having external symbols used in a function, most likely, would not prevent it from being inline-optimized, since the compiler would unpack the function in place, leave any external symbols in an as-is manner. These symbols are then resolved by the linker. However compilers implementations can vary so no definite word can be said about this. That said, using extern const would most likely not cause any trouble for inline optimization. (I tried with msvc, hinted inline with __inline keyword, and the emitted assembly shows that SomeFunction() got inlined).

    I would suggest using the extern const approach since there will be only one instance for each result object.

    Below was my original answer, which is good to have if you use c++ 17 (with extern const you will have to write things twice: definition and then declaration, but not if you are using inline variables)


    C++ 17 has been shipped with some features you need. Essentially, we can build the my_result_t class with string_view, constexpr and inline variable. I have done some modification to your code:

    // ErrorCode.h
    
    #pragma once
    #include <string_view>
    /** Ultra-lightweight type for returning the success/error status from a function. */
    class my_result_t
    {
    public:
        /** Default constructor, creates a my_result_t indicating success */
        constexpr my_result_t() : _errorString{} {/* empty */ }
    
        /** Constructor for returning an error-result */
        constexpr my_result_t(const char* s) : _errorString(s) {/* empty */ }
    
        /** Returns true iff the result was "success" */
        constexpr bool IsOK() const { return (_errorString.data() == nullptr); }
    
        /** Returns true iff the result was an error of some type */
        constexpr bool IsError() const { return (_errorString.data() != nullptr); }
    
        /** Return a human-readable description of the result */
        constexpr std::string_view GetDescription() const { return _errorString.data() ? _errorString : "Success"; }
    
        /** Returns true iff the two objects are equivalent */
        constexpr bool operator ==(const my_result_t& rhs) const
        {
            return _errorString.data() ? ((rhs._errorString.data()) && 
                (_errorString == rhs._errorString)) : (rhs._errorString.data() == nullptr);
        }
    
        /** Returns true iff the two objects are not equivalent */
        constexpr bool operator !=(const my_result_t& rhs) const { return !(*this == rhs); }
    
    private:
        std::string_view _errorString;
    };
    
    inline constexpr my_result_t RESULT_SUCCESS{};
    inline constexpr my_result_t RESULT_OUT_OF_MEMORY("Out of Memory");
    inline constexpr my_result_t RESULT_ACCESS_DENIED("Access Denied");
    inline constexpr my_result_t RESULT_BAD_ARGUMENT("Bad Argument");
    inline constexpr my_result_t RESULT_FILE_NOT_FOUND("File not Found");
    
    // main.cpp
    
    #include "ErrorCode.h"
    #include <iostream>
    
    inline my_result_t SomeFunction()
    {
        FILE* fp = fopen("some_file.txt", "r");
        if (fp)
        {
            fclose(fp);
            return RESULT_SUCCESS;  // success!
        }
        else return RESULT_FILE_NOT_FOUND;
    }
    
    int main() {
        std::cout << SomeFunction().GetDescription();
        return 0;
    }
    

    Compiler explorer link

    In the code, every translation unit share the same result objects, because:

    An inline function or variable with external linkage (e.g. not declared static) has the following additional properties:

    1) It must be declared inline in every translation unit.

    2) It has the same address in every translation unit.

    And it does not affect inlining of functions using the objects, since they are essentially just compile time constants!

    The only drawback is that string_view bigger in size (approximately two times than a const char pointer, depending on implementation). However, this can be avoided by implementing a constexpr "strcmp" function:

    constexpr bool strings_equal(char const* a, char const* b) {
        return a == nullptr ? (b == nullptr) : (b != nullptr && std::string_view(a) == b);
    }
    

    And then you can happily use const char * in your result class instead of string_view!