Daniel J H's Code Review: Master Ticket

[daniel-j-h]

I'm in Berlin this week, where @joto @kkaefer and I are co-working for a couple of days, exchanging knowledge, tricks and sharing the pain. Today was mapbox-gl-native day for me.

The following is what I found looking over the code base (master@ e5f136a). I started with the platform specific code and could finish that in a handful of hours. There's more to come, as soon as I can spare some time and @mapbox/directions let's me do this :P

• Whole codebase:
  • throw std::runtime_error{"Ooops"}; instead of throw new std::runtime_error{"Oops"};
  • mark all classes and structs final where there is no virtual dtor (no intend of polymorphism) to avoid UB and help compiler in devirtualization
  • no need to mark member functions as inline when they are defined inside the class definition; maybe grep through all header for inline and check manually
  • mark all operator bool explicit, otherwise you will experience funny implicit conversion issues
  • catch exceptions by const ref
• In platform/android/native_map_view.cpp
  • configList is std::list, but you only push_back and then even .sort(). Use a std::vector, use .emplace_back() with the tuple's arguments and then std::sort that. Or even better, don't sort it at all, since you want the best => std::min_element. By the way, is this really the best, sorting the tuples lexicographically?
  • the ConfigProperties type is std::tuple with 6 types, some of which are the same: just make a small local type for that (e.g. struct with named member attributes)
• In platform/node/src/util/async_queue.hpp:
  • do as little as possible under the locks: std::make_unique(std::move(data)); (without specifying the type, after all type inference is why you are using make_unique) outside the lock, then lock and .push(std::move(uptr)); under the lock
  • process() spins on empty queue, contention on the lock, better: std::condition_variable
  • instead of storing the callback in a std::function that allocates memory, better template it typename Callback, then take it by value Callback callback, and store it in a member attribute of type Callback. This does not allocate memory, and is likely to get inlined when passing a lambda
  • the loop in process() should destroy the item after the callback ran; also, passing a T& to the callback is strange, when you really transfer ownership; better: callback takes a T by value, and then on the call site std::move the *item into the callback
  • maybe refactor with uv.hpp abstraction?
• In platform/node/src/node_log.cpp:
  • NodeLogOserver's constructor: std::move arguments into their sink, e.g. the std::string to avoic a potential copy (probably everywhere in codebase)
• In platform/node/src/node_map.cpp:
  • return by const value does not make sense, probably in the whole code base --- there should be compiler warnings for that
  • NodeMap::ParseOptions returns a std::unique_ptr: this does not make sense, better: just return a RenderOptions type by value
• In platform/default/asset_request_fs.cpp:
  • only file fds are of interest, consider using fopen, fclose if applicable
  • wrap fopen, fclose with Rule-of-Zero technique in RAII object, see 1 and 2
  • also see the readOne, readMany abstractions ^
  • do not take callbacks a std::function, since it allocates memory, better: take a templated typename Callback by value, also helps optimizer in inlining
  • createRequest should probably return a unique_ptr instead of a newed raw pointer
• In platform/default/asset_request_zip.cpp:
  • same RAII wrapper as above for file, this time with zip_fdopen and zip_fclose; once you have the file abstraction, this wrapper simply takes a file, and uses the unique_ptr Rule-of-Zero ownership technique to handle the zip resource around the file abstraction
  • instead of a map<string, forward_list<>> just use a unordered_multimap
  • in getHandle the handles[path] creates a default constructed item in case path is not in there --- this is probably not what you want
• In platform/default/asset_root.cpp:
  • copy pasted from application_root.cpp ?!
• In platform/default/async_task.cpp:
  • instead of std::function, consider taking a F fn by value
• In platform/default/glfw_view.cpp:
  • just use a mersenne twister engine from the random header, you're using C++14 after all
  • make_shared() does not make sense, you use the make_ functions for type inference after all, that is to not having to specify the type T; just return {a, b}; and use the shared_ptr's constructor
  • in all add*Annotations: .reserve() the vector's capacity upfront, you know how many items you insert
  • please come back to me and explain me the 4.000244140625 in onScroll
  • make delta in onScroll const, there's a lot of const-potential in the whole code base in general
  • in run: hoist variables outside the loop, help the optimizer: this is a hot path after all
  • in show_Image: take name by const-ref or const char_ if that's all what you really need
• In platform/default/headless_display.cpp
  • return after throw is dead code
• In platform/default/headless_view.cpp
  • empty line at top, argh my ocd is killing me
  • move the shared_ptrs into their sink in the constructors, otherwise you have an additional refcount increment and decrement
  • isActive can be const
  • functions in the HeadlessView destructor can throw, throwing from destructor is bad: wrap in try-catch, log or exit on exception; I would check the whole codebase for that. I think if you mark them noexcept, the standard guarentees for std::terminate to be called; at least it's not UB.
• In platform/default/http_request-curl.cpp:
  • hardcoded assets/ca-bundle.crt in multiple places
  • in sslctx_function: the comments help nothing, this is a mess :-( the whole file could profit from some abstractions
  • HTTPCURLRequest: again hardcodes cert, also user agent etc.
  • cancel function delete's the HTTPCURLRequest (more of this in the file): hopefully no one ever creates an object of this on the stack
• In platform/default/image.cpp:
  • sprintf with size 96 is somewhat ugly, and looks error prone
  • PremultipliedImage copy should probably be done with a proper copy constructor and then take a PremultipliedImageby value, so you automatically get copy
  • struct ptrs is what make_unique<png_bytep[]> returns
  • could benefit from a png RAII abstraction, for all the init, destroy logic
  • in decodeImage: I suppose the magic bytes are header signatures for png and jpeg respectively; sensible variable names would be great
• In platform/default/jpeg_reader.cpp:
  • this includes Boost.IOStream, but the docs did not say I have to install Boost
  • do not return move(x);
• In platform/default/png_reader.cpp:
  • this uses Boost.IOStream's stream and file abstractions, what about the file abstraction from before
• In platform/default/run_loop.cpp:
  • RunLoop::Impl why explicitely marking the default constructor as default; also *loop = nullptr;
  • avoid refcount increment, decrement by moving the task into the queue in RunLoop::push
• In platform/default/settings_json.cpp:
  • each operator>> and each operator<< may fail
• In platform/default/sqlite3.cpp:
  • snprintf with size 96 is ugly and error prone
  • in Database::Database: you don't have to set the db to nullptr; I think the plan here was to make sure the destructor does not clean up the db, but throwing from a constructor does not invoke the destructor by the standard, as the object is not yet fully constructed
  • no need to manually write move assign operator and constructor for the Database, just implement a RAII wrapper for sqlite3 and statement, and then all the Database has to do is store objects of those types per Rule-of-Zero
• In platform/default/sqlite_cache.cpp:
  • schema hardcoded, slit this from the implementation code
  • in createSchema: do not deleteFile if path specifies a memory db
  • SQLiteCache::get should be const
  • SQLiteCache::Impl::get probably should create an enum so that there are names instead of positional arguments for binding (same for put, refresh)
  • what i smasterPtr good for; why not simply a meyers singleton
• In platform/default/string_stdlib.cpp:
  • no need for stringstreams, just use a std::string with its append member function

[incanus]

Great review @daniel-j-h, can't wait to go over it in a bit more detail.

[tmpsantos]

platform/node/src/util/async_queue.hpp

We can probably remove the AsyncQueue from node and replace it with util::WorkQueue that does basically the same.

[mikemorris]

We can probably remove the AsyncQueue from node and replace it with util::WorkQueue that does basically the same.

Yea, that sounds like a good idea. AsyncQueue was originally in mbgl core when it was pulled into Node bindings as a dependency, then later moved to platform/node when it was removed from core.

[jfirebaugh]

AsyncQueue will no longer be necessary once #2909 is finished. Let's just leave it for now.

[jfirebaugh]

Most of these have been addressed.