spirv-reader: SaveName -> Register, add RegisterWithoutId

Change-Id: Iab2eaa3b2d13a3cbb8f6bca01ed79be14dedbfd1
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/49141
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/reader/spirv/namer.cc

@@ -18,6 +18,8 @@
 #include <sstream>
 #include <unordered_set>
 
+#include "src/debug.h"
+
 namespace tint {
 namespace reader {
 namespace spirv {
@@ -120,28 +122,39 @@ std::string Namer::FindUnusedDerivedName(const std::string& base_name) const {
 
 std::string Namer::MakeDerivedName(const std::string& base_name) {
   auto result = FindUnusedDerivedName(base_name);
-  // Register it.
+  const bool registered = RegisterWithoutId(result);
-  name_to_id_[result] = 0;
+  TINT_ASSERT(registered);
   return result;
 }
 
-bool Namer::SaveName(uint32_t id, const std::string& name) {
+bool Namer::Register(uint32_t id, const std::string& name) {
   if (HasName(id)) {
     return Fail() << "internal error: ID " << id
                   << " already has registered name: " << id_to_name_[id];
   }
+  if (!RegisterWithoutId(name)) {
+    return false;
+  }
   id_to_name_[id] = name;
   name_to_id_[name] = id;
   return true;
 }
 
+bool Namer::RegisterWithoutId(const std::string& name) {
+  if (IsRegistered(name)) {
+    return Fail() << "internal error: name already registered: " << name;
+  }
+  name_to_id_[name] = 0;
+  return true;
+}
+
 bool Namer::SuggestSanitizedName(uint32_t id,
                                  const std::string& suggested_name) {
   if (HasName(id)) {
     return false;
   }
 
-  return SaveName(id, FindUnusedDerivedName(Sanitize(suggested_name)));
+  return Register(id, FindUnusedDerivedName(Sanitize(suggested_name)));
 }
 
 bool Namer::SuggestSanitizedMemberName(uint32_t struct_id,

src/reader/spirv/namer.h

@@ -106,7 +106,13 @@ class Namer {
   /// @param id the SPIR-V ID
   /// @param name the name to map to the ID
   /// @returns true if the ID did not have a previously registered name.
-  bool SaveName(uint32_t id, const std::string& name);
+  bool Register(uint32_t id, const std::string& name);
+
+  /// Registers a name, but not associated to any ID. Fails if the name
+  /// was already registered.
+  /// @param name the name to register
+  /// @returns true if the name was not already reegistered.
+  bool RegisterWithoutId(const std::string& name);
 
   /// Saves a sanitized name for the given ID, if that ID does not yet
   /// have a registered name, and if the sanitized name has not already

src/reader/spirv/namer_test.cc

@@ -87,15 +87,15 @@ TEST_F(SpvNamerTest, FindUnusedDerivedName_NoRecordedName) {
 
 TEST_F(SpvNamerTest, FindUnusedDerivedName_HasRecordedName) {
   Namer namer(fail_stream_);
-  namer.SaveName(12, "rigby");
+  namer.Register(12, "rigby");
   EXPECT_THAT(namer.FindUnusedDerivedName("rigby"), Eq("rigby_1"));
 }
 
 TEST_F(SpvNamerTest, FindUnusedDerivedName_HasMultipleConflicts) {
   Namer namer(fail_stream_);
-  namer.SaveName(12, "rigby");
+  namer.Register(12, "rigby");
-  namer.SaveName(13, "rigby_1");
+  namer.Register(13, "rigby_1");
-  namer.SaveName(14, "rigby_3");
+  namer.Register(14, "rigby_3");
   // It picks the first non-conflicting suffix.
   EXPECT_THAT(namer.FindUnusedDerivedName("rigby"), Eq("rigby_2"));
 }
@@ -107,7 +107,7 @@ TEST_F(SpvNamerTest, IsRegistered_NoRecordedName) {
 
 TEST_F(SpvNamerTest, IsRegistered_RegisteredById) {
   Namer namer(fail_stream_);
-  namer.SaveName(1, "abbey");
+  namer.Register(1, "abbey");
   EXPECT_TRUE(namer.IsRegistered("abbey"));
 }
 
@@ -127,25 +127,60 @@ TEST_F(SpvNamerTest, MakeDerivedName_NoRecordedName) {
 
 TEST_F(SpvNamerTest, MakeDerivedName_HasRecordedName) {
   Namer namer(fail_stream_);
-  namer.SaveName(12, "rigby");
+  namer.Register(12, "rigby");
   EXPECT_THAT(namer.MakeDerivedName("rigby"), Eq("rigby_1"));
 }
 
 TEST_F(SpvNamerTest, MakeDerivedName_HasMultipleConflicts) {
   Namer namer(fail_stream_);
-  namer.SaveName(12, "rigby");
+  namer.Register(12, "rigby");
-  namer.SaveName(13, "rigby_1");
+  namer.Register(13, "rigby_1");
-  namer.SaveName(14, "rigby_3");
+  namer.Register(14, "rigby_3");
   // It picks the first non-conflicting suffix.
   EXPECT_THAT(namer.MakeDerivedName("rigby"), Eq("rigby_2"));
 }
 
-TEST_F(SpvNamerTest, SaveNameOnce) {
+TEST_F(SpvNamerTest, RegisterWithoutId_Once) {
+  Namer namer(fail_stream_);
+
+  const std::string n("abbey");
+  EXPECT_FALSE(namer.IsRegistered(n));
+  EXPECT_TRUE(namer.RegisterWithoutId(n));
+  EXPECT_TRUE(namer.IsRegistered(n));
+  EXPECT_TRUE(success_);
+  EXPECT_TRUE(error().empty());
+}
+
+TEST_F(SpvNamerTest, RegisterWithoutId_Twice) {
+  Namer namer(fail_stream_);
+
+  const std::string n("abbey");
+  EXPECT_FALSE(namer.IsRegistered(n));
+  EXPECT_TRUE(namer.RegisterWithoutId(n));
+  // Fails on second attempt.
+  EXPECT_FALSE(namer.RegisterWithoutId(n));
+  EXPECT_FALSE(success_);
+  EXPECT_EQ(error(),"internal error: name already registered: abbey");
+}
+
+TEST_F(SpvNamerTest, RegisterWithoutId_ConflictsWithIdRegisteredName) {
+  Namer namer(fail_stream_);
+
+  const std::string n("abbey");
+  EXPECT_TRUE(namer.Register(1,n));
+  EXPECT_TRUE(namer.IsRegistered(n));
+  // Fails on attempt to register without ID.
+  EXPECT_FALSE(namer.RegisterWithoutId(n));
+  EXPECT_FALSE(success_);
+  EXPECT_EQ(error(),"internal error: name already registered: abbey");
+}
+
+TEST_F(SpvNamerTest, Register_Once) {
   Namer namer(fail_stream_);
 
   const uint32_t id = 9;
   EXPECT_FALSE(namer.HasName(id));
-  const bool save_result = namer.SaveName(id, "abbey road");
+  const bool save_result = namer.Register(id, "abbey road");
   EXPECT_TRUE(save_result);
   EXPECT_TRUE(namer.HasName(id));
   EXPECT_EQ(namer.GetName(id), "abbey road");
@@ -153,13 +188,13 @@ TEST_F(SpvNamerTest, SaveNameOnce) {
   EXPECT_TRUE(error().empty());
 }
 
-TEST_F(SpvNamerTest, SaveNameTwoIds) {
+TEST_F(SpvNamerTest, Register_TwoIds) {
   Namer namer(fail_stream_);
 
   EXPECT_FALSE(namer.HasName(8));
   EXPECT_FALSE(namer.HasName(9));
-  EXPECT_TRUE(namer.SaveName(8, "abbey road"));
+  EXPECT_TRUE(namer.Register(8, "abbey road"));
-  EXPECT_TRUE(namer.SaveName(9, "rubber soul"));
+  EXPECT_TRUE(namer.Register(9, "rubber soul"));
   EXPECT_TRUE(namer.HasName(8));
   EXPECT_TRUE(namer.HasName(9));
   EXPECT_EQ(namer.GetName(9), "rubber soul");
@@ -168,12 +203,12 @@ TEST_F(SpvNamerTest, SaveNameTwoIds) {
   EXPECT_TRUE(error().empty());
 }
 
-TEST_F(SpvNamerTest, SaveNameFailsDueToIdReuse) {
+TEST_F(SpvNamerTest, Register_FailsDueToIdReuse) {
   Namer namer(fail_stream_);
 
   const uint32_t id = 9;
-  EXPECT_TRUE(namer.SaveName(id, "abbey road"));
+  EXPECT_TRUE(namer.Register(id, "abbey road"));
-  EXPECT_FALSE(namer.SaveName(id, "rubber soul"));
+  EXPECT_FALSE(namer.Register(id, "rubber soul"));
   EXPECT_TRUE(namer.HasName(id));
   EXPECT_EQ(namer.GetName(id), "abbey road");
   EXPECT_FALSE(success_);
@@ -191,7 +226,7 @@ TEST_F(SpvNamerTest,
        SuggestSanitizedName_RejectSuggestionWhenConflictOnSameId) {
   Namer namer(fail_stream_);
 
-  namer.SaveName(1, "lennon");
+  namer.Register(1, "lennon");
   EXPECT_FALSE(namer.SuggestSanitizedName(1, "mccartney"));
   EXPECT_THAT(namer.GetName(1), Eq("lennon"));
 }
@@ -207,7 +242,7 @@ TEST_F(SpvNamerTest,
        SuggestSanitizedName_GenerateNewNameWhenConflictOnDifferentId) {
   Namer namer(fail_stream_);
 
-  namer.SaveName(7, "rice");
+  namer.Register(7, "rice");
   EXPECT_TRUE(namer.SuggestSanitizedName(9, "rice"));
   EXPECT_THAT(namer.GetName(9), Eq("rice_1"));
 }
@@ -260,13 +295,13 @@ TEST_F(SpvNamerTest, Name_GeneratesNameIfNoneRegistered) {
 
 TEST_F(SpvNamerTest, Name_GeneratesNameWithoutConflict) {
   Namer namer(fail_stream_);
-  namer.SaveName(42, "x_14");
+  namer.Register(42, "x_14");
   EXPECT_THAT(namer.Name(14), Eq("x_14_1"));
 }
 
 TEST_F(SpvNamerTest, Name_ReturnsRegisteredName) {
   Namer namer(fail_stream_);
-  namer.SaveName(14, "hello");
+  namer.Register(14, "hello");
   EXPECT_THAT(namer.Name(14), Eq("hello"));
 }