Fix some bugs in parsing literals.

- a single hyphen is a string, not a number.
- a string with more than one period is a string, not a number
- check for string overflow

Add some unit tests

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8157b39..18732d1 100644 (file)
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -171,6 +171,7 @@ if (TARGET gtest)
     ${CMAKE_CURRENT_SOURCE_DIR}/test/OpcodeSplit.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test/OperandTableGet.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test/TextAdvance.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/test/TextLiteral.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test/TextWordGet.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/test/TextDestroy.cpp

diff --git a/source/text.cpp b/source/text.cpp
index dd4109b..9a412aa 100644 (file)
--- a/source/text.cpp
+++ b/source/text.cpp
@@ -34,7 +34,7 @@
 
 #include <assert.h>
 #include <stdio.h>
-#include <stdlib.h>
+#include <cstdlib>
 #include <string.h>
 
 #include <string>
@@ -247,14 +247,11 @@ spv_result_t spvTextToUInt32(const char *textValue, uint32_t *pValue) {
 
 spv_result_t spvTextToLiteral(const char *textValue, spv_literal_t *pLiteral) {
   bool isSigned = false;
-  bool isFloat = false;
+  int numPeriods = 0;
   bool isString = false;
 
-  if ('-' == textValue[0]) {
-    isSigned = true;
-  }
-
-  for (uint64_t index = 0; index < strlen(textValue); ++index) {
+  const size_t len = strlen(textValue);
+  for (uint64_t index = 0; index < len; ++index) {
     switch (textValue[index]) {
       case '0':
       case '1':
@@ -268,19 +265,33 @@ spv_result_t spvTextToLiteral(const char *textValue, spv_literal_t *pLiteral) {
       case '9':
         break;
       case '.':
-        isFloat = true;
+        numPeriods++;
+        break;
+      case '-':
+        if (index == 0) {
+          isSigned = true;
+        } else {
+          isString = true;
+        }
         break;
       default:
         isString = true;
+        index = len; // break out of the loop too.
         break;
     }
   }
 
-  if (isString) {
+  pLiteral->type = spv_literal_type_t(99);
+
+  if (isString || numPeriods > 1 || (isSigned && len==1)) {
+    // TODO(dneto): Quotes should be required, and stripped.
+    // TODO(dneto): Allow escaping.
     pLiteral->type = SPV_LITERAL_TYPE_STRING;
-    strncpy(pLiteral->value.str, textValue, strlen(textValue));
-  } else if (isFloat) {
-    double d = strtod(textValue, nullptr);
+    // Need room for the null-terminator.
+    if (len + 1 > sizeof(pLiteral->value.str)) return SPV_ERROR_OUT_OF_MEMORY;
+    strncpy(pLiteral->value.str, textValue, len+1);
+  } else if (numPeriods == 1) {
+    double d = std::strtod(textValue, nullptr);
     float f = (float)d;
     if (d == (double)f) {
       pLiteral->type = SPV_LITERAL_TYPE_FLOAT_32;

diff --git a/test/TextLiteral.cpp b/test/TextLiteral.cpp
new file mode 100644 (file)
index 0000000..7139b43
--- /dev/null
+++ b/test/TextLiteral.cpp
@@ -0,0 +1,123 @@
+// Copyright (c) 2015 The Khronos Group Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and/or associated documentation files (the
+// "Materials"), to deal in the Materials without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Materials, and to
+// permit persons to whom the Materials are furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Materials.
+//
+// MODIFICATIONS TO THIS FILE MAY MEAN IT NO LONGER ACCURATELY REFLECTS
+// KHRONOS STANDARDS. THE UNMODIFIED, NORMATIVE VERSIONS OF KHRONOS
+// SPECIFICATIONS AND HEADER INFORMATION ARE LOCATED AT
+//    https://www.khronos.org/registry/
+//
+// THE MATERIALS ARE PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+// IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+// CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+// TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+// MATERIALS OR THE USE OR OTHER DEALINGS IN THE MATERIALS.
+
+#include "UnitSPIRV.h"
+
+#include <string>
+
+TEST(TextLiteral, GoodI32) {
+  spv_literal_t l;
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("-0", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_INT_32, l.type);
+  EXPECT_EQ(0, l.value.i32);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("-2147483648", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_INT_32, l.type);
+  EXPECT_EQ(-2147483648, l.value.i32);
+}
+
+TEST(TextLiteral, GoodU32) {
+  spv_literal_t l;
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("0", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_UINT_32, l.type);
+  EXPECT_EQ(0, l.value.i32);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("4294967295", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_UINT_32, l.type);
+  EXPECT_EQ(4294967295, l.value.u32);
+}
+
+TEST(TextLiteral, GoodI64) {
+  spv_literal_t l;
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("-2147483649", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_INT_64, l.type);
+  EXPECT_EQ(-2147483649, l.value.i64);
+}
+
+TEST(TextLiteral, GoodU64) {
+  spv_literal_t l;
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("4294967296", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_UINT_64, l.type);
+  EXPECT_EQ(4294967296, l.value.u64);
+}
+
+TEST(TextLiteral, GoodFloat) {
+  spv_literal_t l;
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("1.0", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_FLOAT_32, l.type);
+  EXPECT_EQ(1.0, l.value.f);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("1.5", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_FLOAT_32, l.type);
+  EXPECT_EQ(1.5, l.value.f);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("-.25", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_FLOAT_32, l.type);
+  EXPECT_EQ(-.25, l.value.f);
+}
+
+TEST(TextLiteral, GoodString) {
+  spv_literal_t l;
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("-", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_STRING, l.type);
+  EXPECT_STREQ("-", l.value.str);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("--", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_STRING, l.type);
+  EXPECT_STREQ("--", l.value.str);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("1-2", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_STRING, l.type);
+  EXPECT_STREQ("1-2", l.value.str);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("123a", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_STRING, l.type);
+  EXPECT_STREQ("123a", l.value.str);
+
+  ASSERT_EQ(SPV_SUCCESS, spvTextToLiteral("12.2.3", &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_STRING, l.type);
+  EXPECT_STREQ("12.2.3", l.value.str);
+}
+
+TEST(TextLiteral, StringTooLong) {
+  spv_literal_t l;
+  std::string too_long(SPV_LIMIT_LITERAL_STRING_MAX, 'a');
+  EXPECT_EQ(SPV_ERROR_OUT_OF_MEMORY, spvTextToLiteral(too_long.data(), &l));
+}
+
+TEST(TextLiteral, GoodLongString) {
+  spv_literal_t l;
+  std::string good_long(SPV_LIMIT_LITERAL_STRING_MAX-1, 'a');
+  EXPECT_EQ(SPV_SUCCESS, spvTextToLiteral(good_long.data(), &l));
+  EXPECT_EQ(SPV_LITERAL_TYPE_STRING, l.type);
+  EXPECT_STREQ(good_long.data(), l.value.str);
+}