[torque-ls][cleanup] JsonValue accessors are now checked

Instead of accessing JsonValue struct fields directly, typed
accessors check that the tag matches with the type access.

Drive-by: The factory methods are now static methods on the JsonValue
type itself, making call-sites more readable.

R=tebbi@chromium.org

Bug: v8:8880
Change-Id: I49b37b3ba8eaf1153b8aa93ea08913077c923fdc
Reviewed-on: https://chromium-review.googlesource.com/c/1495559
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59968}

src/torque/ls/json-parser.cc

@@ -37,7 +37,7 @@ using JsonMember = std::pair<std::string, JsonValue>;
 template <bool value>
 base::Optional<ParseResult> MakeBoolLiteral(
     ParseResultIterator* child_results) {
-  return ParseResult{From(value)};
+  return ParseResult{JsonValue::From(value)};
 }
 
 base::Optional<ParseResult> MakeNullLiteral(
@@ -51,18 +51,18 @@ base::Optional<ParseResult> MakeNumberLiteral(
     ParseResultIterator* child_results) {
   auto number = child_results->NextAs<std::string>();
   double d = std::stod(number.c_str());
-  return ParseResult{From(d)};
+  return ParseResult{JsonValue::From(d)};
 }
 
 base::Optional<ParseResult> MakeStringLiteral(
     ParseResultIterator* child_results) {
   std::string literal = child_results->NextAs<std::string>();
-  return ParseResult{From(StringLiteralUnquote(literal))};
+  return ParseResult{JsonValue::From(StringLiteralUnquote(literal))};
 }
 
 base::Optional<ParseResult> MakeArray(ParseResultIterator* child_results) {
   JsonArray array = child_results->NextAs<JsonArray>();
-  return ParseResult{From(std::move(array))};
+  return ParseResult{JsonValue::From(std::move(array))};
 }
 
 base::Optional<ParseResult> MakeMember(ParseResultIterator* child_results) {
@@ -80,7 +80,7 @@ base::Optional<ParseResult> MakeObject(ParseResultIterator* child_results) {
   JsonObject object;
   for (auto& member : members) object.insert(std::move(member));
 
-  return ParseResult{From(std::move(object))};
+  return ParseResult{JsonValue::From(std::move(object))};
 }
 
 class JsonGrammar : public Grammar {

src/torque/ls/json.cc

@@ -18,24 +18,24 @@ namespace {
 void SerializeToString(std::stringstream& str, const JsonValue& value) {
   switch (value.tag) {
     case JsonValue::NUMBER:
-      str << value.number;
+      str << value.ToNumber();
       break;
     case JsonValue::STRING:
-      str << StringLiteralQuote(value.string);
+      str << StringLiteralQuote(value.ToString());
       break;
     case JsonValue::IS_NULL:
       str << "\"null\"";
       break;
     case JsonValue::BOOL:
-      str << (value.flag ? "\"true\"" : "\"false\"");
+      str << (value.ToBool() ? "\"true\"" : "\"false\"");
       break;
     case JsonValue::OBJECT: {
       str << "{";
       size_t i = 0;
-      for (const auto& pair : *value.object) {
+      for (const auto& pair : value.ToObject()) {
         str << "\"" << pair.first << "\":";
         SerializeToString(str, pair.second);
-        if (++i < value.object->size()) str << ",";
+        if (++i < value.ToObject().size()) str << ",";
       }
       str << "}";
       break;
@@ -43,9 +43,9 @@ void SerializeToString(std::stringstream& str, const JsonValue& value) {
     case JsonValue::ARRAY: {
       str << "[";
       size_t i = 0;
-      for (const auto& element : *value.array) {
+      for (const auto& element : value.ToArray()) {
         SerializeToString(str, element);
-        if (++i < value.array->size()) str << ",";
+        if (++i < value.ToArray().size()) str << ",";
       }
       str << "]";
       break;

src/torque/ls/json.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "src/base/logging.h"
 #include "src/base/template-utils.h"
 
 namespace v8 {
@@ -22,54 +23,95 @@ using JsonObject = std::map<std::string, JsonValue>;
 using JsonArray = std::vector<JsonValue>;
 
 struct JsonValue {
+ public:
   enum { OBJECT, ARRAY, STRING, NUMBER, BOOL, IS_NULL } tag;
-  std::unique_ptr<JsonObject> object;
-  std::unique_ptr<JsonArray> array;
-  std::string string;
-  double number = 0;
-  bool flag = false;
-};
 
-inline JsonValue From(JsonObject object) {
+  static JsonValue From(double number) {
     JsonValue result;
-  result.tag = JsonValue::OBJECT;
-  result.object = base::make_unique<JsonObject>(std::move(object));
+    result.tag = JsonValue::NUMBER;
+    result.number_ = number;
     return result;
-}
+  }
 
-inline JsonValue From(const std::string& string) {
+  static JsonValue From(JsonObject object) {
     JsonValue result;
-  result.tag = JsonValue::STRING;
-  result.string = string;
+    result.tag = JsonValue::OBJECT;
+    result.object_ = base::make_unique<JsonObject>(std::move(object));
     return result;
-}
+  }
 
-inline JsonValue From(double number) {
+  static JsonValue From(bool b) {
     JsonValue result;
-  result.tag = JsonValue::NUMBER;
-  result.number = number;
+    result.tag = JsonValue::BOOL;
+    result.flag_ = b;
     return result;
-}
+  }
 
-inline JsonValue From(bool b) {
+  static JsonValue From(const std::string& string) {
     JsonValue result;
-  result.tag = JsonValue::BOOL;
-  result.flag = b;
+    result.tag = JsonValue::STRING;
+    result.string_ = string;
     return result;
-}
+  }
 
-inline JsonValue From(JsonArray array) {
+  static JsonValue From(JsonArray array) {
     JsonValue result;
     result.tag = JsonValue::ARRAY;
-  result.array = base::make_unique<JsonArray>(std::move(array));
+    result.array_ = base::make_unique<JsonArray>(std::move(array));
     return result;
-}
+  }
 
-inline JsonValue JsonNull() {
+  static JsonValue JsonNull() {
     JsonValue result;
     result.tag = JsonValue::IS_NULL;
     return result;
-}
+  }
+
+  bool IsNumber() const { return tag == NUMBER; }
+  double ToNumber() const {
+    CHECK(IsNumber());
+    return number_;
+  }
+
+  bool IsBool() const { return tag == BOOL; }
+  bool ToBool() const {
+    CHECK(IsBool());
+    return flag_;
+  }
+
+  bool IsString() const { return tag == STRING; }
+  const std::string& ToString() const {
+    CHECK(IsString());
+    return string_;
+  }
+
+  bool IsObject() const { return object_ && tag == OBJECT; }
+  const JsonObject& ToObject() const {
+    CHECK(IsObject());
+    return *object_;
+  }
+  JsonObject& ToObject() {
+    CHECK(IsObject());
+    return *object_;
+  }
+
+  bool IsArray() const { return array_ && tag == ARRAY; }
+  const JsonArray& ToArray() const {
+    CHECK(IsArray());
+    return *array_;
+  }
+  JsonArray& ToArray() {
+    CHECK(IsArray());
+    return *array_;
+  }
+
+ private:
+  double number_ = 0;
+  bool flag_ = false;
+  std::string string_;
+  std::unique_ptr<JsonObject> object_;
+  std::unique_ptr<JsonArray> array_;
+};
 
 std::string SerializeToString(const JsonValue& value);
 

src/torque/ls/message-handler.cc

@@ -114,12 +114,13 @@ void HandleTorqueFileListNotification(TorqueFileListNotification notification) {
 
   std::vector<std::string>& files = TorqueFileList::Get();
   Logger::Log("[info] Initial file list:\n");
-  for (const auto& fileJson : *notification.params().object()["files"].array) {
-    CHECK_EQ(fileJson.tag, JsonValue::STRING);
+  for (const auto& fileJson :
+       notification.params().object()["files"].ToArray()) {
+    CHECK(fileJson.IsString());
     // We only consider file URIs (there shouldn't be anything else).
-    if (fileJson.string.rfind(kFileUriPrefix) != 0) continue;
+    if (fileJson.ToString().rfind(kFileUriPrefix) != 0) continue;
 
-    std::string file = fileJson.string.substr(kFileUriPrefixLength);
+    std::string file = fileJson.ToString().substr(kFileUriPrefixLength);
     files.push_back(file);
     Logger::Log("    ", file, "\n");
   }

src/torque/ls/message-macros.h

@@ -11,22 +11,24 @@ namespace torque {
 namespace ls {
 
 #define JSON_STRING_ACCESSORS(name)                \
-  inline const std::string& name() const { return object().at(#name).string; } \
+  inline const std::string& name() const {         \
+    return object().at(#name).ToString();          \
+  }                                                \
   inline void set_##name(const std::string& str) { \
-    object()[#name] = From(str);                                               \
+    object()[#name] = JsonValue::From(str);        \
   }                                                \
   inline bool has_##name() const {                 \
     return object().find(#name) != object().end(); \
   }
 
 #define JSON_BOOL_ACCESSORS(name)                                  \
-  inline bool name() const { return object().at(#name).flag; } \
-  inline void set_##name(bool b) { object()[#name] = From(b); }
+  inline bool name() const { return object().at(#name).ToBool(); } \
+  inline void set_##name(bool b) { object()[#name] = JsonValue::From(b); }
 
 #define JSON_INT_ACCESSORS(name)                                    \
-  inline int name() const { return object().at(#name).number; } \
+  inline int name() const { return object().at(#name).ToNumber(); } \
   inline void set_##name(int n) {                                   \
-    object()[#name] = From(static_cast<double>(n));             \
+    object()[#name] = JsonValue::From(static_cast<double>(n));      \
   }
 
 #define JSON_OBJECT_ACCESSORS(type, name) \
@@ -46,7 +48,7 @@ namespace ls {
   inline std::size_t name##_size() { return GetArrayProperty(#name).size(); } \
   inline type name(size_t idx) {                                              \
     CHECK(idx < name##_size());                                               \
-    return type(*GetArrayProperty(#name)[idx].object);                        \
+    return type(GetArrayProperty(#name)[idx].ToObject());                     \
   }
 
 }  // namespace ls

src/torque/ls/message.h

@@ -28,7 +28,9 @@ class BaseJsonAccessor {
     return object().count(property) > 0;
   }
 
-  void SetNull(const std::string& property) { object()[property] = JsonNull(); }
+  void SetNull(const std::string& property) {
+    object()[property] = JsonValue::JsonNull();
+  }
 
   bool IsNull(const std::string& property) const {
     return HasProperty(property) &&
@@ -40,24 +42,24 @@ class BaseJsonAccessor {
   virtual JsonObject& object() = 0;
 
   JsonObject& GetObjectProperty(const std::string& property) {
-    if (!object()[property].object) {
-      object()[property] = From(JsonObject{});
+    if (!object()[property].IsObject()) {
+      object()[property] = JsonValue::From(JsonObject{});
     }
-    return *object()[property].object;
+    return object()[property].ToObject();
   }
 
   JsonArray& GetArrayProperty(const std::string& property) {
-    if (!object()[property].array) {
-      object()[property] = From(JsonArray{});
+    if (!object()[property].IsArray()) {
+      object()[property] = JsonValue::From(JsonArray{});
     }
-    return *object()[property].array;
+    return object()[property].ToArray();
   }
 
   JsonObject& AddObjectElementToArrayProperty(const std::string& property) {
     JsonArray& array = GetArrayProperty(property);
-    array.push_back(From(JsonObject{}));
+    array.push_back(JsonValue::From(JsonObject{}));
 
-    return *array.back().object;
+    return array.back().ToObject();
   }
 };
 
@@ -67,7 +69,7 @@ class BaseJsonAccessor {
 class Message : public BaseJsonAccessor {
  public:
   Message() {
-    value_ = From(JsonObject{});
+    value_ = JsonValue::From(JsonObject{});
     set_jsonrpc("2.0");
   }
   explicit Message(JsonValue& value) : value_(std::move(value)) {
@@ -79,8 +81,8 @@ class Message : public BaseJsonAccessor {
   JSON_STRING_ACCESSORS(jsonrpc)
 
  protected:
-  const JsonObject& object() const { return *value_.object; }
-  JsonObject& object() { return *value_.object; }
+  const JsonObject& object() const { return value_.ToObject(); }
+  JsonObject& object() { return value_.ToObject(); }
 
  private:
   JsonValue value_;

test/cctest/torque/test-torque-ls-json.cc

@@ -14,80 +14,80 @@ namespace ls {
 TEST(TestJsonPrimitives) {
   const JsonValue true_result = ParseJson("true");
   CHECK_EQ(true_result.tag, JsonValue::BOOL);
-  CHECK_EQ(true_result.flag, true);
+  CHECK_EQ(true_result.ToBool(), true);
 
   const JsonValue false_result = ParseJson("false");
   CHECK_EQ(false_result.tag, JsonValue::BOOL);
-  CHECK_EQ(false_result.flag, false);
+  CHECK_EQ(false_result.ToBool(), false);
 
   const JsonValue null_result = ParseJson("null");
   CHECK_EQ(null_result.tag, JsonValue::IS_NULL);
 
   const JsonValue number = ParseJson("42");
   CHECK_EQ(number.tag, JsonValue::NUMBER);
-  CHECK_EQ(number.number, 42);
+  CHECK_EQ(number.ToNumber(), 42);
 }
 
 TEST(TestJsonStrings) {
   const JsonValue basic = ParseJson("\"basic\"");
   CHECK_EQ(basic.tag, JsonValue::STRING);
-  CHECK_EQ(basic.string, "basic");
+  CHECK_EQ(basic.ToString(), "basic");
 
   const JsonValue singleQuote = ParseJson("\"'\"");
   CHECK_EQ(singleQuote.tag, JsonValue::STRING);
-  CHECK_EQ(singleQuote.string, "'");
+  CHECK_EQ(singleQuote.ToString(), "'");
 }
 
 TEST(TestJsonArrays) {
   const JsonValue empty_array = ParseJson("[]");
   CHECK_EQ(empty_array.tag, JsonValue::ARRAY);
-  CHECK_EQ(empty_array.array->size(), 0);
+  CHECK_EQ(empty_array.ToArray().size(), 0);
 
   const JsonValue number_array = ParseJson("[1, 2, 3, 4]");
   CHECK_EQ(number_array.tag, JsonValue::ARRAY);
 
-  const JsonArray& array = *number_array.array;
+  const JsonArray& array = number_array.ToArray();
   CHECK_EQ(array.size(), 4);
   CHECK_EQ(array[1].tag, JsonValue::NUMBER);
-  CHECK_EQ(array[1].number, 2);
+  CHECK_EQ(array[1].ToNumber(), 2);
 
   const JsonValue string_array_object = ParseJson("[\"a\", \"b\"]");
   CHECK_EQ(string_array_object.tag, JsonValue::ARRAY);
 
-  const JsonArray& string_array = *string_array_object.array;
+  const JsonArray& string_array = string_array_object.ToArray();
   CHECK_EQ(string_array.size(), 2);
   CHECK_EQ(string_array[1].tag, JsonValue::STRING);
-  CHECK_EQ(string_array[1].string, "b");
+  CHECK_EQ(string_array[1].ToString(), "b");
 }
 
 TEST(TestJsonObjects) {
   const JsonValue empty_object = ParseJson("{}");
   CHECK_EQ(empty_object.tag, JsonValue::OBJECT);
-  CHECK_EQ(empty_object.object->size(), 0);
+  CHECK_EQ(empty_object.ToObject().size(), 0);
 
   const JsonValue primitive_fields = ParseJson("{ \"flag\": true, \"id\": 5}");
   CHECK_EQ(primitive_fields.tag, JsonValue::OBJECT);
 
-  const JsonValue& flag = primitive_fields.object->at("flag");
+  const JsonValue& flag = primitive_fields.ToObject().at("flag");
   CHECK_EQ(flag.tag, JsonValue::BOOL);
-  CHECK(flag.flag);
+  CHECK(flag.ToBool());
 
-  const JsonValue& id = primitive_fields.object->at("id");
+  const JsonValue& id = primitive_fields.ToObject().at("id");
   CHECK_EQ(id.tag, JsonValue::NUMBER);
-  CHECK_EQ(id.number, 5);
+  CHECK_EQ(id.ToNumber(), 5);
 
   const JsonValue& complex_fields =
       ParseJson("{ \"array\": [], \"object\": { \"name\": \"torque\" } }");
   CHECK_EQ(complex_fields.tag, JsonValue::OBJECT);
 
-  const JsonValue& array = complex_fields.object->at("array");
+  const JsonValue& array = complex_fields.ToObject().at("array");
   CHECK_EQ(array.tag, JsonValue::ARRAY);
-  CHECK_EQ(array.array->size(), 0);
+  CHECK_EQ(array.ToArray().size(), 0);
 
-  const JsonValue& object = complex_fields.object->at("object");
+  const JsonValue& object = complex_fields.ToObject().at("object");
   CHECK_EQ(object.tag, JsonValue::OBJECT);
-  CHECK_EQ(object.object->at("name").tag, JsonValue::STRING);
-  CHECK_EQ(object.object->at("name").string, "torque");
+  CHECK_EQ(object.ToObject().at("name").tag, JsonValue::STRING);
+  CHECK_EQ(object.ToObject().at("name").ToString(), "torque");
 }
 
 }  // namespace ls