Handle operands when OpCode is !<integer>.

diff --git a/source/text.cpp b/source/text.cpp
index 6c44008..5912558 100644 (file)
--- a/source/text.cpp
+++ b/source/text.cpp
@@ -389,13 +389,17 @@ spv_result_t spvTextEncodeOperand(
       if (spvTextIsNamedId(textValue)) {
         id = spvNamedIdAssignOrGet(namedIdTable, textValue, pBound);
       } else {
-        spvCheck(spvTextToUInt32(textValue, &id),
-                 DIAGNOSTIC
-                     << "Invalid "
-                     << ((type == SPV_OPERAND_TYPE_RESULT_ID) ? "result " : "")
-                     << "ID '" << textValue << "'.";
-                 return (spvOperandIsOptional(type) ? SPV_FAILED_MATCH
-                                                    : SPV_ERROR_INVALID_TEXT));
+        if (spvTextToUInt32(textValue, &id) != SPV_SUCCESS) {
+          if (spvOperandIsOptional(type)) {
+            return SPV_FAILED_MATCH;
+          } else {
+            DIAGNOSTIC << "Invalid "
+                       << ((type == SPV_OPERAND_TYPE_RESULT_ID) ? "result "
+                                                                : "")
+                       << "ID '" << textValue << "'.";
+            return SPV_ERROR_INVALID_TEXT;
+          }
+        }
       }
       pInst->words[pInst->wordCount++] = id;
       if (*pBound <= id) {
@@ -431,10 +435,14 @@ spv_result_t spvTextEncodeOperand(
     case SPV_OPERAND_TYPE_LITERAL_IN_OPTIONAL_TUPLE:
     case SPV_OPERAND_TYPE_OPTIONAL_LITERAL: {
       spv_literal_t literal = {};
-      // TODO(dneto): Is return code different for optional operands?
-      spvCheck(spvTextToLiteral(textValue, &literal),
-               DIAGNOSTIC << "Invalid literal '" << textValue << "'.";
-               return SPV_ERROR_INVALID_TEXT);
+      if (spvTextToLiteral(textValue, &literal) != SPV_SUCCESS) {
+        if (spvOperandIsOptional(type)) {
+          return SPV_FAILED_MATCH;
+        } else {
+          DIAGNOSTIC << "Invalid literal '" << textValue << "'.";
+          return SPV_ERROR_INVALID_TEXT;
+        }
+      }
       switch (literal.type) {
         // We do not have to print diagnostics here because spvBinaryEncode*
         // prints diagnostic messages on failure.
@@ -521,37 +529,100 @@ spv_result_t spvTextEncodeOperand(
   return SPV_SUCCESS;
 }
 
+namespace {
+
+/// Encodes an instruction started by !<integer> at the given position in text.
+///
+/// Puts the encoded words into *pInst.  If successful, moves position past the
+/// instruction and returns SPV_SUCCESS.  Otherwise, returns an error code and
+/// leaves position pointing to the error in text.
+spv_result_t encodeInstructionStartingWithImmediate(
+    const spv_text text, const spv_operand_table operandTable,
+    const spv_ext_inst_table extInstTable, spv_named_id_table namedIdTable,
+    uint32_t *pBound, spv_instruction_t *pInst, spv_position position,
+    spv_diagnostic *pDiagnostic) {
+  std::string firstWord;
+  spv_position_t nextPosition = {};
+  auto error = spvTextWordGet(text, position, firstWord, &nextPosition);
+  spvCheck(error, DIAGNOSTIC << "Internal Error"; return error);
+
+  assert(firstWord[0] == '!');
+  const char *begin = firstWord.data() + 1;
+  char *end = nullptr;
+  uint32_t immediateInt = strtoul(begin, &end, 0);
+  spvCheck((begin + firstWord.size() - 1) != end,
+           DIAGNOSTIC << "Invalid immediate integer '" << firstWord << "'.";
+           return SPV_ERROR_INVALID_TEXT);
+  position->column += firstWord.size();
+  position->index += firstWord.size();
+  pInst->words[0] = immediateInt;
+  pInst->wordCount = 1;
+  while (spvTextAdvance(text, position) != SPV_END_OF_STREAM) {
+    // A beginning of a new instruction means we're done.
+    if (spvTextIsStartOfNewInst(text, position)) return SPV_SUCCESS;
+
+    // Otherwise, there must be an operand that's either a literal, an ID, or
+    // an immediate.
+    std::string operandValue;
+    if ((error = spvTextWordGet(text, position, operandValue, &nextPosition))) {
+      DIAGNOSTIC << "Internal Error";
+      return error;
+    }
+
+    // Needed to pass to spvTextEncodeOpcode(), but it shouldn't ever be
+    // expanded.
+    spv_operand_pattern_t dummyExpectedOperands;
+    error = spvTextEncodeOperand(
+        SPV_OPERAND_TYPE_OPTIONAL_LITERAL, operandValue.c_str(), operandTable,
+        extInstTable, namedIdTable, pInst, &dummyExpectedOperands, pBound,
+        position, pDiagnostic);
+    if (error == SPV_FAILED_MATCH) {
+      // It's not a literal -- is it an ID?
+      error = spvTextEncodeOperand(
+          SPV_OPERAND_TYPE_OPTIONAL_ID, operandValue.c_str(), operandTable,
+          extInstTable, namedIdTable, pInst, &dummyExpectedOperands, pBound,
+          position, pDiagnostic);
+      if (error) {
+        DIAGNOSTIC << "Invalid word following " << firstWord << ": "
+                   << operandValue;
+      }
+    }
+    spvCheck(error, return error);
+    *position = nextPosition;
+  }
+  return SPV_SUCCESS;
+}
+
+}  // anonymous namespace
+
 spv_result_t spvTextEncodeOpcode(
     const spv_text text, spv_assembly_syntax_format_t format,
     const spv_opcode_table opcodeTable, const spv_operand_table operandTable,
     const spv_ext_inst_table extInstTable, spv_named_id_table namedIdTable,
     uint32_t *pBound, spv_instruction_t *pInst, spv_position position,
     spv_diagnostic *pDiagnostic) {
+  // Check for !<integer> first.
+  if ('!' == text->str[position->index]) {
+    return encodeInstructionStartingWithImmediate(
+        text, operandTable, extInstTable, namedIdTable, pBound, pInst, position,
+        pDiagnostic);
+  }
+
   // An assembly instruction has two possible formats:
   // 1(CAF): <opcode> <operand>..., e.g., "OpTypeVoid %void".
   // 2(AAF): <result-id> = <opcode> <operand>..., e.g., "%void = OpTypeVoid".
 
+  if ('!' == text->str[position->index]) {
+    return encodeInstructionStartingWithImmediate(
+        text, operandTable, extInstTable, namedIdTable, pBound, pInst, position,
+        pDiagnostic);
+  }
+
   std::string firstWord;
   spv_position_t nextPosition = {};
   spv_result_t error = spvTextWordGet(text, position, firstWord, &nextPosition);
   spvCheck(error, DIAGNOSTIC << "Internal Error"; return error);
 
-  // NOTE: Handle insertion of an immediate integer into the binary stream
-  if ('!' == text->str[position->index]) {
-    const char *begin = firstWord.data() + 1;
-    char *end = nullptr;
-    uint32_t immediateInt = strtoul(begin, &end, 0);
-    size_t size = firstWord.size() - 1;
-    spvCheck(size != (size_t)(end - begin),
-             DIAGNOSTIC << "Invalid immediate integer '" << firstWord << "'.";
-             return SPV_ERROR_INVALID_TEXT);
-    position->column += firstWord.size();
-    position->index += firstWord.size();
-    pInst->words[0] = immediateInt;
-    pInst->wordCount = 1;
-    return SPV_SUCCESS;
-  }
-
   std::string opcodeName;
   std::string result_id;
   spv_position_t result_id_position = {};

diff --git a/test/ImmediateInt.cpp b/test/ImmediateInt.cpp
index 5260a69..4ef06d2 100644 (file)
--- a/test/ImmediateInt.cpp
+++ b/test/ImmediateInt.cpp
@@ -33,8 +33,9 @@
 
 namespace {
 
-using ::testing::HasSubstr;
 using ::testing::ElementsAre;
+using ::testing::HasSubstr;
+using ::testing::StrEq;
 using test_fixture::TextToBinaryTest;
 
 TEST_F(TextToBinaryTest, ImmediateIntOpCode) {
@@ -64,7 +65,7 @@ using ImmediateIntTest = TextToBinaryTest;
 TEST_F(ImmediateIntTest, AnyWordInSimpleStatement) {
   const SpirvVector original = CompileCAFSuccessfully("OpConstant %1 %2 123");
   // TODO(deki): uncomment assertions below and make them pass.
-  // EXPECT_EQ(original, CompileCAFSuccessfully("!0x0004002B %1 %2 123"));
+  EXPECT_EQ(original, CompileCAFSuccessfully("!0x0004002B %1 %2 123"));
   EXPECT_EQ(original, CompileCAFSuccessfully("OpConstant !1 %2 123"));
   // EXPECT_EQ(original, CompileCAFSuccessfully("OpConstant %1 !2 123"));
   EXPECT_EQ(original, CompileCAFSuccessfully("OpConstant %1 %2 !123"));
@@ -152,8 +153,7 @@ TEST_F(ImmediateIntTest, IdFollowingImmediate) {
 TEST_F(ImmediateIntTest, ImmediateFollowingImmediate) {
   const SpirvVector original = CompileCAFSuccessfully("OpTypeMatrix %11 %10 7");
   EXPECT_EQ(original, CompileCAFSuccessfully("OpTypeMatrix %11 !10 !7"));
-  // TODO(deki): uncomment assertions below and make them pass.
-  // EXPECT_EQ(original, CompileCAFSuccessfully("!0x00040018 %11 !10 !7"));
+  EXPECT_EQ(original, CompileCAFSuccessfully("!0x00040018 %11 !10 !7"));
 }
 
 TEST_F(ImmediateIntTest, InvalidStatement) {
@@ -246,4 +246,13 @@ TEST_F(ImmediateIntTest, ForbiddenOperands) {
               HasSubstr("random_bareword"));
 }
 
+TEST_F(ImmediateIntTest, NotInteger) {
+  EXPECT_THAT(CompileFailure("!abc"),
+              StrEq("Invalid immediate integer '!abc'."));
+  EXPECT_THAT(CompileFailure("!12.3"),
+              StrEq("Invalid immediate integer '!12.3'."));
+  EXPECT_THAT(CompileFailure("!12K"),
+              StrEq("Invalid immediate integer '!12K'."));
+}
+
 }  // anonymous namespace