5 ms·
That struct JSONValue { std::optional<std::string> string; std::optional<double> number; std::optional<bool> boolean; std::optional<std::vect
by jcelerier 5y ago
That
struct JSONValue {
std::optional<std::string> string;
std::optional<double> number;
std::optional<bool> boolean;
std::optional<std::vector<JSONValue>> array;
std::optional<std::map<std::string, JSONValue>> object;
JSONValueType type;
};
really ought to be a variant, that would simplify things a lot. It'd just be
struct JSONValue;
using variant_type =
std::variant<
std::string
, double
, bool
, std::vector<JSONValue>
, std::map<std::string, JSONValue>
>;
struct JSONValue : variant_type {
using variant::variant;
};
then instead of switches you'd do:
std::string_view print_type(const variant_type& jtt) {
using namespace std::literals;
struct {
auto operator()(const std::string&) const noexcept { return "String"sv; }
auto operator()(double) const noexcept { return "Number"sv; }
auto operator()(bool) const noexcept { return "Bool"sv; }
auto operator()(const std::vector<JSONValue>&) const noexcept { return "Array"sv; }
auto operator()(const std::map<std::string, JSONValue>&) const noexcept { return "Dict"sv; }
auto operator()(std::monostate) const noexcept { return "Null"sv; }
} vis;
return std::visit(vis, jtt);
}
which is much safer than switch/cases
- eatonphil 5y agoI do get that it's the type-safe approach but your resulting code looks more complicated than what I did (to me). Still, thanks for sharing so that other people can see for themselves. One of the biggest turnoffs about std::variant to me (which isn't relevant here) is that you can't name the types so the hacks for having multiple of the same type in std::variant look even hairier.
- gravypod 5y agoA nice follow up would also be looking at std::string_view as this has the potential to save a lot of memory.
- eatonphil 5y agoSince I'm unfamiliar with the underlying concept, could you give an example of how you could see that being used?
- gravypod 5y ago`std::string_view` is essentially something like this: struct string_view { char *data; size_t length; } It is a pointer to a subset of an existing string. It has all of the existing features of `std::string` but passing it around is zero copy. As long as the original memory allocation exists your `string_view`s are still valid. Also, it has an implicit constructor from `const std::string` and `const char #` allowing you you define a single function like `Thing ParseThing(std::string_view line)` and accept `char #` and `std::string` as an input. More examples are here: https://abseil.io/tips/1 https://abseil.io/tips/1 # -> *. HN formats bad
- Arnavion 5y ago\* (backslash asterisk) will escape the asterisk. >Also, it has an implicit constructor from `const std::string` and `const char *` allowing you you define a single function like `Thing ParseThing(std::string_view line)` and accept `char *` and `std::string` as an input.
- jcelerier 5y agoevery function where you are reading from a string without writing to it or saving it, you can use std::string_view. Same for vector / array: anything non-owning, read-only should use std::span instead.
- jcelerier 5y agoit's not only type-safe, it uses much less memory, takes less time to construct and does not duplicate the "null" state: - your JSONStruct takes 160 bytes (on 64-bit systems with libstdc++), the variant one takes 56, almost three times less. - it needs std::optional<T> for each member to avoid construction, while std::variant does that by default. But the compiler still have to initialize all these optionals to their default state. - you need on every access two branches: * one for the type switch * one for the optional the variant only has one level of indirection.
- eatonphil 5y agoThat's fair. I assumed it would be clear to anyone who knows C++ better than I that the point of this post wasn't exactly performance. Actually I was most interested in just how simple and readable I could make it. But I didn't say that in the post anywhere. Will add.
- jcelerier 5y ago> I assumed it would be clear to anyone who knows It's just not possible to assume that at all on the internet. Every year I have students who learn from blog posts like this for instance.
- gravypod 5y agoAs someone who does quite a bit of C++ recently: for most uses readability is king. If you did want squeeze out the extra performance you could also do something where you store `std::variant<>` inside a struct and provide an API like this: if (auto int_value = value.i()) { // *int_value == your int } and expose a `std::variant<> &variant();` for use cases where you want the more complex/faster stuff.
- tcbawo 5y agoUsing variant as the storage type while providing a nice public accessor API makes sense given the huge difference in semantics between a struct containing optional fields and a variant.
- quietbritishjim 5y agoRather than using std::visit, you can just use a chain of if statements with get_if [1]. You keep the most important benefits of std::variant but the code is much more readable (IMO). [1] https://news.ycombinator.com/item?id=25316192 https://news.ycombinator.com/item?id=25316192
- jcelerier 5y agoInteresting, I find the list of of ifs much less easy on the eyes. I wonder why that is.
- quietbritishjim 5y agoAgreed that it's a personal preference. If you geniunely have a standalone function, like print_type shown here, then std::visit seems roughly even with std::get_if to me (but I'd tend to use the latter for consistency with my other code). std::visit would clearly come out ahead if you had some sort of complex nested variant and you called out to other visitors. get_if is clearly ahead IMO if you're using a variant in passing in a broader function, and especially if you when you want to use control flow (if you return in an if branch you return from the wider function).
- jcelerier 5y agoYes, this case (checking for a type and returning early) is pretty much my only usage of get_if (and in this case it makes a lot of sense)
- jcelerier 5y agoSince you're interested in readability, you may also like std::format: std::string format_parse_error(std::string base, JSONToken token) { std::ostringstream s; s << "Unexpected token '" << token.value << "', type '" << JSONTokenType_to_string(token.type) << "', index "; s << std::endl << base; return format_error(s.str(), *token.full_source, token.location); } becomes std::string format_parse_error(std::string_view base, JSONToken token) { return format_error( std::format( "Unexpected token '{}', type '{}', index '{}'\n{}" , token.value , JSONTokenType_to_string(token.type) , index , base) , *token.full_source , token.location ); }
- eatonphil 5y agoI was excited to use it! But I don't think it's available in Clang 12 (noted in this post) which is the latest version that comes with latest Fedora. I didn't want to have to spring a newer compiler version than came with the distro. Unless it's just a flag I did not turn on.
- lbhdc 5y agoYou can install it. dnf install fmt-devel
- agent327 5y agoIf you really cared about readability , you'd put your commas where normal human beings put them . Instead of in a silly place like that . Prefering structure over normal communication is not , typically , seen as a good thing .
- Kranar 5y agoYour alternative is undefined behavior due to using an incomplete type in std::map. C++17 allows std::vector to use an incomplete type but std::map does not. Reference this paper for further details: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4510.html http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n451...
- thethirdone 5y agoWhy doesn't the compiler error when compiling if it is undefined behavior? It seems really simple to detect if a type is incomplete.
- jcelerier 5y agoIt's undefined because the standard says so explicitly for these containers, but if you copy-pasted all the code of libstdc++ or libc++'s std::map in a new namespace and gave it your name it would be perfectly defined. It's a bit like naming a global variable _Weeeeeeeeee. It's technically UB because the standard says so, but in practice the compiled code will be fine if your stdlib does not use that variable, the compiler isn't actively looking for elements of the spec to check and enforce, it's just using UB where it can as a tool to optimize
- thethirdone 5y agoI was only really aware of undefined behavior that was relevant to runtime behavior. I wasn't aware using compiler reserved identifiers was undefined behavior; I assumed it was required to error. I don't see the purpose in allowing trivially detectable UB to exist. In this case and in the case of reserved identifiers, I don't see how any optimization could be made using the UB. Noticing the UB allows you to do anything, so you might as well rewrite the program to do nothing.
- account42 5y ago> I assumed it was required to error. They are reserved because the compiler (and OS) writers need identifiers they can use in their own headers without breaking valid user code. Since there is no good way for the compiler to say which headers are user headers, erroring on those symbols would be counter-productive.
- forrestthewoods 5y agostd::variant is an abomination. The idea is stellar. But actually using it is a frustrating nightmare. I prefer the simple way, unfortunately.
- ojeda 5y agoC++ may get pattern matching with P1371, which may make visiting look more reasonable: std::string_view print_type(const variant_type& jtt) { using namespace std::literals; return inpect (jtt) { <double> __ => "Number"sv; <bool> __ => "Bool"sv; ... }; } In any case, `std::variant` should have been a language feature all along, instead of a library one; cf. Rust: enum Value { Number(f64), Bool(bool), ... } fn print_type(v: &Value) -> &str { match v { Value::Number(_) => "Number", Value::Bool(_) => "Bool", ... } }
- jcelerier 5y agoYes, pattern-matching like this would be the bee's knees
- deleted 5y ago[deleted]