cleanup: use same clang-tidy in all repositories (#206)

Use the clang-tidy distributed with Fedora (currently clang-tidy-9) in
this repository. I am planning to make similar changes in the other
repositories and bring all our `clang-tidy` builds to use the same
versions of `clang-format`, `clang-tidy`, `doxygen`, `buildifier`,
`shellcheck`, and `cmake_format`.

I also updated the clang-tidy configuration to match the newer repos
(`-pubsub` and `-spanner`), and cleaned up any errors found in the
files.

Finally, fixed a number of Doxygen comments that used `<U>` (for a
template parameter) which Doxygen thought was an unordered list or
something.

Author: coryan

.clang-tidy

@@ -1,9 +1,30 @@
 ---
 # Configure clang-tidy for this project.
 
-# Disabled:
-#  -google-readability-namespace-comments the *_CLIENT_NS is a macro, and
-#   clang-tidy fails to match it against the initial value.
+# Here is an explanation for why some of the checks are disabled:
+#
+#  -google-readability-namespace-comments: the *_CLIENT_NS is a macro, and
+#      clang-tidy fails to match it against the initial value.
+#
+#  -modernize-use-trailing-return-type: clang-tidy recommends using
+#      `auto Foo() -> std::string { return ...; }`, we think the code is less
+#      readable in this form.
+#
+#  -modernize-return-braced-init-list: We think removing typenames and using
+#      only braced-init can hurt readability.
+#
+#  -performance-move-const-arg: This warning requires the developer to
+#      know/care more about the implementation details of types/functions than
+#      should be necessary. For example, `A a; F(std::move(a));` will trigger a
+#      warning IFF `A` is a trivial type (and therefore the move is
+#      meaningless). It would also warn if `F` accepts by `const&`, which is
+#      another detail that the caller need not care about.
+#
+#  -readability-redundant-declaration: A friend declaration inside a class
+#      counts as a declaration, so if we also declare that friend outside the
+#      class in order to document it as part of the public API, that will
+#      trigger a redundant declaration warning from this check.
+#
 Checks: >
   -*,
   bugprone-*,
@@ -13,24 +34,32 @@ Checks: >
   performance-*,
   portability-*,
   readability-*,
+  -google-readability-braces-around-statements,
   -google-readability-namespace-comments,
-  -google-runtime-int,
   -google-runtime-references,
   -misc-non-private-member-variables-in-classes,
+  -modernize-return-braced-init-list,
+  -performance-move-const-arg,
   -readability-named-parameter,
+  -modernize-use-trailing-return-type,
   -readability-braces-around-statements,
-  -readability-magic-numbers
+  -readability-redundant-declaration
 
 # Turn all the warnings from the checks above into errors.
 WarningsAsErrors: "*"
 
+# TODO(#205) - Enable clang-tidy checks in our headers.
+#    HeaderFilterRegex: "google/cloud/.*"
+
 CheckOptions:
   - { key: readability-identifier-naming.NamespaceCase,          value: lower_case }
   - { key: readability-identifier-naming.ClassCase,              value: CamelCase  }
   - { key: readability-identifier-naming.StructCase,             value: CamelCase  }
   - { key: readability-identifier-naming.TemplateParameterCase,  value: CamelCase  }
-  - { key: readability-identifier-naming.FunctionCase,           value: CamelCase  }
+  - { key: readability-identifier-naming.FunctionCase,           value: aNy_CasE  }
   - { key: readability-identifier-naming.VariableCase,           value: lower_case }
+  - { key: readability-identifier-naming.ClassMemberCase,        value: lower_case }
+  - { key: readability-identifier-naming.ClassMemberSuffix,      value: _          }
   - { key: readability-identifier-naming.PrivateMemberSuffix,    value: _          }
   - { key: readability-identifier-naming.ProtectedMemberSuffix,  value: _          }
   - { key: readability-identifier-naming.MacroDefinitionCase,    value: UPPER_CASE }

ci/kokoro/docker/Dockerfile.fedora-install

@@ -12,62 +12,59 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-ARG DISTRO_VERSION=30
+ARG DISTRO_VERSION=31
 FROM fedora:${DISTRO_VERSION}
 ARG NCPU=4
 
-RUN dnf makecache && dnf install -y \
-    autoconf \
-    automake \
-    c-ares-devel \
-    ccache \
-    clang \
-    clang-analyzer \
-    clang-tools-extra \
-    cmake \
-    curl \
-    doxygen \
-    gcc-c++ \
-    git \
-    grpc-devel \
-    grpc-plugins \
-    libcxx-devel \
-    libcxxabi-devel \
-    libtool \
-    make \
-    ncurses-term \
-    openssl-devel \
-    pkgconfig \
-    protobuf-compiler \
-    python3 \
-    shtool \
-    unzip \
-    wget \
-    which \
-    zlib-devel
+# Fedora includes packages for gRPC, libcurl, and OpenSSL that are recent enough
+# for `google-cloud-cpp`. Install these packages and additional development
+# tools to compile the dependencies:
+RUN dnf makecache && \
+    dnf install -y abi-compliance-checker abi-dumper \
+        clang clang-tools-extra cmake diffutils doxygen findutils gcc-c++ git \
+        grpc-devel grpc-plugins libcxx-devel libcxxabi-devel libcurl-devel \
+        make openssl-devel pkgconfig protobuf-compiler python3 python3-pip \
+        ShellCheck tar unzip w3m wget which zlib-devel
 
-# Install googleapis.
-WORKDIR /var/tmp/build
-RUN wget -q https://github.com/googleapis/cpp-cmakefiles/archive/v0.4.1.tar.gz
-RUN tar -xf v0.4.1.tar.gz
-WORKDIR /var/tmp/build/cpp-cmakefiles-0.4.1
-RUN cmake \
-    -DBUILD_SHARED_LIBS=YES \
-    -H. -Bcmake-out
-RUN cmake --build cmake-out --target install -- -j ${NCPU}
-RUN ldconfig
+# Install the the buildifier tool, which does not compile with the default
+# golang compiler for Ubuntu 16.04 and Ubuntu 18.04.
+RUN wget -q -O /usr/bin/buildifier https://github.com/bazelbuild/buildtools/releases/download/0.29.0/buildifier
+RUN chmod 755 /usr/bin/buildifier
 
-# Install googletest.
+# Install cmake_format to automatically format the CMake list files.
+#     https://github.com/cheshirekow/cmake_format
+# Pin this to an specific version because the formatting changes when the
+# "latest" version is updated, and we do not want the builds to break just
+# because some third party changed something.
+RUN pip3 install --upgrade pip
+RUN pip3 install cmake_format==0.6.8
+
+# Install googletest, remove the downloaded files and the temporary artifacts
+# after a successful build to keep the image smaller (and with fewer layers)
 WORKDIR /var/tmp/build
-RUN wget -q https://github.com/google/googletest/archive/release-1.10.0.tar.gz
-RUN tar -xf release-1.10.0.tar.gz
-WORKDIR /var/tmp/build/googletest-release-1.10.0
-RUN cmake \
+RUN wget -q https://github.com/google/googletest/archive/release-1.10.0.tar.gz && \
+    tar -xf release-1.10.0.tar.gz && \
+    cd /var/tmp/build/googletest-release-1.10.0 && \
+    cmake \
       -DCMAKE_BUILD_TYPE="Release" \
       -DBUILD_SHARED_LIBS=yes \
-      -H. -Bcmake-out/googletest
-RUN cmake --build cmake-out/googletest --target install -- -j ${NCPU}
-RUN ldconfig
+      -H. -Bcmake-out/googletest && \
+    cmake --build cmake-out/googletest --target install -- -j ${NCPU} && \
+    ldconfig && \
+    cd /var/tmp && rm -fr build
+
+# Install googletest, remove the downloaded files and the temporary artifacts
+# after a successful build to keep the image smaller (and with fewer layers)
+WORKDIR /var/tmp/build
+RUN wget -q https://github.com/googleapis/cpp-cmakefiles/archive/v0.5.0.tar.gz && \
+    tar -xf v0.5.0.tar.gz && \
+    cd /var/tmp/build/cpp-cmakefiles-0.5.0 && \
+    cmake \
+      -DBUILD_SHARED_LIBS=YES \
+      -H. -Bcmake-out && \
+    cmake --build cmake-out --target install -- -j ${NCPU} && \
+    ldconfig && \
+    cd /var/tmp && rm -fr build
 
 # Install Bazel because some of the builds need it.
 WORKDIR /var/tmp/ci

ci/kokoro/docker/Dockerfile.ubuntu-install

@@ -51,28 +51,6 @@ RUN apt-get update && \
         wget \
         zlib1g-dev
 
-# By default, Ubuntu 18.04 does not install the alternatives for clang-format
-# and clang-tidy, so we need to manually install those.
-RUN if grep -q 18.04 /etc/lsb-release; then \
-      apt-get update && apt-get --no-install-recommends install -y clang-tidy clang-format-7 clang-tools; \
-      update-alternatives --install /usr/bin/clang-tidy clang-tidy /usr/bin/clang-tidy-6.0 100; \
-      update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-7 100; \
-      update-alternatives --install /usr/bin/scan-build scan-build /usr/bin/scan-build-6.0 100; \
-    fi
-
-# Install the the buildifier tool, which does not compile with the default
-# golang compiler for Ubuntu 16.04 and Ubuntu 18.04.
-RUN wget -q -O /usr/bin/buildifier https://github.com/bazelbuild/buildtools/releases/download/0.29.0/buildifier
-RUN chmod 755 /usr/bin/buildifier
-
-# Install cmake_format to automatically format the CMake list files.
-#     https://github.com/cheshirekow/cmake_format
-# Pin this to an specific version because the formatting changes when the
-# "latest" version is updated, and we do not want the builds to break just
-# because some third party changed something.
-RUN pip3 install --upgrade pip
-RUN pip3 install setuptools cmake_format==0.6.8
-
 # Install protobuf using CMake. Some distributions include protobuf, but gRPC
 # requires 3.4.x or newer, and many of those distribution use older versions.
 # We need to install both the debug and Release version because:

ci/kokoro/docker/build-in-docker-cmake.sh

@@ -55,15 +55,15 @@ cmake_extra_flags=(
     # Always set the build type, so it can be configured by the `build.sh`
     # script.
     "-DCMAKE_BUILD_TYPE=${BUILD_TYPE}"
-
-    # Always disable the ccache, it can make some builds flaky, and we do not
-    # preserve the cache between builds.
-    "-DGOOGLE_CLOUD_CPP_ENABLE_CCACHE=OFF"
 )
 if [[ "${BUILD_TESTING:-}" == "no" ]]; then
   cmake_extra_flags+=( "-DBUILD_TESTING=OFF" )
 fi
 
+if [[ "${CLANG_TIDY:-}" = "yes" ]]; then
+  cmake_extra_flags+=("-DGOOGLE_CLOUD_CPP_CLANG_TIDY=yes")
+fi
+
 if [[ "${GOOGLE_CLOUD_CPP_CXX_STANDARD:-}" != "" ]]; then
   cmake_extra_flags+=(
     "-DGOOGLE_CLOUD_CPP_CXX_STANDARD=${GOOGLE_CLOUD_CPP_CXX_STANDARD}")

ci/kokoro/docker/build.sh

@@ -57,12 +57,16 @@ fi
 if [[ "${BUILD_NAME}" = "clang-tidy" ]]; then
   # Compile with clang-tidy(1) turned on. The build treats clang-tidy warnings
   # as errors.
-  export BUILD_TYPE=Debug
+  export DISTRO=fedora-install
+  export DISTRO_VERSION=31
   export CC=clang
   export CXX=clang++
-  export CMAKE_FLAGS="-DGOOGLE_CLOUD_CPP_CLANG_TIDY=yes"
+  export BUILD_TYPE=Debug
   export CHECK_STYLE=yes
   export GENERATE_DOCS=yes
+  export CLANG_TIDY=yes
+  export TEST_INSTALL=yes
+  in_docker_script="ci/kokoro/docker/build-in-docker-cmake.sh"
 elif [[ "${BUILD_NAME}" = "publish-refdocs" ]]; then
   export BUILD_TYPE=Debug
   export CC=clang
@@ -132,7 +136,6 @@ elif [[ "${BUILD_NAME}" = "no-tests" ]]; then
   # package maintainers, where the cost of running the tests for a fixed version
   # is too high.
   export BUILD_TESTING=no
-  export CHECK_STYLE=yes
 elif [[ "${BUILD_NAME}" = "libcxx" ]]; then
   # Compile using libc++. This is easier to install on Fedora.
   export CC=clang
@@ -374,6 +377,10 @@ docker_flags=(
     # clang-format, cmake-format, and buildifier).
     "--env" "CHECK_STYLE=${CHECK_STYLE:-}"
 
+    # If set to 'yes', the build script will configure clang-tidy. Currently
+    # only the CMake builds use this flag.
+    "--env" "CLANG_TIDY=${CLANG_TIDY:-}"
+
     # If set to 'no', skip the integration tests.
     "--env" "RUN_INTEGRATION_TESTS=${RUN_INTEGRATION_TESTS:-}"
 

google/cloud/future_generic.h

@@ -90,14 +90,14 @@ class future final : private internal::future_base<T> {
    * ready.  The return type is a future wrapping the return type of
    * @a func.
    *
-   * @return future<T> where T is std::result_of_t<F, R> (basically).
-   * If T matches future<U> then it returns future<U>.  The returned
+   * @return `future<T>` where T is `std::result_of_t<F, R>` (basically).
+   * If T matches `future<U>` then it returns `future<U>`.  The returned
    * future will contain the result of @a func.
    * @param func a Callable to be invoked when the future is ready.
    * The function might be called immediately, e.g., if the future is
    * ready.
    *
-   * Side effects: valid() == false if the operation is successful.
+   * Side effects: `valid() == false` if the operation is successful.
    */
   template <typename F>
   typename internal::then_helper<F, T>::future_t then(F&& func) {

google/cloud/future_generic_test.cc

@@ -98,11 +98,12 @@ TEST(FutureTestInt, conform_30_6_5_7) {
   EXPECT_TRUE(f0.valid());
   ASSERT_EQ(std::future_status::ready, f0.wait_for(0_ms));
 #if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_THROW(try { f0.get(); } catch (std::future_error const& ex) {
-    EXPECT_EQ(std::future_errc::broken_promise, ex.code());
-    throw;
-  },
-               std::future_error);
+  EXPECT_THROW(
+      try { f0.get(); } catch (std::future_error const& ex) {
+        EXPECT_EQ(std::future_errc::broken_promise, ex.code());
+        throw;
+      },
+      std::future_error);
 #else
   EXPECT_DEATH_IF_SUPPORTED(
       f0.get(),
@@ -201,11 +202,12 @@ TEST(FutureTestInt, conform_30_6_5_18) {
   p0.set_exception(std::make_exception_ptr(std::runtime_error("testing")));
   ASSERT_EQ(std::future_status::ready, f0.wait_for(0_ms));
 #if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_THROW(try { f0.get(); } catch (std::runtime_error const& ex) {
-    EXPECT_EQ(std::string("testing"), ex.what());
-    throw;
-  },
-               std::runtime_error);
+  EXPECT_THROW(
+      try { f0.get(); } catch (std::runtime_error const& ex) {
+        EXPECT_EQ(std::string("testing"), ex.what());
+        throw;
+      },
+      std::runtime_error);
 #else
   EXPECT_DEATH_IF_SUPPORTED(
       f0.get(),
@@ -430,11 +432,12 @@ TEST(FutureTestInt, conform_30_6_6_17) {
   future<int> f = p.get_future();
   p.set_exception(std::make_exception_ptr(std::runtime_error("test message")));
 #if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_THROW(try { f.get(); } catch (std::runtime_error const& ex) {
-    EXPECT_THAT(ex.what(), HasSubstr("test message"));
-    throw;
-  },
-               std::runtime_error);
+  EXPECT_THROW(
+      try { f.get(); } catch (std::runtime_error const& ex) {
+        EXPECT_THAT(ex.what(), HasSubstr("test message"));
+        throw;
+      },
+      std::runtime_error);
 #else
   EXPECT_DEATH_IF_SUPPORTED(
       f.get(),