Lint fixes for fml, tools subdirs (flutter#19990)

This does lint fixes for the fml and tools subdirs.

Author: gspencergoog

fml/ascii_trie.cc

@@ -1,7 +1,7 @@
 // Copyright 2013 The Flutter Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-// FLUTTER_NOLINT
+
 #include "flutter/fml/ascii_trie.h"
 #include "flutter/fml/logging.h"
 
@@ -38,7 +38,7 @@ bool AsciiTrie::Query(TrieNode* trie, const char* query) {
   FML_DCHECK(trie);
   const char* char_position = query;
   TrieNode* trie_position = trie;
-  TrieNode* child;
+  TrieNode* child = nullptr;
   int ch;
   while ((ch = *char_position) && (child = trie_position->children[ch].get())) {
     char_position++;

fml/command_line.cc

@@ -1,7 +1,6 @@
 // Copyright 2013 The Flutter Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-// FLUTTER_NOLINT
 
 #include "flutter/fml/command_line.h"
 
@@ -27,8 +26,9 @@ CommandLine::CommandLine(const std::string& argv0,
       argv0_(argv0),
       options_(options),
       positional_args_(positional_args) {
-  for (size_t i = 0; i < options_.size(); i++)
+  for (size_t i = 0; i < options_.size(); i++) {
     option_index_[options_[i].name] = i;
+  }
 }
 
 CommandLine::~CommandLine() = default;
@@ -39,18 +39,21 @@ CommandLine& CommandLine::operator=(CommandLine&& from) = default;
 
 bool CommandLine::HasOption(std::string_view name, size_t* index) const {
   auto it = option_index_.find(name.data());
-  if (it == option_index_.end())
+  if (it == option_index_.end()) {
     return false;
-  if (index)
+  }
+  if (index) {
     *index = it->second;
+  }
   return true;
 }
 
 bool CommandLine::GetOptionValue(std::string_view name,
                                  std::string* value) const {
   size_t index;
-  if (!HasOption(name, &index))
+  if (!HasOption(name, &index)) {
     return false;
+  }
   *value = options_[index].value;
   return true;
 }
@@ -59,8 +62,9 @@ std::vector<std::string_view> CommandLine::GetOptionValues(
     std::string_view name) const {
   std::vector<std::string_view> ret;
   for (const auto& option : options_) {
-    if (option.name == name)
+    if (option.name == name) {
       ret.push_back(option.value);
+    }
   }
   return ret;
 }
@@ -69,8 +73,9 @@ std::string CommandLine::GetOptionValueWithDefault(
     std::string_view name,
     std::string_view default_value) const {
   size_t index;
-  if (!HasOption(name, &index))
+  if (!HasOption(name, &index)) {
     return {default_value.data(), default_value.size()};
+  }
   return options_[index].value;
 }
 
@@ -125,16 +130,18 @@ bool CommandLineBuilder::ProcessArg(const std::string& arg) {
 }
 
 CommandLine CommandLineBuilder::Build() const {
-  if (!has_argv0_)
+  if (!has_argv0_) {
     return CommandLine();
+  }
   return CommandLine(argv0_, options_, positional_args_);
 }
 
 }  // namespace internal
 
 std::vector<std::string> CommandLineToArgv(const CommandLine& command_line) {
-  if (!command_line.has_argv0())
+  if (!command_line.has_argv0()) {
     return std::vector<std::string>();
+  }
 
   std::vector<std::string> argv;
   const std::vector<CommandLine::Option>& options = command_line.options();
@@ -146,17 +153,19 @@ std::vector<std::string> CommandLineToArgv(const CommandLine& command_line) {
 
   argv.push_back(command_line.argv0());
   for (const auto& option : options) {
-    if (option.value.empty())
+    if (option.value.empty()) {
       argv.push_back("--" + option.name);
-    else
+    } else {
       argv.push_back("--" + option.name + "=" + option.value);
+    }
   }
 
   if (!positional_args.empty()) {
     // Insert a "--" if necessary.
     if (positional_args[0].size() >= 2u && positional_args[0][0] == '-' &&
-        positional_args[0][1] == '-')
+        positional_args[0][1] == '-') {
       argv.push_back("--");
+    }
 
     argv.insert(argv.end(), positional_args.begin(), positional_args.end());
   }

fml/icu_util.cc

@@ -1,7 +1,6 @@
 // Copyright 2013 The Flutter Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-// FLUTTER_NOLINT
 
 #include "flutter/fml/icu_util.h"
 
@@ -20,11 +19,12 @@ namespace icu {
 
 class ICUContext {
  public:
-  ICUContext(const std::string& icu_data_path) : valid_(false) {
+  explicit ICUContext(const std::string& icu_data_path) : valid_(false) {
     valid_ = SetupMapping(icu_data_path) && SetupICU();
   }
 
-  ICUContext(std::unique_ptr<Mapping> mapping) : mapping_(std::move(mapping)) {
+  explicit ICUContext(std::unique_ptr<Mapping> mapping)
+      : mapping_(std::move(mapping)) {
     valid_ = SetupICU();
   }
 

fml/logging.cc

@@ -1,7 +1,6 @@
 // Copyright 2013 The Flutter Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-// FLUTTER_NOLINT
 
 #include <algorithm>
 #include <iostream>
@@ -23,23 +22,26 @@ const char* const kLogSeverityNames[LOG_NUM_SEVERITIES] = {"INFO", "WARNING",
                                                            "ERROR", "FATAL"};
 
 const char* GetNameForLogSeverity(LogSeverity severity) {
-  if (severity >= LOG_INFO && severity < LOG_NUM_SEVERITIES)
+  if (severity >= LOG_INFO && severity < LOG_NUM_SEVERITIES) {
     return kLogSeverityNames[severity];
+  }
   return "UNKNOWN";
 }
 
 const char* StripDots(const char* path) {
-  while (strncmp(path, "../", 3) == 0)
+  while (strncmp(path, "../", 3) == 0) {
     path += 3;
+  }
   return path;
 }
 
 const char* StripPath(const char* path) {
   auto* p = strrchr(path, '/');
-  if (p)
+  if (p) {
     return p + 1;
-  else
+  } else {
     return path;
+  }
 }
 
 }  // namespace
@@ -50,15 +52,17 @@ LogMessage::LogMessage(LogSeverity severity,
                        const char* condition)
     : severity_(severity), file_(file), line_(line) {
   stream_ << "[";
-  if (severity >= LOG_INFO)
+  if (severity >= LOG_INFO) {
     stream_ << GetNameForLogSeverity(severity);
-  else
+  } else {
     stream_ << "VERBOSE" << -severity;
+  }
   stream_ << ":" << (severity > LOG_INFO ? StripDots(file_) : StripPath(file_))
           << "(" << line_ << ")] ";
 
-  if (condition)
+  if (condition) {
     stream_ << "Check failed: " << condition << ". ";
+  }
 }
 
 LogMessage::~LogMessage() {

fml/memory/ref_counted_unittest.cc

@@ -1,7 +1,6 @@
 // Copyright 2013 The Flutter Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-// FLUTTER_NOLINT
 
 // This file tests both ref_counted.h and ref_ptr.h (which the former includes).
 // TODO(vtl): Possibly we could separate these tests out better, since a lot of
@@ -47,12 +46,14 @@ class MyClass : public RefCountedThreadSafe<MyClass> {
  protected:
   MyClass(MyClass** created, bool* was_destroyed)
       : was_destroyed_(was_destroyed) {
-    if (created)
+    if (created) {
       *created = this;
+    }
   }
   virtual ~MyClass() {
-    if (was_destroyed_)
+    if (was_destroyed_) {
       *was_destroyed_ = true;
+    }
   }
 
  private:
@@ -71,8 +72,9 @@ class MySubclass final : public MyClass {
 
   MySubclass(MySubclass** created, bool* was_destroyed)
       : MyClass(nullptr, was_destroyed) {
-    if (created)
+    if (created) {
       *created = this;
+    }
   }
   ~MySubclass() override {}
 
@@ -225,7 +227,7 @@ TEST(RefCountedTest, NullAssignmentToNull) {
   // No-op null assignment using move constructor.
   r1 = std::move(r2);
   EXPECT_TRUE(r1.get() == nullptr);
-  EXPECT_TRUE(r2.get() == nullptr);
+  EXPECT_TRUE(r2.get() == nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_FALSE(r1);
   EXPECT_FALSE(r2);
 
@@ -270,7 +272,7 @@ TEST(RefCountedTest, NonNullAssignmentToNull) {
     RefPtr<MyClass> r2;
     // Move assignment (to null ref pointer).
     r2 = std::move(r1);
-    EXPECT_TRUE(r1.get() == nullptr);
+    EXPECT_TRUE(r1.get() == nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
     EXPECT_EQ(created, r2.get());
     EXPECT_FALSE(r1);
     EXPECT_TRUE(r2);
@@ -334,7 +336,7 @@ TEST(RefCountedTest, NullAssignmentToNonNull) {
   // Null assignment using move constructor.
   r1 = std::move(r2);
   EXPECT_TRUE(r1.get() == nullptr);
-  EXPECT_TRUE(r2.get() == nullptr);
+  EXPECT_TRUE(r2.get() == nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
   EXPECT_FALSE(r1);
   EXPECT_FALSE(r2);
   EXPECT_TRUE(was_destroyed);
@@ -387,7 +389,7 @@ TEST(RefCountedTest, NonNullAssignmentToNonNull) {
     RefPtr<MyClass> r2(MakeRefCounted<MyClass>(nullptr, &was_destroyed2));
     // Move assignment (to non-null ref pointer).
     r2 = std::move(r1);
-    EXPECT_TRUE(r1.get() == nullptr);
+    EXPECT_TRUE(r1.get() == nullptr);  // NOLINT(clang-analyzer-cplusplus.Move)
     EXPECT_FALSE(r2.get() == nullptr);
     EXPECT_FALSE(r1);
     EXPECT_TRUE(r2);
@@ -596,13 +598,14 @@ TEST(RefCountedTest, PublicCtorAndDtor) {
 TEST(RefCountedTest, DebugChecks) {
   {
     MyPublicClass* p = new MyPublicClass();
-    EXPECT_DEATH_IF_SUPPORTED(delete p, "!adoption_required_");
+    EXPECT_DEATH_IF_SUPPORTED(  // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
+        delete p, "!adoption_required_");
   }
 
   {
     MyPublicClass* p = new MyPublicClass();
-    EXPECT_DEATH_IF_SUPPORTED(RefPtr<MyPublicClass> r(p),
-                              "!adoption_required_");
+    EXPECT_DEATH_IF_SUPPORTED(  // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
+        RefPtr<MyPublicClass> r(p), "!adoption_required_");
   }
 
   {

fml/memory/ref_ptr.h

@@ -66,42 +66,48 @@ template <typename T>
 class RefPtr final {
  public:
   RefPtr() : ptr_(nullptr) {}
-  RefPtr(std::nullptr_t) : ptr_(nullptr) {}
+  RefPtr(std::nullptr_t)
+      : ptr_(nullptr) {}  // NOLINT(google-explicit-constructor)
 
   // Explicit constructor from a plain pointer (to an object that must have
   // already been adopted). (Note that in |T::T()|, references to |this| cannot
   // be taken, since the object being constructed will not have been adopted
   // yet.)
   template <typename U>
   explicit RefPtr(U* p) : ptr_(p) {
-    if (ptr_)
+    if (ptr_) {
       ptr_->AddRef();
+    }
   }
 
   // Copy constructor.
   RefPtr(const RefPtr<T>& r) : ptr_(r.ptr_) {
-    if (ptr_)
+    if (ptr_) {
       ptr_->AddRef();
+    }
   }
 
   template <typename U>
-  RefPtr(const RefPtr<U>& r) : ptr_(r.ptr_) {
-    if (ptr_)
+  RefPtr(const RefPtr<U>& r)
+      : ptr_(r.ptr_) {  // NOLINT(google-explicit-constructor)
+    if (ptr_) {
       ptr_->AddRef();
+    }
   }
 
   // Move constructor.
   RefPtr(RefPtr<T>&& r) : ptr_(r.ptr_) { r.ptr_ = nullptr; }
 
   template <typename U>
-  RefPtr(RefPtr<U>&& r) : ptr_(r.ptr_) {
+  RefPtr(RefPtr<U>&& r) : ptr_(r.ptr_) {  // NOLINT(google-explicit-constructor)
     r.ptr_ = nullptr;
   }
 
   // Destructor.
   ~RefPtr() {
-    if (ptr_)
+    if (ptr_) {
       ptr_->Release();
+    }
   }
 
   T* get() const { return ptr_; }
@@ -118,25 +124,34 @@ class RefPtr final {
 
   // Copy assignment.
   RefPtr<T>& operator=(const RefPtr<T>& r) {
-    // Call |AddRef()| first so self-assignments work.
-    if (r.ptr_)
+    // Handle self-assignment.
+    if (r.ptr_ == ptr_) {
+      return *this;
+    }
+    if (r.ptr_) {
       r.ptr_->AddRef();
+    }
     T* old_ptr = ptr_;
     ptr_ = r.ptr_;
-    if (old_ptr)
+    if (old_ptr) {
       old_ptr->Release();
+    }
     return *this;
   }
 
   template <typename U>
   RefPtr<T>& operator=(const RefPtr<U>& r) {
-    // Call |AddRef()| first so self-assignments work.
-    if (r.ptr_)
+    if (reinterpret_cast<T*>(r.ptr_) == ptr_) {
+      return *this;
+    }
+    if (r.ptr_) {
       r.ptr_->AddRef();
+    }
     T* old_ptr = ptr_;
     ptr_ = r.ptr_;
-    if (old_ptr)
+    if (old_ptr) {
       old_ptr->Release();
+    }
     return *this;
   }