Skip to content
Snippets Groups Projects
  • Nat Goodspeed's avatar
    5a260e0c
    SL-11216: Convert LLVersionInfo to an LLSingleton. · 5a260e0c
    Nat Goodspeed authored
    This changeset is meant to exemplify how to convert a "namespace" class whose
    methods are static -- and whose data are module-static -- to an LLSingleton.
    LLVersionInfo has no initClass() or cleanupClass() methods, but the general
    idea is the same.
    
    * Derive the class from LLSingleton<T>:
      class LLSomeSingleton: public LLSingleton<LLSomeSingleton> { ... };
    * Add LLSINGLETON(LLSomeSingleton); in the private section of the class. This
      usage implies a separate LLSomeSingleton::LLSomeSingleton() definition, as
      described in indra/llcommon/llsingleton.h.
    * Move module-scope data in the .cpp file to non-static class members. Change
      any sVariableName to mVariableName to avoid being outright misleading.
    * Make static class methods non-static. Remove '//static' comments from method
      definitions as needed.
    * For LLVersionInfo specifically, the 'const std::string&' return type was
      replaced with 'std::string'. Returning a reference to a static or a member,
      const or otherwise, is an anti-pattern: the interface constrains the
      implementation, prohibiting possibly later returning a temporary (an
      expression).
    * For LLVersionInfo specifically, 'const S32' return type was replaced with
      simple 'S32'. 'const' is just noise in that usage.
    * Simple member initialization (e.g. the original initializer expressions for
      static variables) can be done with member{ value } initializers (no examples
      here though).
    * Delete initClass() method.
    * LLSingleton's forté is of course lazy initialization. It might work to
      simply delete any calls to initClass(). But if there are side effects that
      must happen at that moment, replace LLSomeSingleton::initClass() with
      (void)LLSomeSingleton::instance();
    * Most initClass() initialization can be done in the constructor, as would
      normally be the case.
    * Initialization that might cause a circular LLSingleton reference should be
      moved to initSingleton(). Override 'void initSingleton();' should be private.
    * For LLVersionInfo specifically, certain initialization that used to be
      lazily performed was made unconditional, due to its low cost.
    * For LLVersionInfo specifically, certain initialization involved calling
      methods that have become non-static. This was moved to initSingleton()
      because, in a constructor body, 'this' does not yet point to the enclosing
      class.
    * Delete cleanupClass() method.
    * There is already a generic LLSingletonBase::deleteAll() call in
      LLAppViewer::cleanup(). It might work to let this new LLSingleton be cleaned
      up with all the rest. But if there are side effects that must happen at that
      moment, replace LLSomeSingleton::cleanupClass() with
      LLSomeSingleton::deleteSingleton(). That said, much of the benefit of
      converting to LLSingleton is deleteAll()'s guarantee that cross-LLSingleton
      dependencies will be properly honored: we're trying to migrate the code base
      away from the present fragile manual cleanup sequence.
    * Most cleanupClass() cleanup can be done in the destructor, as would normally
      be the case.
    * Cleanup that might throw an exception should be moved to cleanupSingleton().
      Override 'void cleanupSingleton();' should be private.
    * Within LLSomeSingleton methods, remove any existing
      LLSomeSingleton::methodName() qualification: simple methodName() is better.
    * In the rest of the code base, convert most LLSomeSingleton::methodName()
      references to LLSomeSingleton::instance().methodName(). (Prefer instance() to
      getInstance() because a reference does not admit the possibility of NULL.)
    * Of course, LLSomeSingleton::ENUM_VALUE can remain unchanged.
    
    In general, for many successive references to an LLSingleton instance, it
    can be useful to capture the instance() as in:
    
    auto& versionInfo{LLVersionInfo::instance()};
    // ... versionInfo.getVersion() ...
    
    We did not do that here only to simplify the code review.
    
    The STRINGIZE(expression) macro encapsulates:
    std::ostringstream out;
    out << expression;
    return out.str();
    We used that in a couple places.
    
    For LLVersionInfo specifically, lllogininstance_test.cpp used to dummy out a
    couple specific static methods. It's harder to dummy out
    LLSingleton::instance() references, so we add the real class to that test.
    5a260e0c
    History
    SL-11216: Convert LLVersionInfo to an LLSingleton.
    Nat Goodspeed authored
    This changeset is meant to exemplify how to convert a "namespace" class whose
    methods are static -- and whose data are module-static -- to an LLSingleton.
    LLVersionInfo has no initClass() or cleanupClass() methods, but the general
    idea is the same.
    
    * Derive the class from LLSingleton<T>:
      class LLSomeSingleton: public LLSingleton<LLSomeSingleton> { ... };
    * Add LLSINGLETON(LLSomeSingleton); in the private section of the class. This
      usage implies a separate LLSomeSingleton::LLSomeSingleton() definition, as
      described in indra/llcommon/llsingleton.h.
    * Move module-scope data in the .cpp file to non-static class members. Change
      any sVariableName to mVariableName to avoid being outright misleading.
    * Make static class methods non-static. Remove '//static' comments from method
      definitions as needed.
    * For LLVersionInfo specifically, the 'const std::string&' return type was
      replaced with 'std::string'. Returning a reference to a static or a member,
      const or otherwise, is an anti-pattern: the interface constrains the
      implementation, prohibiting possibly later returning a temporary (an
      expression).
    * For LLVersionInfo specifically, 'const S32' return type was replaced with
      simple 'S32'. 'const' is just noise in that usage.
    * Simple member initialization (e.g. the original initializer expressions for
      static variables) can be done with member{ value } initializers (no examples
      here though).
    * Delete initClass() method.
    * LLSingleton's forté is of course lazy initialization. It might work to
      simply delete any calls to initClass(). But if there are side effects that
      must happen at that moment, replace LLSomeSingleton::initClass() with
      (void)LLSomeSingleton::instance();
    * Most initClass() initialization can be done in the constructor, as would
      normally be the case.
    * Initialization that might cause a circular LLSingleton reference should be
      moved to initSingleton(). Override 'void initSingleton();' should be private.
    * For LLVersionInfo specifically, certain initialization that used to be
      lazily performed was made unconditional, due to its low cost.
    * For LLVersionInfo specifically, certain initialization involved calling
      methods that have become non-static. This was moved to initSingleton()
      because, in a constructor body, 'this' does not yet point to the enclosing
      class.
    * Delete cleanupClass() method.
    * There is already a generic LLSingletonBase::deleteAll() call in
      LLAppViewer::cleanup(). It might work to let this new LLSingleton be cleaned
      up with all the rest. But if there are side effects that must happen at that
      moment, replace LLSomeSingleton::cleanupClass() with
      LLSomeSingleton::deleteSingleton(). That said, much of the benefit of
      converting to LLSingleton is deleteAll()'s guarantee that cross-LLSingleton
      dependencies will be properly honored: we're trying to migrate the code base
      away from the present fragile manual cleanup sequence.
    * Most cleanupClass() cleanup can be done in the destructor, as would normally
      be the case.
    * Cleanup that might throw an exception should be moved to cleanupSingleton().
      Override 'void cleanupSingleton();' should be private.
    * Within LLSomeSingleton methods, remove any existing
      LLSomeSingleton::methodName() qualification: simple methodName() is better.
    * In the rest of the code base, convert most LLSomeSingleton::methodName()
      references to LLSomeSingleton::instance().methodName(). (Prefer instance() to
      getInstance() because a reference does not admit the possibility of NULL.)
    * Of course, LLSomeSingleton::ENUM_VALUE can remain unchanged.
    
    In general, for many successive references to an LLSingleton instance, it
    can be useful to capture the instance() as in:
    
    auto& versionInfo{LLVersionInfo::instance()};
    // ... versionInfo.getVersion() ...
    
    We did not do that here only to simplify the code review.
    
    The STRINGIZE(expression) macro encapsulates:
    std::ostringstream out;
    out << expression;
    return out.str();
    We used that in a couple places.
    
    For LLVersionInfo specifically, lllogininstance_test.cpp used to dummy out a
    couple specific static methods. It's harder to dummy out
    LLSingleton::instance() references, so we add the real class to that test.
Code owners
Assign users and groups as approvers for specific file changes. Learn more.