retest validator changes Pr/39897

.circleci/install_validator_dependencies.sh

@@ -13,7 +13,7 @@ sudo mv bazel.gpg /etc/apt/trusted.gpg.d/
 echo "deb [arch=amd64] https://storage.googleapis.com/bazel-apt stable jdk1.8" | sudo tee /etc/apt/sources.list.d/bazel.list
 
 echo $(GREEN "Updating and installing apt packages...")
-sudo apt update && sudo apt install bazel-5.4.0 clang python3 python3-pip protobuf-compiler
+sudo apt update && sudo apt install bazel-7.1.0 clang python3 python3-pip protobuf-compiler
 
 echo $(GREEN "Installing protobuf python module...")
 pip3 install protobuf==3.19.4

build-system/tasks/validator.js

@@ -25,7 +25,7 @@ async function validator() {
  */
 async function validatorCpp() {
   const bazelCmd = [
-    'bazel-5.4.0 test',
+    'bazel-7.1.0 test',
     '--repo_env=CC=clang',
     "--cxxopt='-std=c++17'",
     '--discard_analysis_cache',

validator/cpp/engine/BUILD

@@ -1,6 +1,7 @@
 load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test")
 load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
 load("embed_data.bzl", "embed_data")
+load("//tools/build_defs/license:license.bzl", "license")
 
 # Requirements:
 # clang with c++17 support.
@@ -14,7 +15,15 @@ load("embed_data.bzl", "embed_data")
 #
 # bazel test --cxxopt='-std=c++17' validator_test
 
-package(default_visibility = ["//visibility:public"])
+package(
+    default_applicable_licenses = [":license"],
+    default_visibility = ["//visibility:public"],
+)
+
+license(
+    name = "license",
+    package_name = "ampvalidator",
+)
 
 licenses(["notice"])
 

validator/cpp/engine/validator-internal.cc

@@ -595,7 +595,7 @@ class ParsedAttrSpec {
     return value_property_by_name_;
   }
 
-  const unordered_map<std::string, const CssDeclaration*>&
+  const absl::flat_hash_map<std::string, const CssDeclaration*>&
   css_declaration_by_name() const {
     return css_declaration_by_name_;
   }
@@ -620,7 +620,8 @@ class ParsedAttrSpec {
   // Name lookup for spec().value_properties().properties().
   unordered_map<std::string, const PropertySpec*> value_property_by_name_;
   // Name lookup for spec().css_declaration().
-  unordered_map<std::string, const CssDeclaration*> css_declaration_by_name_;
+  absl::flat_hash_map<std::string, const CssDeclaration*>
+      css_declaration_by_name_;
   // The mandatory spec().value_properties().properties().
   vector<const PropertySpec*> mandatory_value_properties_;
   vector<TypeIdentifier> disabled_by_;
@@ -4323,7 +4324,7 @@ void ValidateAttrDeclaration(const ParsedAttrSpec& parsed_attr_spec,
   // If there were errors parsing, exit from validating further.
   if (!css_errors.empty()) return;
 
-  const unordered_map<std::string, const CssDeclaration*>&
+  const absl::flat_hash_map<std::string, const CssDeclaration*>&
       css_declaration_by_name = parsed_attr_spec.css_declaration_by_name();
 
   for (auto& declaration : declarations) {

validator/cpp/engine/validator_test.cc

@@ -1220,7 +1220,8 @@ TEST(ValidatorTest, RulesMakeSense) {
                         HtmlFormat::UNKNOWN_CODE),
               html_format.cend())
         << "tagSpec.htmlFormat should never contain UNKNOWN_CODE"
-        << ":\n" << tag_spec.DebugString();
+        << ":\n"
+        << tag_spec;
 
     EXPECT_TRUE(tag_spec.has_tag_name());
     EXPECT_TRUE(RE2::PartialMatch(tag_spec.tag_name(), tag_name_regex));
@@ -1562,12 +1563,12 @@ TEST(ValidatorTest, RulesMakeSense) {
     }
 
     for (const auto& attr_spec : tag_spec.attrs()) {
-      EXPECT_TRUE(attr_spec.has_name()) << attr_spec.DebugString();
+      EXPECT_TRUE(attr_spec.has_name()) << attr_spec;
       // Attribute Spec names are matched against lowercased attributes,
       // so the rules *must* also be lower case or non-cased.
       EXPECT_TRUE(RE2::FullMatch(attr_spec.name(), RE2("[^A-Z]+")))
-          << attr_spec.DebugString();
-      EXPECT_NE(attr_spec.name(), "[style]") << attr_spec.DebugString();
+          << attr_spec;
+      EXPECT_NE(attr_spec.name(), "[style]") << attr_spec;
       if (attr_spec.has_value_url()) {
         for (const std::string& protocol : attr_spec.value_url().protocol()) {
           // UrlSpec protocol is matched against lowercased protocol names,
@@ -1582,27 +1583,24 @@ TEST(ValidatorTest, RulesMakeSense) {
             if (protocol == "http" &&
                 attr_spec.value_url().has_allow_relative()) {
               EXPECT_TRUE(attr_spec.value_url().allow_relative())
-                  << attr_spec.value_url().DebugString();
+                  << attr_spec.value_url();
             }
           }
         }
       }
       if (attr_spec.has_value_regex()) {
-        EXPECT_TRUE(RE2(attr_spec.value_regex()).ok())
-            << attr_spec.DebugString();
+        EXPECT_TRUE(RE2(attr_spec.value_regex()).ok()) << attr_spec;
       }
       if (attr_spec.has_value_regex_casei()) {
-        EXPECT_TRUE(RE2(attr_spec.value_regex_casei()).ok())
-            << attr_spec.DebugString();
+        EXPECT_TRUE(RE2(attr_spec.value_regex_casei()).ok()) << attr_spec;
       }
       if (attr_spec.has_disallowed_value_regex()) {
-        EXPECT_TRUE(RE2(attr_spec.disallowed_value_regex()).ok())
-            << attr_spec.DebugString();
+        EXPECT_TRUE(RE2(attr_spec.disallowed_value_regex()).ok()) << attr_spec;
       }
       if (attr_spec.has_value_url()) {
         EXPECT_GT(attr_spec.value_url().protocol().size(), 0)
             << "value_url must have at least one protocol\n"
-            << attr_spec.DebugString();
+            << attr_spec;
       }
       int num_values = 0;
       if (!attr_spec.value().empty()) {
@@ -1627,12 +1625,12 @@ TEST(ValidatorTest, RulesMakeSense) {
       if (attr_spec.name() == "id" && num_values == 0) {
         EXPECT_TRUE(attr_spec.has_disallowed_value_regex())
             << "'id' attribute must have 'disallowed_value_regex' set\n"
-            << attr_spec.DebugString();
+            << attr_spec;
       }
       if (attr_spec.name() == "name" && num_values == 0) {
         EXPECT_TRUE(attr_spec.has_disallowed_value_regex())
             << "'name' attribute must have 'disallowed_value_regex' set\n"
-            << attr_spec.DebugString();
+            << attr_spec;
       }
       if (attr_spec.has_deprecation()) {
         EXPECT_TRUE(attr_spec.has_deprecation_url());

validator/cpp/htmlparser/BUILD

@@ -195,10 +195,10 @@ cc_library(
     hdrs = [
         "logging.h",
     ],
+    copts = ["-std=c++17"],
     deps = [
         ":glog_polyfill",
     ],
-    copts = ["-std=c++17"],
 )
 
 # Defines token type and token structures, used during tokenization.
@@ -551,7 +551,7 @@ cc_test(
         ":strings",
         ":testconstants",
         ":tokenizer",
-        "@com_google_googletest//:gtest_main",
         "@com_google_absl//absl/flags:flag",
+        "@com_google_googletest//:gtest_main",
     ],
 )

validator/cpp/htmlparser/allocator.h

@@ -138,7 +138,7 @@
 // IMPORTANT: Tree like structure must not destroy the child nodes or sibling
 // nodes. Allocator destroys all the objects and call its destructor, it is an
 // error to invoke destructors on objects allocated by this allocator. Allocator
-// is the master owner of all the objects. Client treats all objects as const
+// is the primary owner of all the objects. Client treats all objects as const
 // pointer as far as destruction goes.
 //
 // It is not possible to destroy random objects or free up the slots to be
@@ -174,6 +174,7 @@
 #include <cstring>
 #include <memory>
 #include <tuple>
+#include <type_traits>
 #include <vector>
 
 namespace htmlparser {

validator/cpp/htmlparser/allocator_test.cc

@@ -1,5 +1,10 @@
 #include "cpp/htmlparser/allocator.h"
 
+#include <array>
+#include <cstdint>
+#include <string>
+#include <vector>
+
 #include "gtest/gtest.h"
 
 // Memory leaks will automatically be detected by the test framework.
@@ -12,9 +17,9 @@ TEST(AllocatorTest, BasicTest) {
     int32_t a;
     int64_t b;
     std::string c;
-    short d;
+    uint16_t d;
     Data() {}
-    Data(int32_t a_, int64_t b_, std::string c_, short d_)
+    Data(int32_t a_, int64_t b_, std::string c_, uint16_t d_)
         : a(a_), b(b_), c(c_), d(d_) {}
   };
 
@@ -146,7 +151,7 @@ TEST(AllocatorTest, DestructorCalled) {
 
 TEST(AllocatorTest, BitFields) {
   struct HasBitFields {
-    short s;
+    uint16_t s;
     char c;
     int flip : 1;
     int nybble : 4;

validator/cpp/htmlparser/atomutil.cc

@@ -1,5 +1,8 @@
 #include "cpp/htmlparser/atomutil.h"
 
+#include <string>
+#include <string_view>
+
 #include "cpp/htmlparser/hash.h"
 
 namespace htmlparser {
@@ -20,7 +23,7 @@ Atom AtomUtil::ToAtom(const std::string& s) {
   table_index = (hash >> 16) & (kNamesHashTable.size() - 1);
   atom_value = kNamesHashTable[table_index];
   atom_len = atom_value & 0xff;
-  if (atom_len == s.size() && ToString(atom_value).compare(s) == 0) {
+  if (atom_len == s.size() && ToString(atom_value) == s) {
     return CastToAtom(atom_value);
   }
 

validator/cpp/htmlparser/bin/atomgen.cc

@@ -13,6 +13,7 @@
 #include <memory>
 #include <sstream>
 #include <string>
+#include <string_view>
 #include <utility>
 #include <vector>
 
@@ -55,12 +56,12 @@ class TableBuilder {
 
   bool Insert(const std::string& s) {
     std::pair<uint32_t, uint32_t> hashes = Hash(s);
-    if (table_[hashes.first] == "") {
+    if (table_[hashes.first].empty()) {
       table_[hashes.first] = s;
       return true;
     }
 
-    if (table_[hashes.second] == "") {
+    if (table_[hashes.second].empty()) {
       table_[hashes.second] = s;
       return true;
     }
@@ -100,7 +101,7 @@ class TableBuilder {
     std::pair<uint32_t, uint32_t> hashes = Hash(entry);
     uint32_t new_location = hashes.first + hashes.second - i;
 
-    if (table_[new_location] != "" && !Push(new_location, depth + 1)) {
+    if (!table_[new_location].empty() && !Push(new_location, depth + 1)) {
       return false;
     }
 
@@ -190,17 +191,16 @@ int main(int argc, char** argv) {
   // Find hash that minimizes table size.
   std::unique_ptr<TableBuilder> table{nullptr};
   for (int i = 0; i < 1; i++) {
-    if (table.get() != nullptr
-        && (1 << (table->hash_num_bits() - 1)) < all_names.size()) {
+    if (table != nullptr &&
+        (1 << (table->hash_num_bits() - 1)) < all_names.size()) {
       break;
     }
 
     srand(time(nullptr));
     uint32_t rand_state;
     uint32_t hash0 = rand_r(&rand_state) % 1000000000;
     for (uint32_t k = 0; k <= 16; k++) {
-      if (table.get() != nullptr && k >= table->hash_num_bits())
-        break;
+      if (table != nullptr && k >= table->hash_num_bits()) break;
       std::unique_ptr<TableBuilder> base(new TableBuilder());
       if (base->Init(hash0, k, all_names)) {
         table.reset(base.release());
@@ -209,7 +209,7 @@ int main(int argc, char** argv) {
     }
   }
 
-  if (table.get() == nullptr) {
+  if (table == nullptr) {
     std::cerr << "Failed to construct string table." << std::endl;
     std::cerr <<  all_names.size() << ": elements." << std::endl;
     return EXIT_FAILURE;
@@ -224,11 +224,11 @@ int main(int argc, char** argv) {
   while (changed) {
     changed = false;
     for (std::size_t i = 0; i < layout.size(); i++) {
-      if (layout[i] == "") continue;
+      if (layout[i].empty()) continue;
 
       for (std::size_t j = 0; j < layout.size(); j++) {
-        if (i != j && layout[j] != ""
-            && layout[i].find(layout[j]) != std::string::npos) {
+        if (i != j && !layout[j].empty() &&
+            layout[i].find(layout[j]) != std::string::npos) {
           changed = true;
           layout[j] = "";
         }
@@ -244,7 +244,7 @@ int main(int argc, char** argv) {
     int bestj = -1;
     int bestk = 0;
     for (std::size_t i = 0; i < layout.size(); i++) {
-      if (layout[i] == "") continue;
+      if (layout[i].empty()) continue;
 
       for (std::size_t j = 0; j < layout.size(); j++) {
         if (i == j) continue;

validator/cpp/htmlparser/bin/casetablegen.cc

@@ -1,7 +1,9 @@
 #include <fstream>
 #include <iostream>
 #include <sstream>
+#include <string>
 #include <utility>
+#include <vector>
 
 #include "cpp/htmlparser/defer.h"
 #include "cpp/htmlparser/fileutil.h"

validator/cpp/htmlparser/bin/entitytablegen.cc

@@ -6,6 +6,7 @@
 #include <fstream>
 #include <iostream>
 #include <sstream>
+#include <string>
 #include <utility>
 #include <vector>
 

validator/cpp/htmlparser/bin/validatorgen.cc

@@ -15,6 +15,7 @@
 //
 
 #include <iostream>
+#include <string>
 
 #include "glog/logging.h"
 #include "absl/flags/flag.h"

validator/cpp/htmlparser/casetable_test.cc

@@ -1,5 +1,7 @@
 #include "cpp/htmlparser/casetable.h"
 
+#include <string_view>
+
 #include "gtest/gtest.h"
 #include "cpp/htmlparser/strings.h"
 

validator/cpp/htmlparser/css/amp4ads-parse-css.cc

@@ -1,13 +1,14 @@
 #include "cpp/htmlparser/css/amp4ads-parse-css.h"
 
-#include "absl/memory/memory.h"
+#include <memory>
+#include <string>
+#include <vector>
+
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_join.h"
 #include "cpp/htmlparser/css/parse-css.h"
 #include "re2/re2.h"
 
-using absl::make_unique;
-using std::string_view;
 using amp::validator::ValidationError;
 using std::unique_ptr;
 
@@ -18,7 +19,7 @@ namespace {
 unique_ptr<ErrorToken> CreateParseErrorTokenAt(
     const Token& position_token, ValidationError::Code code,
     const std::vector<std::string>& params) {
-  auto token = make_unique<ErrorToken>(code, params);
+  auto token = std::make_unique<ErrorToken>(code, params);
   position_token.CopyStartPositionTo(token.get());
   return token;
 }

validator/cpp/htmlparser/css/parse-css-urls.cc

@@ -1,6 +1,8 @@
 #include "cpp/htmlparser/css/parse-css-urls.h"
 
 #include <memory>
+#include <string>
+#include <utility>
 
 #include "absl/algorithm/container.h"
 #include "absl/memory/memory.h"