-------------------------------------------------------------------------------- TODO -------------------------------------------------------------------------------- - Fix all leaks on exit (!) Say there's a leak, a ledger that can never be accessed is locked in some structure. If the organized teardown code frees that structure, the leak will not be reported. Yes, so you'll detect some small subset of leaks that way. You'll still have to be vigilant for the leaks that won't detect. The problem is ordering. There are lots of circular dependencies. The biggest problem is the order of destruction of global objects. (I think) Getting rid of global objects is a good solution to that. Vinnie Falco: Those I can resolve with my ReferenceCountedSingleton. And yeah thats a good approach, one that I am doing slowly anyway Yeah, that's good for other reasons too, not just the unpredictability of creation order that can hide bugs. There may also just be some missing destructors. Some of it may be things being shut down in the wrong order. Like if you shut down the cache and then something that uses the cache, objects may get put in the cache after it was shut down. - Remove "ENABLE_INSECURE" when the time is right. - lift bind, function, and placeholders into ripple namespace - lift beast into the ripple namespace, remove ripple's duplicated integer types - lift unique_ptr / auto_ptr into ripple namespace, or replace with ScopedPointer - Rewrite functions passed to bind to not take reference parameters, so we can used std::bind instead of boost. - Make LevelDB and Ripple code work with both Unicode and non-Unicode Windows APIs - Raise the warning level and fix everything - Go searching through VFALCO notes and fix everything - Deal with function-level statics used for SqliteDatabase (like in HSBESQLite::visitAll) - Document in order: SerializedType STObject SerializedLedgerEntry - Replace uint160, uint256 in argument lists, template parameter lists, and data members with tyepdefs from ripple_ProtocolTypes.h - Consolidate SQLite database classes: DatabaseCon, Database, SqliteDatabase. -------------------------------------------------------------------------------- LoadEvent Is referenced with both a shared pointer and an auto pointer. JobQueue getLoadEvent and getLoadEventAP differ only in the style of pointer container which is returned. Unnecessary complexity. -------------------------------------------------------------------------------- Naming Some names don't make sense. Index Stop using Index to refer to keys in tables. Replace with "Key" ? Index implies a small integer, or a data structure. Inconsistent names We have full names like SerializedType and then acronyms like STObject Two names for some things, e.g. SerializedLedgerEntry and SLE Shared/Smart pointer typedefs in classes have a variety of different names for the same thing. e.g. "pointer", "ptr", "ptr_t", "wptr" Verbose names The prefix "Flat" is more appealing than "Serialized" because its shorter and easier to pronounce. Ledger "Skip List" Is not really a skip list data structure Duplicate Code LedgerEntryFormat and TxFormat -------------------------------------------------------------------------------- Interfaces Serializer Upon analysis this class does two incompatible things. Flattening, and unflattening. The interface should be reimplemented as two distinct abstract classes, InputStream and OutputStream with suitable implementations such as to and from a block of memory or dynamically allocated buffer. The name and conflation of dual roles serves to confuse code at the point of call. Does set(Serializer& s) flatten or unflatten the data? This would be more clear: bool write (OutputStream& stream); -------------------------------------------------------------------------------- Implementation LoadManager What is going on in the destructor -------------------------------------------------------------------------------- boost Unclear from the class declaration what style of shared object management is used. Prefer to derive from a ReferenceCountedObject class so that the behavior is explicit. Furthermore the use of intrusive containers is preferred over the alternative. make_shared <> () is awkward. boost::recursive_mutex Recursive mutexes should never be necessary. They require the "mutable" keyword for const members to acquire the lock (yuck) Replace recursive_mutex with juce::Mutex to remove boost dependency