Clean up common dir clang-tidy warnings (#907)

common/check.h

@@ -9,8 +9,35 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 
-namespace Carbon {
-namespace Internal {
+// Raw exiting stream. This should be used when building other forms of exiting
+// macros like those below. It evaluates to a temporary `ExitingStream` object
+// that can be manipulated, streamed into, and then will exit the program.
+#define RAW_EXITING_STREAM() \
+  Carbon::Internal::ExitingStream::Helper() | Carbon::Internal::ExitingStream()
+
+// Checks the given condition, and if it's false, prints a stack, streams the
+// error message, then exits. This should be used for unexpected errors, such as
+// a bug in the application.
+//
+// For example:
+//   CHECK(is_valid) << "Data is not valid!";
+#define CHECK(condition)                                                  \
+  (condition) ? (void)0                                                   \
+              : RAW_EXITING_STREAM().TreatAsBug()                         \
+                    << "CHECK failure at " << __FILE__ << ":" << __LINE__ \
+                    << ": " #condition                                    \
+                    << Carbon::Internal::ExitingStream::AddSeparator()
+
+// This is similar to CHECK, but is unconditional. Writing FATAL() is clearer
+// than CHECK(false) because it avoids confusion about control flow.
+//
+// For example:
+//   FATAL() << "Unreachable!";
+#define FATAL()                     \
+  RAW_EXITING_STREAM().TreatAsBug() \
+      << "FATAL failure at " << __FILE__ << ":" << __LINE__ << ": "
+
+namespace Carbon::Internal {
 
 // Wraps a stream and exiting for fatal errors. Should only be used by the
 // macros below.
@@ -33,7 +60,7 @@ class ExitingStream {
 
   // Indicates that the program is exiting due to a bug in the program, rather
   // than, e.g., invalid input.
-  ExitingStream& TreatAsBug() {
+  auto TreatAsBug() -> ExitingStream& {
     treat_as_bug_ = true;
     return *this;
   }
@@ -44,7 +71,7 @@ class ExitingStream {
 
   // Forward output to llvm::errs.
   template <typename T>
-  ExitingStream& operator<<(const T& message) {
+  auto operator<<(const T& message) -> ExitingStream& {
     if (separator_) {
       llvm::errs() << ": ";
       separator_ = false;
@@ -53,7 +80,7 @@ class ExitingStream {
     return *this;
   }
 
-  ExitingStream& operator<<(AddSeparator /*unused*/) {
+  auto operator<<(AddSeparator /*unused*/) -> ExitingStream& {
     separator_ = true;
     return *this;
   }
@@ -79,36 +106,6 @@ class ExitingStream {
   bool treat_as_bug_ = false;
 };
 
-}  // namespace Internal
-
-// Raw exiting stream. This should be used when building other forms of exiting
-// macros like those below. It evaluates to a temporary `ExitingStream` object
-// that can be manipulated, streamed into, and then will exit the program.
-#define RAW_EXITING_STREAM() \
-  Carbon::Internal::ExitingStream::Helper() | Carbon::Internal::ExitingStream()
-
-// Checks the given condition, and if it's false, prints a stack, streams the
-// error message, then exits. This should be used for unexpected errors, such as
-// a bug in the application.
-//
-// For example:
-//   CHECK(is_valid) << "Data is not valid!";
-#define CHECK(condition)                                                  \
-  (condition) ? (void)0                                                   \
-              : RAW_EXITING_STREAM().TreatAsBug()                         \
-                    << "CHECK failure at " << __FILE__ << ":" << __LINE__ \
-                    << ": " #condition                                    \
-                    << Carbon::Internal::ExitingStream::AddSeparator()
-
-// This is similar to CHECK, but is unconditional. Writing FATAL() is clearer
-// than CHECK(false) because it avoids confusion about control flow.
-//
-// For example:
-//   FATAL() << "Unreachable!";
-#define FATAL()                     \
-  RAW_EXITING_STREAM().TreatAsBug() \
-      << "FATAL failure at " << __FILE__ << ":" << __LINE__ << ": "
-
-}  // namespace Carbon
+}  // namespace Carbon::Internal
 
 #endif  // COMMON_CHECK_H_

common/indirect_value.h

@@ -48,6 +48,7 @@ class IndirectValue {
   IndirectValue() : value_(std::make_unique<T>()) {}
 
   // Initializes the underlying T object as if by `T(std::move(value))`.
+  // NOLINTNEXTLINE(google-explicit-constructor): Implicit constructor.
   IndirectValue(T value) : value_(std::make_unique<T>(std::move(value))) {}
 
   // TODO(geoffromer): consider defining implicit conversions from
@@ -56,7 +57,7 @@ class IndirectValue {
   IndirectValue(const IndirectValue& other)
       : value_(std::make_unique<T>(*other)) {}
 
-  IndirectValue(IndirectValue&& other)
+  IndirectValue(IndirectValue&& other) noexcept
       : value_(std::make_unique<T>(std::move(*other))) {}
 
   auto operator=(const IndirectValue& other) -> IndirectValue& {
@@ -64,7 +65,7 @@ class IndirectValue {
     return *this;
   }
 
-  auto operator=(IndirectValue&& other) -> IndirectValue& {
+  auto operator=(IndirectValue&& other) noexcept -> IndirectValue& {
     *value_ = std::move(*other.value_);
     return *this;
   }
@@ -91,7 +92,7 @@ class IndirectValue {
       -> IndirectValue<std::decay_t<decltype(callable())>>;
 
   template <typename... Args>
-  IndirectValue(std::unique_ptr<T> value) : value_(std::move(value)) {}
+  explicit IndirectValue(std::unique_ptr<T> value) : value_(std::move(value)) {}
 
   const std::unique_ptr<T> value_;
 };

common/indirect_value_test.cpp

@@ -26,7 +26,7 @@ TEST(IndirectValueTest, MutableAccess) {
 }
 
 struct NonMovable {
-  NonMovable(int i) : i(i) {}
+  explicit NonMovable(int i) : i(i) {}
   NonMovable(NonMovable&&) = delete;
   auto operator=(NonMovable&&) -> NonMovable& = delete;
 
@@ -39,7 +39,7 @@ TEST(IndirectValueTest, Create) {
   EXPECT_EQ(v->i, 42);
 }
 
-const int& GetIntReference() {
+auto GetIntReference() -> const int& {
   static int i = 42;
   return i;
 }
@@ -54,15 +54,15 @@ TEST(IndirectValueTest, CreateWithDecay) {
 // member function (if any) caused it to reach its present value.
 struct TestValue {
   TestValue() : state("default constructed") {}
-  TestValue(const TestValue& rhs) : state("copy constructed") {}
-  TestValue(TestValue&& other) : state("move constructed") {
+  TestValue(const TestValue& /*rhs*/) : state("copy constructed") {}
+  TestValue(TestValue&& other) noexcept : state("move constructed") {
     other.state = "move constructed from";
   }
-  TestValue& operator=(const TestValue&) {
+  auto operator=(const TestValue&) noexcept -> TestValue& {
     state = "copy assigned";
     return *this;
   }
-  TestValue& operator=(TestValue&& other) {
+  auto operator=(TestValue&& other) noexcept -> TestValue& {
     state = "move assigned";
     other.state = "move assigned from";
     return *this;
@@ -101,6 +101,8 @@ TEST(IndirectValueTest, CopyAssign) {
 TEST(IndirectValueTest, MoveConstruct) {
   IndirectValue<TestValue> v1;
   auto v2 = std::move(v1);
+  // While not entirely safe, the `v1->state` access tests move behavior.
+  // NOLINTNEXTLINE(bugprone-use-after-move)
   EXPECT_EQ(v1->state, "move constructed from");
   EXPECT_EQ(v2->state, "move constructed");
 }
@@ -109,6 +111,8 @@ TEST(IndirectValueTest, MoveAssign) {
   IndirectValue<TestValue> v1;
   IndirectValue<TestValue> v2;
   v2 = std::move(v1);
+  // While not entirely safe, the `v1->state` access tests move behavior.
+  // NOLINTNEXTLINE(bugprone-use-after-move)
   EXPECT_EQ(v1->state, "move assigned from");
   EXPECT_EQ(v2->state, "move assigned");
 }