Improve Variant::from_string() (#603)

LLNL · Oct 8, 2024 · f95bca8 · f95bca8
1 parent c101dca
commit f95bca8
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 47 deletions.
diff --git a/include/caliper/common/Variant.h b/include/caliper/common/Variant.h
@@ -125,7 +125,7 @@ class Variant
         return Variant(cali_variant_unpack(buf, inc, ok));
     }
 
-    static Variant from_string(cali_attr_type type, const char* str, bool* ok = nullptr);
+    static Variant from_string(cali_attr_type type, const char* str);
 
     // vector<unsigned char> data() const;
 

diff --git a/src/common/Variant.cpp b/src/common/Variant.cpp
@@ -9,6 +9,7 @@
 #include "caliper/common/StringConverter.h"
 
 #include "util/format_util.h"
+#include "util/parse_util.h"
 
 #include <algorithm>
 #include <cctype>
@@ -98,76 +99,53 @@ Variant::to_string() const
 }
 
 Variant
-Variant::from_string(cali_attr_type type, const char* str, bool* okptr)
+Variant::from_string(cali_attr_type type, const char* str)
 {
-    Variant ret;
-    bool    ok = false;
-
     switch (type) {
     case CALI_TYPE_INV:
     case CALI_TYPE_USR:
     case CALI_TYPE_PTR:
-        // Can't convert USR or INV types at this point
-        break;
+        return Variant();
     case CALI_TYPE_STRING:
-        ret = Variant(CALI_TYPE_STRING, str, strlen(str));
-        ok  = true;
-        break;
+        return Variant(CALI_TYPE_STRING, str, strlen(str));
     case CALI_TYPE_INT:
         {
-            int64_t i = StringConverter(str).to_int64(&ok);
-
-            if (ok)
-                ret = Variant(cali_make_variant_from_int64(i));
+            char* str_end = nullptr;
+            long long ll = std::strtoll(str, &str_end, 10);
+            return str != str_end ? Variant(cali_make_variant_from_int64(ll)) : Variant();
         }
-        break;
     case CALI_TYPE_ADDR:
         {
+            bool ok = false;
             uint64_t u = StringConverter(str).to_uint(&ok, 16);
-
-            if (ok)
-                ret = Variant(CALI_TYPE_ADDR, &u, sizeof(uint64_t));
+            return ok ? Variant(CALI_TYPE_ADDR, &u, sizeof(uint64_t)) : Variant();
         }
-        break;
     case CALI_TYPE_UINT:
         {
-            uint64_t u = StringConverter(str).to_uint(&ok);
-
-            if (ok)
-                ret = Variant(CALI_TYPE_UINT, &u, sizeof(uint64_t));
+            auto p = util::str_to_uint64(str);
+            return p.first ? Variant(cali_make_variant_from_uint(p.second)) : Variant();
         }
-        break;
     case CALI_TYPE_DOUBLE:
         {
-            double d = StringConverter(str).to_double(&ok);
-
-            if (ok)
-                ret = Variant(d);
+            char* str_end = nullptr;
+            double d = std::strtod(str, &str_end);
+            return str != str_end ? Variant(d) : Variant();
         }
-        break;
     case CALI_TYPE_BOOL:
         {
+            bool ok = false;
             bool b = StringConverter(str).to_bool(&ok);
-
-            if (ok)
-                ret = Variant(b);
+            return ok ? Variant(b) : Variant();
         }
-        break;
     case CALI_TYPE_TYPE:
         {
             cali_attr_type type = cali_string2type(str);
-            ok = (type != CALI_TYPE_INV);
+            return (type != CALI_TYPE_INV) ? Variant(type) : Variant();
 
-            if (ok)
-                ret = Variant(type);
         }
-        break;
     }
 
-    if (okptr)
-        *okptr = ok;
-
-    return ret;
+    return Variant();
 }
 
 std::ostream&

diff --git a/src/common/test/test_variant.cpp b/src/common/test/test_variant.cpp
@@ -50,10 +50,9 @@ TEST(Variant_Test, FromString) {
     };
 
     for (const testcase_t* t = testcases; t->str; ++t) {
-        bool    ok = false;
-        Variant v(Variant::from_string(t->type, t->str, &ok));
+        Variant v(Variant::from_string(t->type, t->str));
 
-        EXPECT_EQ(ok, t->ok) << "for \"" << t->str << "\" (" << cali_type2string(t->type) << ")";
+        EXPECT_EQ(!v.empty(), t->ok) << "for \"" << t->str << "\" (" << cali_type2string(t->type) << ")";
         EXPECT_EQ(v, t->expected) << "for \"" << t->str << "\" (" << cali_type2string(t->type) << ")";
     }
 }
@@ -97,7 +96,7 @@ TEST(Variant_Test, Conversions) {
         EXPECT_EQ(val_int, val_1_int_neg);
     }
     {
-        bool v_1_int_neg_to_i64 = false;        
+        bool v_1_int_neg_to_i64 = false;
         int64_t val_i64 = v_1_int_neg.to_int64(&v_1_int_neg_to_i64);
         EXPECT_TRUE(v_1_int_neg_to_i64);
         EXPECT_EQ(val_i64, static_cast<int64_t>(val_1_int_neg));
@@ -123,7 +122,7 @@ TEST(Variant_Test, Conversions) {
         bool v_2_i64_nlrg_to_uint = true;
         v_2_i64_nlrg.to_uint(&v_2_i64_nlrg_to_uint);
         EXPECT_FALSE(v_2_i64_nlrg_to_uint);
-    }    
+    }
 
     {
         bool v_3_uint_sml_to_int = false;

diff --git a/src/common/util/parse_util.h b/src/common/util/parse_util.h
@@ -37,4 +37,14 @@ read_nested_text(std::istream& is, char start_char, char end_char);
 char
 read_char(std::istream& is);
 
+inline std::pair<bool, uint64_t>
+str_to_uint64(const char* str)
+{
+    uint64_t ret = 0;
+    const char* p = str;
+    for ( ; *p >= '0' && *p <= '9'; ++p)
+        ret = ret * 10 + static_cast<uint64_t>(*p - '0');
+    return std::make_pair(p != str, ret);
+}
+
 }
diff --git a/src/reader/RecordSelector.cpp b/src/reader/RecordSelector.cpp
@@ -113,7 +113,7 @@ struct RecordSelector::RecordSelectorImpl
         Clause clause { f.op, db.get_attribute(f.attr_name), Variant() };
 
         if (clause.attr)
-            clause.value = Variant::from_string(clause.attr.type(), f.value.c_str(), nullptr);
+            clause.value = Variant::from_string(clause.attr.type(), f.value.c_str());
 
         return clause;
     }