Add clang-tidy checks for Firestore

Firestore/core/.clang-tidy

@@ -0,0 +1,34 @@
+---
+# cert-*
+#   -cert-dcl50-cpp
+#     We use variadic functions
+#   -cert-err58-cpp
+#     GoogleTest creates instances in static scope in a way that trips this
+#     warning for every test.
+# readability-*
+#   -readability-else-after-return
+#   -readability-implicit-bool-conversion
+#     These checks generate a bunch of noise that we're just not religious
+#     about.
+# modernize-*
+#   -modernize-use-equals-default
+#     VS 2015 and Xcode <= 8.2 don't fully support this so we don't use
+#     `= default`.
+#   -modernize-use-equals-delete
+#     GoogleTest generates test classes that use the old idiom of making
+#     default constructors and operator= private.
+Checks:          'bugprone-*,cert-*,-cert-dcl50-cpp,-cert-err58-cpp,google-*,objc-*,readability-*,-readability-else-after-return,-readability-implicit-bool-conversion,misc-*,modernize-*,-modernize-use-equals-default,-modernize-use-equals-delete,clang-diagnostic-*,clang-analyzer-*'
+WarningsAsErrors: ''
+HeaderFilterRegex: ''
+CheckOptions:
+  - key:             readability-braces-around-statements.ShortStatementLines
+    value:           '1'
+  - key:             google-readability-braces-around-statements.ShortStatementLines
+    value:           '1'
+  - key:             google-readability-function-size.StatementThreshold
+    value:           '800'
+  - key:             google-readability-namespace-comments.ShortNamespaceLines
+    value:           '10'
+  - key:             google-readability-namespace-comments.SpacesBeforeComments
+    value:           '2'
+...

Firestore/core/include/firebase/firestore/document_reference.h

@@ -16,7 +16,7 @@
 
 // TODO(rsgowman): This file isn't intended to be used just yet. It's just an
 // outline of what the API might eventually look like. Most of this was
-// shamelessly stolen and modified from rtdb's header file, melded with the
+// shamelessly stolen and modified from RTDB's header file, melded with the
 // (java) firestore api.
 
 #ifndef FIRESTORE_CORE_INCLUDE_FIREBASE_FIRESTORE_DOCUMENT_REFERENCE_H_
@@ -36,7 +36,7 @@
 // here so we don't forget to mention this during the API review, and should be
 // removed once this note has migrated to the API review doc.
 
-// TODO(rsgowman): replace these forward decl's with appropriate includes (once
+// TODO(rsgowman): replace these forward decls with appropriate includes (once
 // they exist)
 namespace firebase {
 class App;
@@ -47,7 +47,7 @@ class Future;
 namespace firebase {
 namespace firestore {
 
-// TODO(rsgowman): replace these forward decl's with appropriate includes (once
+// TODO(rsgowman): replace these forward decls with appropriate includes (once
 // they exist)
 class FieldValue;
 class DocumentSnapshot;
@@ -361,7 +361,7 @@ namespace std {
 // C++ style guide. But we think this is probably ok in this case since:
 // a) It's the standard way of doing this outside of Google (as the style guide
 // itself points out), and
-// b) This has a straightfoward hash function anyway (just hash the path) so I
+// b) This has a straightforward hash function anyway (just hash the path) so I
 // don't think the concerns in the style guide are going to bite us.
 //
 // Raise this concern during the API review.

Firestore/core/src/firebase/firestore/auth/credentials_provider.h

@@ -31,10 +31,10 @@ namespace firestore {
 namespace auth {
 
 // `TokenErrorListener` is a listener that gets a token or an error.
-typedef std::function<void(util::StatusOr<Token>)> TokenListener;
+using TokenListener = std::function<void(util::StatusOr<Token>)>;
 
 // Listener notified with a User change.
-typedef std::function<void(User user)> UserChangeListener;
+using UserChangeListener = std::function<void(User user)>;
 
 /**
  * Provides methods for getting the uid and token for the current user and

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h

@@ -78,7 +78,7 @@ class FirebaseCredentialsProvider : public CredentialsProvider {
    */
   struct Contents {
     Contents(FIRApp* app, User&& user)
-        : app(app), current_user(std::move(user)), mutex() {
+        : app(app), current_user(std::move(user)) {
     }
 
     const FIRApp* app;

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm

@@ -24,6 +24,7 @@
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 
 // NB: This is also defined in Firestore/Source/Public/FIRFirestoreErrors.h
+// NOLINTNEXTLINE: public constant
 NSString* const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain";
 
 namespace firebase {
@@ -47,7 +48,7 @@
                 std::unique_lock<std::mutex> lock(contents->mutex);
                 NSDictionary<NSString*, id>* user_info = notification.userInfo;
 
-                // ensure we're only notifiying for the current app.
+                // ensure we're only notifying for the current app.
                 FIRApp* notified_app =
                     user_info[FIRAuthStateDidChangeInternalNotificationAppKey];
                 if (![contents->app isEqual:notified_app]) {

Firestore/core/src/firebase/firestore/auth/token.h

@@ -42,7 +42,7 @@ namespace auth {
 // TODO(zxu123): Make this support token-type for desktop workflow.
 class Token {
  public:
-  Token(const absl::string_view token, const User& user);
+  Token(absl::string_view token, const User& user);
 
   /** The actual raw token. */
   const std::string& token() const {

Firestore/core/src/firebase/firestore/auth/user.cc

@@ -22,7 +22,7 @@ namespace firebase {
 namespace firestore {
 namespace auth {
 
-User::User() : uid_(), is_authenticated_(false) {
+User::User() : is_authenticated_(false) {
 }
 
 User::User(const absl::string_view uid) : uid_(uid), is_authenticated_(true) {

Firestore/core/src/firebase/firestore/auth/user.h

@@ -41,7 +41,7 @@ class User {
   User();
 
   /** Construct an authenticated user with the given UID. */
-  explicit User(const absl::string_view uid);
+  explicit User(absl::string_view uid);
 
   const std::string& uid() const {
     return uid_;

Firestore/core/src/firebase/firestore/core/database_info.h

@@ -41,12 +41,12 @@ class DatabaseInfo {
    * @param database_id The project/database to use.
    * @param persistence_key A unique identifier for this Firestore's local
    *        storage. Usually derived from -[FIRApp appName].
-   * @param host The hostname of the datastore backend.
+   * @param host The hostname of the Firestore backend.
    * @param ssl_enabled Whether to use SSL when connecting.
    */
   DatabaseInfo(const firebase::firestore::model::DatabaseId& database_id,
-               const absl::string_view persistence_key,
-               const absl::string_view host,
+               absl::string_view persistence_key,
+               absl::string_view host,
                bool ssl_enabled);
 
   const firebase::firestore::model::DatabaseId& database_id() const {

Firestore/core/src/firebase/firestore/immutable/array_sorted_map.h

@@ -70,8 +70,8 @@ class FixedArray {
    */
   template <typename SourceIterator>
   void append(SourceIterator src_begin, SourceIterator src_end) {
-    size_type appending = static_cast<size_type>(src_end - src_begin);
-    size_type new_size = size_ + appending;
+    auto appending = static_cast<size_type>(src_end - src_begin);
+    auto new_size = size_ + appending;
     FIREBASE_ASSERT(new_size <= fixed_size);
 
     std::copy(src_begin, src_end, end());
@@ -229,8 +229,6 @@ class ArraySortedMap : public SortedMapBase {
     }
   }
 
-  // TODO(wilhuff): indexof
-
   /** Returns true if the map contains no elements. */
   bool empty() const {
     return size() == 0;

Firestore/core/src/firebase/firestore/immutable/map_entry.h

@@ -33,7 +33,7 @@ namespace immutable {
  */
 template <typename K, typename V, typename C = std::less<K>>
 struct KeyComparator {
-  typedef std::pair<K, V> pair_type;
+  using pair_type = std::pair<K, V>;
 
   explicit KeyComparator(const C& comparator = C())
       : key_comparator_(comparator) {

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

@@ -373,7 +373,7 @@ DocumentKey Reader::ReadDocumentKey() {
   }
 
   ResourcePath path{std::move(path_segments)};
-  if (ok_ && path.size() > 0 && DocumentKey::IsDocumentKey(path)) {
+  if (ok_ && !path.empty() && DocumentKey::IsDocumentKey(path)) {
     return DocumentKey{std::move(path)};
   }
 

Firestore/core/src/firebase/firestore/model/database_id.h

@@ -44,8 +44,7 @@ class DatabaseId {
    * @param project_id The project for the database.
    * @param database_id The database in the project to use.
    */
-  DatabaseId(const absl::string_view project_id,
-             const absl::string_view database_id);
+  DatabaseId(absl::string_view project_id, absl::string_view database_id);
 
   const std::string& project_id() const {
     return project_id_;

Firestore/core/src/firebase/firestore/model/document.cc

@@ -39,7 +39,7 @@ bool Document::Equals(const MaybeDocument& other) const {
   if (other.type() != Type::Document) {
     return false;
   }
-  const Document& other_doc = static_cast<const Document&>(other);
+  auto& other_doc = static_cast<const Document&>(other);
   return MaybeDocument::Equals(other) &&
          has_local_mutations_ == other_doc.has_local_mutations_ &&
          data_ == other_doc.data_;

Firestore/core/src/firebase/firestore/model/field_mask.h

@@ -44,9 +44,7 @@ class FieldMask {
 
   FieldMask(std::initializer_list<FieldPath> list) : fields_{list} {
   }
-  explicit FieldMask(const std::vector<FieldPath>& fields) : fields_{fields} {
-  }
-  explicit FieldMask(std::vector<FieldPath>&& fields)
+  explicit FieldMask(std::vector<FieldPath> fields)
       : fields_{std::move(fields)} {
   }
 

Firestore/core/src/firebase/firestore/model/field_value.cc

@@ -38,7 +38,7 @@ namespace {
 /**
  * This deviates from the other platforms that define TypeOrder. Since
  * we already define Type for union types, we use it together with this
- * function to achive the equivalent order of types i.e.
+ * function to achieve the equivalent order of types i.e.
  *     i) if two types are comparable, then they are of equal order;
  *    ii) otherwise, their order is the same as the order of their Type.
  */
@@ -148,7 +148,8 @@ FieldValue& FieldValue::operator=(FieldValue&& value) {
       return *this;
     default:
       // We just copy over POD union types.
-      return *this = value;
+      *this = value;
+      return *this;
   }
 }
 

Firestore/core/src/firebase/firestore/model/field_value.h

@@ -163,7 +163,7 @@ class FieldValue {
   /**
    * Switch to the specified type, if different from the current type.
    */
-  void SwitchTo(const Type type);
+  void SwitchTo(Type type);
 
   Type tag_ = Type::Null;
   union {

Firestore/core/src/firebase/firestore/remote/serializer.cc

@@ -294,7 +294,7 @@ void Writer::WriteVarint(uint64_t value) {
  * Note that (despite the return type) this works for bool, enum, int32, int64,
  * uint32 and uint64 proto field types.
  *
- * Note: This is not expected to be called direclty, but rather only via the
+ * Note: This is not expected to be called directly, but rather only via the
  * other Decode* methods (i.e. DecodeBool, DecodeLong, etc)
  *
  * @return The decoded varint as a uint64_t.

Firestore/core/src/firebase/firestore/remote/serializer.h

@@ -72,7 +72,7 @@ class Serializer {
   // TODO(rsgowman): If we never support any output except to a vector, it may
   // make sense to have Serializer own the vector and provide an accessor rather
   // than asking the user to create it first.
-  static util::Status EncodeFieldValue(
+  util::Status EncodeFieldValue(
       const firebase::firestore::model::FieldValue& field_value,
       std::vector<uint8_t>* out_bytes);
 
@@ -84,8 +84,7 @@ class Serializer {
    * @return The model equivalent of the bytes.
    */
   // TODO(rsgowman): error handling.
-  static firebase::firestore::model::FieldValue DecodeFieldValue(
-      const uint8_t* bytes, size_t length);
+  model::FieldValue DecodeFieldValue(const uint8_t* bytes, size_t length);
 
   /**
    * @brief Converts from bytes to the model FieldValue format.
@@ -95,14 +94,13 @@ class Serializer {
    * @return The model equivalent of the bytes.
    */
   // TODO(rsgowman): error handling.
-  static firebase::firestore::model::FieldValue DecodeFieldValue(
-      const std::vector<uint8_t>& bytes) {
+  model::FieldValue DecodeFieldValue(const std::vector<uint8_t>& bytes) {
     return DecodeFieldValue(bytes.data(), bytes.size());
   }
 
  private:
   // TODO(rsgowman): We don't need the database_id_ yet (but will eventually).
-  // const firebase::firestore::model::DatabaseId& database_id_;
+  // model::DatabaseId* database_id_;
 };
 
 }  // namespace remote

Firestore/core/src/firebase/firestore/timestamp.cc

@@ -48,7 +48,7 @@ Timestamp Timestamp::Now() {
 }
 
 Timestamp Timestamp::FromTimeT(const time_t seconds_since_unix_epoch) {
-  return Timestamp(seconds_since_unix_epoch, 0);
+  return {seconds_since_unix_epoch, 0};
 }
 
 #if !defined(_STLPORT_VERSION)

Firestore/core/src/firebase/firestore/util/bits.h

@@ -139,23 +139,23 @@ inline int Bits::Log2FloorNonZero_Portable(uint32_t n) {
 
 // Log2Floor64() is defined in terms of Log2Floor32(), Log2FloorNonZero32()
 inline int Bits::Log2Floor64_Portable(uint64_t n) {
-  const uint32_t topbits = static_cast<uint32_t>(n >> 32);
-  if (topbits == 0) {
+  const auto top_bits = static_cast<uint32_t>(n >> 32);
+  if (top_bits == 0) {
     // Top bits are zero, so scan in bottom bits
     return Log2Floor(static_cast<uint32_t>(n));
   } else {
-    return 32 + Log2FloorNonZero(topbits);
+    return 32 + Log2FloorNonZero(top_bits);
   }
 }
 
 // Log2FloorNonZero64() is defined in terms of Log2FloorNonZero32()
 inline int Bits::Log2FloorNonZero64_Portable(uint64_t n) {
-  const uint32_t topbits = static_cast<uint32_t>(n >> 32);
-  if (topbits == 0) {
+  const auto top_bits = static_cast<uint32_t>(n >> 32);
+  if (top_bits == 0) {
     // Top bits are zero, so scan in bottom bits
     return Log2FloorNonZero(static_cast<uint32_t>(n));
   } else {
-    return 32 + Log2FloorNonZero(topbits);
+    return 32 + Log2FloorNonZero(top_bits);
   }
 }
 

Firestore/core/src/firebase/firestore/util/comparator_holder.h

@@ -47,7 +47,7 @@ class ComparatorHolder {
 template <typename C>
 class ComparatorHolder<C, true> : private C {
  protected:
-  explicit ComparatorHolder(const C&) noexcept {
+  explicit ComparatorHolder(const C& /* comparator */) noexcept {
   }
 
   const C& comparator() const noexcept {

Firestore/core/src/firebase/firestore/util/comparison.cc

@@ -78,7 +78,7 @@ ComparisonResult CompareMixedNumber(double double_value, int64_t int64_value) {
 
   // At this point the long representations are equal but this could be due to
   // rounding.
-  double int64_as_double = static_cast<double>(int64_value);
+  auto int64_as_double = static_cast<double>(int64_value);
   return Compare<double>(double_value, int64_as_double);
 }
 

Firestore/core/test/firebase/firestore/immutable/array_sorted_map_test.cc

@@ -29,7 +29,7 @@ namespace firestore {
 namespace immutable {
 namespace impl {
 
-typedef ArraySortedMap<int, int> IntMap;
+using IntMap = ArraySortedMap<int, int>;
 constexpr IntMap::size_type kFixedSize = IntMap::kFixedSize;
 
 // TODO(wilhuff): ReverseTraversal
@@ -153,7 +153,7 @@ TEST(ArraySortedMap, EmptyRemoval) {
 
 TEST(ArraySortedMap, InsertionAndRemovalOfMaxItems) {
   auto expected_size = kFixedSize;
-  int n = static_cast<int>(expected_size);
+  auto n = static_cast<int>(expected_size);
   std::vector<int> to_insert = Shuffled(Sequence(n));
   std::vector<int> to_remove = Shuffled(to_insert);
 

Firestore/core/test/firebase/firestore/immutable/sorted_map_test.cc

@@ -49,6 +49,7 @@ class SortedMapTest : public ::testing::Test {
   }
 };
 
+// NOLINTNEXTLINE: must be a typedef for the gtest macros
 typedef ::testing::Types<SortedMap<int, int>,
                          impl::ArraySortedMap<int, int>,
                          impl::TreeSortedMap<int, int>>

Firestore/core/test/firebase/firestore/immutable/tree_sorted_map_test.cc

@@ -25,7 +25,7 @@ namespace firestore {
 namespace immutable {
 namespace impl {
 
-typedef TreeSortedMap<int, int> IntMap;
+using IntMap = TreeSortedMap<int, int>;
 
 TEST(TreeSortedMap, EmptySize) {
   IntMap map;