Use -Werror and -isystem

[springmeyer]

All C++ code at Mapbox should be able to run warning free. We should follow the approach of mapbox/node-cpp-skel#42 to ensure that any warnings in the Carmen Cache code become errors, we fix them, and then going forward we'll be protected from bugs that might be found if we were paying attention to all warnings from various compilers.

[apendleton]

We didn't get to this one in #116 but we did resolve all warnings on Clang. We do still have warnings on GCC, so figuring those out would be the barrier to getting this done. Some of them would probably just involve adding more warning pragma instructions, but at least one (our use of the __int128 type), I'm not sure how to resolve.

[springmeyer]

we did resolve all warnings on Clang

Awesome.

We do still have warnings on GCC

Looks like the majority of the g++ compile warnings would be resolved by following mapbox/node-cpp-skel#42 and using -isystem for headers coming from mason and nan.

Within the noise are some ones for carmen-cache that should be fixed. They arise because of -Weffc++, which we enable specifically because it has better warnings in g++ than clang++:

../src/cpp_util.hpp:69:8: warning: ‘struct carmen::PhrasematchSubq’ has pointer data members [-Weffc++]
 struct PhrasematchSubq {
        ^~~~~~~~~~~~~~~
../src/cpp_util.hpp:69:8: warning:   but does not override ‘carmen::PhrasematchSubq(const carmen::PhrasematchSubq&)’ [-Weffc++]
../src/cpp_util.hpp:69:8: warning:   or ‘operator=(const carmen::PhrasematchSubq&)’ [-Weffc++]

This warning is a signal that we don't make clear in the code whether a shallow copy or deep copy of the cache pointer is needed/expected, which is important per https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)

[springmeyer]

I think this work is still important. If we don't pay attention to warnings we may be missing lurking bugs. The next step here would be to pull lots of the latest tooling from node-cpp-skel, which would take some care and manual fixes, but would bring in tooling and compiler tricks that would help catch and prevent bugs.

One gocha however is that node-cpp-skel latest has moved to mason-js and bundling its own clang++. This means that we can no longer ask for a source compile and must depend on binaries. After @miccolis moved us back to binaries for rocksdb this should be okay, but if in the future you need to source compile rocksdb again, you'd be stuck since mason-js only supports binaries and does not have a build command. If that point ever came you could use the mason bash scripts again for rocksdb, which would be somewhat messy (to use both mason bash and mason-js) but should work okay.