JSON reader based on a pushdown automata

Question

I haven't used C++ in over a decade, so to learn modern C++ (17) I built a JSON reader based on a pushdown automata (PDA).

The general idea is this:

• A stream is passed to JsonReader
• JsonReader calls ParserStatesManager, which returns the next state based on the top of the PDA stack, and optionally by reading in an input char
• JsonReader::ProcessState then process the returned state - pops/pushes the PDA stack, optionally copies the input char to the current JValue, etc
• Loop until input is finished or until the error state is reached

Since this is the first time I've touched C++ in a very long time, I'm looking for tips or suggestions on:

• Is this good, clean, idiomatic C++17?
  • Is the project layout and class structure what you would expect?
  • I have some header-only classes, is that okay?
  • Are things named right?
  • Is there any code in headers which should be in implementation files, or vice versa?
  • Are rvalues/lvalues, references, const, auto, and other C++ features being used correctly?
  • I wrote Java-esq getters and setters for private members - is this the idiomatic C++ way for accessors/mutators?
• Performance - any issues I've overlooked?
  • Any excessive copying or other issues?
  • I'm not using pointers anywhere - all memory is either on the stack or managed by STL classes (like vector), is that okay?

And of course, any other suggestions for improvement would be greatly appreciated.

The full project is here - https://github.com/marklar423/jsonreadercpp - and I've added the main relevant files below

main.cc - showing usage

int main(int argc, char *argv[])
{
    JsonReader parser;
    std::optional<JValue> parsed_json = parser.ParseJsonString(cin);

    if (parsed_json.has_value())
        PrintJsonTree(parsed_json.value());
    else
        cout << "No root object in JSON\n";

    return 0;
}

JsonReader.h - This is the main class that parses the JSON.

I'm including all the #includes in this one, as an example of how I use them in the project. #includes are omitted in subsequent files for brevity.

#ifndef JSONREADERCPP_JSONDESERIALIZER_H_
#define JSONREADERCPP_JSONDESERIALIZER_H_

#include <optional>
#include <iostream>

#include "jsonreadercpp/JValue.h"
#include "jsonreadercpp/statemachine/ParserStatesSymbols.h"
#include "jsonreadercpp/statemachine/ParserStatesManager.h"
#include "jsonreadercpp/statemachine/ParserValueStack.h"
#include "jsonreadercpp/statemachine/ParserMachineStack.h"

namespace jsonreadercpp 
{

    class JsonReader 
    {
        public:
            JsonReader(std::ostream& error_output_stream = std::cerr, 
                        std::ostream& debug_output_stream = std::cout, bool debug_output = false);

            std::optional<JValue> ParseJsonString(std::istream& input);

        private:
            std::ostream& error_output_stream_;
            std::ostream& debug_output_stream_;
            bool debug_output_;            

            ParserStatesManager states_manager_;

            ParserStatesManager::NextTransition ProcessState(std::istream& input, ParserStateType current_state_type,
                                                                ParserValueStack& value_stack, ParserMachineStack& machine_stack);

    };
}
#endif

JsonReader.cc

#include "jsonreadercpp/JsonReader.h"

using std::string;
using std::pair;
using std::optional;

namespace jsonreadercpp 
{
    JsonReader::JsonReader(std::ostream& error_output_stream /*= std::cerr*/, 
                        std::ostream& debug_output_stream /*= std::cout*/, bool debug_output /*= false*/)
        :  states_manager_(),
            error_output_stream_(error_output_stream), 
           debug_output_stream_(debug_output_stream), debug_output_(debug_output)
    {        
    } 

    optional<JValue> JsonReader::ParseJsonString(std::istream& input) 
    {        
        auto current_state_type = ParserStateType::Start;

        ParserValueStack value_stack;
        ParserMachineStack machine_stack;

        char processed_char = '\0';
        bool finished_input = false, has_error = false;

        int line_num = 1, char_num = 0, line_char_start = 0;

        input >> std::noskipws;

        while (!finished_input && !has_error) 
        {   
            if (debug_output_)
                debug_output_stream_ << '[' << char_num << ']' << processed_char << " => " << ParserStateTypeName(current_state_type) << "\n";

            if (current_state_type == ParserStateType::Error)
            {
                error_output_stream_ << "Error: Unexpected \"" << processed_char << "\""
                                     << " at Line:" << line_num << ", Line Position:" << (char_num - line_char_start) 
                                     << ", Global Position:" << char_num << "\n";                
                has_error = true;
            }
            else
            {        
                auto next_transition_container = ProcessState(input, current_state_type, value_stack, machine_stack);

                processed_char = next_transition_container.processed_char;
                current_state_type = next_transition_container.transition.next_state;
                finished_input = next_transition_container.finished_input;

                if (processed_char != '\0') char_num++;
                if (processed_char == '\n') 
                {
                    line_num++;
                    line_char_start = char_num;
                }
            }
        }

        optional<JValue> result;

        if (!has_error)
        {
            if (value_stack.GetSize() > 1)
            {
                error_output_stream_ << "Error: Unexpected end of input, JSON isn't complete\n";            
            }
            else
            {
                result = std::move(value_stack.RemoveRootValue());
            }
        }

        return result;
    }

    ParserStatesManager::NextTransition JsonReader::ProcessState(std::istream& input, ParserStateType current_state_type,
                                                                    ParserValueStack& value_stack, ParserMachineStack& machine_stack)    
    {        
        //get next state
        auto stack_symbol = (machine_stack.GetSize() > 0) ? machine_stack.GetTop() : ParserStackSymbol::None;   

        auto next_transition_container = states_manager_.GetNextTransition(current_state_type, input, stack_symbol);
        const auto& next_transition = next_transition_container.transition;

        if (!next_transition_container.finished_input)
        {
            //stack actions
            machine_stack.PopPush(next_transition.stack_pop, next_transition.stack_push);

            //input actions
            value_stack.AccumulateInput(next_transition_container.processed_char, next_transition.char_destination, current_state_type);

            //JValue actions
            if (next_transition.value_action == ParserValueAction::Push || next_transition.value_action == ParserValueAction::PushPop)
            {
                value_stack.PushJValue(next_transition.value_push_type.value());
            }

            if (next_transition.value_action == ParserValueAction::Pop || next_transition.value_action == ParserValueAction::PushPop)
            {
                value_stack.PopJValue();
            }
        }

        return next_transition_container;
    } 
}

JValue.h

namespace jsonreadercpp 
{

    enum class JValueType 
    {
        String,
        Number,
        Boolean,
        Null,
        Object,
        Array
    };

    class JValue 
    {
        public:        
            //create an object, array, or null
            JValue(JValueType value_type) : JValue(value_type, {}) {}

            //create a scalar value
            JValue(std::string value) : JValue(JValueType::String, {value}) {}
            JValue(double value)      : JValue(JValueType::Number, {value}) {}
            JValue(bool value)        : JValue(JValueType::Boolean, {value}) {}

            JValue(JValue&& other) = default;
            JValue& operator=(JValue&& other) = default;

            JValue Clone() const { return JValue(*this); }

            std::optional<std::string> GetStringValue() const;
            std::optional<double>      GetNumberValue() const;
            std::optional<bool>        GetBooleanValue() const;

            std::string GetName() const { return name_; }            
            JValueType  GetValueType() const { return value_type_; }             
            size_t      GetNumberOfChildren() const { return children_.size(); }
            const auto& GetChildren() const { return children_; }
            bool        HasProperty(const std::string& name) { return children_name_indexes_.find(name) != children_name_indexes_.end(); }

            //returns false if this parent object is not an array
            //takes an rvalue since JValue can only be moved (or Clone()d and then moved)
            bool AddArrayChild(JValue&& value);

            //returns true if no property with this name already exists, false otherwise
            //takes an rvalue since JValue can only be moved (or Clone()d and then moved)
            bool AddObjectChild(std::string name, JValue&& value);

            //returns true if the element exists, false otherwise
            bool RemoveChild(size_t index);
            //returns true if the element exists, false otherwise
            bool RemoveChild(const std::string& name);

            //get the nth child element, either object property or array item
            const JValue& operator[](size_t index) const { return children_.at(index); }
            //get an object property by name
            const JValue& operator[](const std::string& name) const { return this[children_name_indexes_.at(name)]; }

            const auto begin() const { return children_.begin(); }
            const auto end()   const { return children_.end(); }

        private:
            JValue(JValueType value_type, std::variant<std::string, double, bool> value);

            //a copy is recursive and copies all children; it's very expensive
            //force the user to use the clone() method so that it doesn't happen by accident
            JValue(const JValue& other);
            JValue& operator=(const JValue& other);

            std::vector<JValue> CopyChildren(const JValue& other);

            std::string name_;
            JValueType value_type_;
            std::vector<JValue> children_;
            std::unordered_map<std::string, size_t> children_name_indexes_;
            std::variant<std::string, double, bool> value_;
    };
}

JValue.cc

namespace jsonreadercpp 
{    
    JValue::JValue(JValueType value_type, 
                    std::variant<std::string, double, bool> value) 
            : name_(""), 
                value_type_(value_type),
                children_(), 
                children_name_indexes_(),
                value_(value)
    {
    }

    JValue::JValue(const JValue& other)
        : name_(other.name_),
            value_type_(other.value_type_),
            children_(std::move(CopyChildren(other))),
            children_name_indexes_(other.children_name_indexes_),
            value_(other.value_)
    {
    }

    JValue& JValue::operator=(const JValue& other)
    {
        name_ = other.name_;
        value_type_ = other.value_type_;
        children_ = std::move(CopyChildren(other));
        children_name_indexes_ = other.children_name_indexes_;
        value_ = other.value_;
    }

    std::vector<JValue> JValue::CopyChildren(const JValue& other)
    {
        std::vector<JValue> copy;
        copy.reserve(other.children_.size());

        for (const auto& child : other.children_)
        {
            copy.emplace_back(child.Clone());
        }

        return copy;
    }

    std::optional<string> JValue::GetStringValue() const
    {
        switch (this->value_type_)
        {
            case JValueType::String:  return {std::get<string>(this->value_) };
            case JValueType::Number:  return {std::to_string(std::get<double>(this->value_))};
            case JValueType::Boolean: return {std::get<bool>(this->value_) ? "true" : "false"};
            case JValueType::Null:    return {""};

            default: return {};     
        }
    }

    std::optional<double> JValue::GetNumberValue() const
    {
        if (this->value_type_ == JValueType::Number)
            return { std::get<double>(this->value_) };

        return {};             
    }

    std::optional<bool> JValue::GetBooleanValue() const
    {
        if (this->value_type_ == JValueType::Boolean)
            return { std::get<bool>(this->value_) };
        return {}; 
    }


    bool JValue::AddArrayChild(JValue&& value)
    {
        bool success = false;

        if (this->value_type_ == JValueType::Array)
        {
            success = true;
            //move() here is superfluous, but leaving it just in case `value` changes to a regular value in the future
            this->children_.emplace_back(std::move(value)); 
        }

        return success;
    }


    bool JValue::AddObjectChild(std::string name, JValue&& value)
    {
        bool success = false;

        if (this->value_type_ == JValueType::Object && name.length() > 0 && !this->HasProperty(name))
        {
            success = true;
            value.name_ = name;
            //move() here is superfluous, but leaving it just in case `value` changes to a regular value in the future
            this->children_.emplace_back(std::move(value));
            this->children_name_indexes_[name] = this->children_.size() - 1;
        }

        return success;
    }

    bool JValue::RemoveChild(size_t index)
    {
        bool exists = false;
        if (index < this->children_.size())
        {
            exists = true;            
            string child_name = this->children_[index].name_;
            this->children_.erase(this->children_.begin() + index);

            if (this->children_name_indexes_.find(child_name) != this->children_name_indexes_.end())
                this->children_name_indexes_.erase(child_name);
        }
        return exists;
    }

    bool JValue::RemoveChild(const string& name)
    {
        bool exists = false;

        auto kvp = this->children_name_indexes_.find(name);
        if (kvp != this->children_name_indexes_.end())
        {
            exists = true;
            this->RemoveChild(kvp->second);
            this->children_name_indexes_.erase(name);
        }

        return exists;
    }
}

statemachine/ParserMachineStack.h

namespace jsonreadercpp
{
    class ParserMachineStack
    {
        public:
            void PopPush(ParserStackSymbol stack_pop, ParserStackSymbol stack_push);

            ParserStackSymbol GetTop() const { return state_machine_stack_.top(); }
            size_t GetSize() const { return state_machine_stack_.size(); }

        private:
            std::stack<ParserStackSymbol> state_machine_stack_;
    };
}

statemachine/ParserMachineStack.cc

namespace jsonreadercpp
{
    void ParserMachineStack::PopPush(ParserStackSymbol stack_pop, ParserStackSymbol stack_push)
    {
        if (stack_pop != stack_push)
        {
            if (stack_pop != ParserStackSymbol::None)
                this->state_machine_stack_.pop();

            if (stack_push != ParserStackSymbol::None)
                this->state_machine_stack_.push(stack_push);
        }
    }
}

statemachine/ParserState.h

namespace jsonreadercpp 
{ 
    class ParserState
    {
        public:
            using TransitionLookup = std::unordered_map<ParserInputSymbol, std::unordered_map<ParserStackSymbol, ParserStateTransition>>;

            ParserState(ParserStateType type, 
                        std::initializer_list<ParserStateTransition> transitions = {},
                        ParserStateTransition else_transition = {ParserInputSymbol::None, ParserStateType::Error})
                            : state_type_(type), transitions_(), else_transition_(else_transition)
            {
                for (auto& transition : transitions)
                {
                    this->AddTransition(transition);
                }
            }

            ParserStateType GetStateType() const { return state_type_; }

            const TransitionLookup&      GetTransitions()    const { return transitions_; };
            const ParserStateTransition& GetElseTransition() const { return else_transition_; };

            bool HasTransition(ParserInputSymbol input_symbol, ParserStackSymbol stack_symbol) const;

            const ParserStateTransition& GetTransition(ParserInputSymbol input_symbol, ParserStackSymbol stack_symbol) const
            {
                return transitions_.at(input_symbol).at(stack_symbol);
            }

            const ParserStateTransition& GetTransitionOrElse(ParserInputSymbol input_symbol, ParserStackSymbol stack_symbol) const
            {
                return HasTransition(input_symbol, stack_symbol) ? 
                            GetTransition(input_symbol, stack_symbol) : else_transition_;
            }

            void AddTransition(ParserStateTransition transition) { transitions_[transition.input].emplace(transition.stack_pop, transition); }

        private:
            ParserStateType state_type_;
            TransitionLookup transitions_;
            ParserStateTransition else_transition_;

    };

}

statemachine/ParserState.cc

namespace jsonreadercpp
{

    bool ParserState::HasTransition(ParserInputSymbol input_symbol, ParserStackSymbol stack_symbol) const
    {
        bool found = false;

        auto find_input = this->transitions_.find(input_symbol);
        if (find_input != this->transitions_.end())
        {
            auto& stack_map = find_input->second;
            auto find_stack = stack_map.find(stack_symbol);
            found = (find_stack != stack_map.end());
        }

        return found;
    }
}

statemachine/ParserStatesManager.h

namespace jsonreadercpp 
{
    class ParserStatesManager
    {
        public:            
            struct NextTransition
            {
                const ParserStateTransition& transition;
                bool finished_input;
                char processed_char;

                NextTransition(const ParserStateTransition& transition, bool finished_input = false, char processed_char = '\0')
                    : transition(transition), finished_input(finished_input), processed_char(processed_char) {}
            };

            ParserStatesManager() : states_(jsonreadercpp::states::CreateStatesMap()) {}

            NextTransition GetNextTransition(ParserStateType current_state_type, std::istream& input, ParserStackSymbol stack_top) const;

        private:
            std::unordered_map<ParserStateType, ParserState> states_;

    };
}

statemachine/ParserStatesManager.cc

namespace jsonreadercpp
{    
    ParserStatesManager::NextTransition ParserStatesManager::GetNextTransition(ParserStateType current_state_type, std::istream& input, ParserStackSymbol stack_top) const
    {
        //order of operations: None, None -> * | None, X -> * | X, None -> * | X, Y -> *        
        const auto& current_state_iter = states_.find(current_state_type);

        if (current_state_iter == states_.end())
        {
            std::cerr << "Unable to find state " << ParserStateTypeName(current_state_type) << "\n";
            exit(1);
        }
        else
        {        
            const auto& current_state = current_state_iter->second;

            if (current_state.HasTransition(ParserInputSymbol::None, ParserStackSymbol::None))
            {
                //None, None -> *
                return { current_state.GetTransition(ParserInputSymbol::None, ParserStackSymbol::None) };
            }
            else if (stack_top != ParserStackSymbol::None && current_state.HasTransition(ParserInputSymbol::None, stack_top))                            
            {
                //None, X -> *
                return { current_state.GetTransition(ParserInputSymbol::None, stack_top) };
            }
            else
            {
                char c = '\0';
                if (input >> c)
                {              
                    ParserInputSymbol input_symbol = jsonreadercpp::CharToInputSymbol(c);

                    //X, None -> *
                    if (current_state.HasTransition(input_symbol, ParserStackSymbol::None))
                        return { current_state.GetTransition(input_symbol, ParserStackSymbol::None), false, c };

                    //X, Y -> *
                    else if (current_state.HasTransition(input_symbol, stack_top))
                        return { current_state.GetTransition(input_symbol, stack_top), false, c };

                    else
                        return { current_state.GetElseTransition(), false, c };
                }
                else
                {
                    //no more input to read
                    //there should be no more states, but we need to return something, sooo
                    return { current_state.GetElseTransition(), true, '\0' };
                }            
            }
        }
    }
}

statemachine/ParserStateTransition.h

namespace jsonreadercpp 
{   
    struct ParserStateTransition
    {
        ParserInputSymbol input;
        ParserStateType next_state;
        ParserStackSymbol stack_pop;
        ParserStackSymbol stack_push;
        ParserCharDestination char_destination;
        ParserValueAction value_action;
        std::optional<JValueType> value_push_type;

        ParserStateTransition(ParserInputSymbol input, ParserStateType next_state) 
                              : input(input), next_state(next_state), 
                                stack_pop(ParserStackSymbol::None), stack_push(ParserStackSymbol::None),
                                char_destination(ParserCharDestination::None), 
                                value_action(ParserValueAction::None), value_push_type()
        {             
        }

        ParserStateTransition& SetStack(ParserStackSymbol stack_pop, ParserStackSymbol stack_push) 
            { this->stack_pop = stack_pop; this->stack_push = stack_push; return *this; }

        ParserStateTransition& SetCharDestination(ParserCharDestination char_destination) 
            { this->char_destination = char_destination; return *this; }

        ParserStateTransition& SetValueAction(ParserValueAction value_action, std::optional<JValueType> value_push_type) 
            { this->value_action = value_action; this->value_push_type = value_push_type; return *this; }
    };

}

statemachine/ParserValueStack.h

namespace jsonreadercpp
{
    class ParserValueStack
    {
        public:
            void AccumulateInput(char input_char, ParserCharDestination destination, ParserStateType current_state_type);
            void PushJValue(JValueType type);
            void PopJValue();
            std::optional<JValue> RemoveRootValue();

            size_t GetSize() const { return value_stack_.size(); }

        private:
            std::stack<std::pair<std::string, JValue>> value_stack_;
            std::stringstream property_name_;
            std::stringstream scalar_value_;  

            //collected unicode digits
            wchar_t unicode_code_point_ = 0;
            ParserCharDestination unicode_destination_;

            char ProcessEscapeCharInput(char input_char, ParserCharDestination destination, ParserStateType current_state_type);

            //Translate a character into the corresponding escape char,
            //i.e. 'n' to '\n', 't' to '\t', etc
            char TranslateEscapeChar(char escaped);

            //Collects hex codes into unicode_code_point_,
            //in the order it appears in the JSON string (big endian)
            void CollectUnicodeCodePoint(char input_char);
            std::string TranslatUnicodeCodePoint();
    };
}

statemachine/ParserValueStack.cc

namespace jsonreadercpp
{
    void ParserValueStack::AccumulateInput(char input_char, ParserCharDestination destination, ParserStateType current_state_type)
    {
        input_char = ProcessEscapeCharInput(input_char, destination, current_state_type);

        if (input_char != '\0')
        {          
            if (destination == ParserCharDestination::Name)
                this->property_name_ << input_char;
            else if (destination == ParserCharDestination::Value)
                this->scalar_value_ << input_char;            
        }
    }

    void ParserValueStack::PushJValue(JValueType type)
    {
        optional<JValue> new_value;

        if (type == JValueType::Array || type == JValueType::Object || type == JValueType::Null)
        {
            new_value.emplace(type);
        }
        else
        {
            string accumulated_chars = this->scalar_value_.str();

            if (type == JValueType::String)
                new_value.emplace(accumulated_chars);
            else if (type == JValueType::Number)
                new_value.emplace(std::stod(accumulated_chars));
            else if (type == JValueType::Boolean)
                new_value.emplace(accumulated_chars == "true");
        }

        //add the new value to the top of the stack
        this->value_stack_.emplace(this->property_name_.str(), std::move(new_value.value()));

        //clear the accumulated values
        this->property_name_.str("");
        this->scalar_value_.str("");
    }

    void ParserValueStack::PopJValue()
    {
        if (this->value_stack_.size() > 1) //root value?
        {
            pair<string, JValue> top_value(std::move(this->value_stack_.top()));
            this->value_stack_.pop();

            auto& parent_pair = this->value_stack_.top();            
            auto& parent_value = parent_pair.second;

            if (parent_value.GetValueType() == JValueType::Array)
            {
                parent_value.AddArrayChild(std::move(top_value.second));
            }
            else if (parent_value.GetValueType() == JValueType::Object)
            {
                parent_value.AddObjectChild(std::move(top_value.first), std::move(top_value.second));
            }
        }
    }


    optional<JValue> ParserValueStack::RemoveRootValue()
    {
        optional<JValue> result;

        if (value_stack_.size() == 1)
        {
            result.emplace(std::move(value_stack_.top().second));
            value_stack_.pop();
        }

        return result;
    }

    char ParserValueStack::ProcessEscapeCharInput(char input_char, ParserCharDestination destination, ParserStateType current_state_type)
    {
        if (current_state_type == ParserStateType::EscapeChar)
        {            
            input_char = (input_char == 'u') ? '\0' : TranslateEscapeChar(input_char);
        }
        else if (current_state_type == ParserStateType::UnicodeValue || current_state_type == ParserStateType::UnicodeProperty)
        {
            //collect unicode code point for later
            this->CollectUnicodeCodePoint(input_char);

            input_char = '\0';
            this->unicode_destination_ = destination;            
        }
        else if (this->unicode_code_point_ > 0)
        {
            //we have a previously collected unicode code point, save it now            
            if (this->unicode_destination_ == ParserCharDestination::Name)
                this->property_name_ << TranslatUnicodeCodePoint();
            else if (this->unicode_destination_ == ParserCharDestination::Value)
                this->scalar_value_ << TranslatUnicodeCodePoint();  

            this->unicode_code_point_ = 0;
        }

        return input_char;
    }

    char ParserValueStack::TranslateEscapeChar(char escaped)
    {
        switch (escaped)
        {
            case 'b': return '\b';
            case 'f': return '\f';
            case 'n': return '\n';
            case 'r': return '\r';
            case 't': return '\t';
        }

        return escaped;
    }

    void ParserValueStack::CollectUnicodeCodePoint(char input_char)
    {
        std::stringstream ss;
        ss << input_char;

        //convert the hex char to a number       
        unsigned char hex_num;
        ss >> std::hex >> hex_num;

        //each hex digit represents 1/2 a byte, so shift by 4
        this->unicode_code_point_ = (this->unicode_code_point_ << 4) | (hex_num & 0x0F);
    }

    std::string ParserValueStack::TranslatUnicodeCodePoint()
    {
        //reset the conversion state
        std::wctomb(nullptr, 0); 

        std::string utf_chars(MB_CUR_MAX, '\0');
        int num_bytes = std::wctomb(&utf_chars[0], unicode_code_point_);        

        return utf_chars;
    }
}

jsonreadercpp::states::CreateStatesMap() is defined as:

unordered_map<ParserStateType, ParserState> CreateStatesMap()
{
    ParserState states[] {
        CreateStartState(),
        CreateFinishState(),
        CreateRootObjectState(),
        CreateObjectState(),
        <etc>
    };

    unordered_map<ParserStateType, ParserState> states_map;

    for (auto& state : states)
    {
        auto state_type = state.GetStateType();
        states_map.emplace(state_type, std::move(state));
    }

    return states_map;
}   

and the CreatexxxState functions are all basically the same, here's a sample:

ParserState CreateObjectState()
{
    return ParserState(ParserStateType::Object, 
                                    {
                                        { ParserInputSymbol::Whitespace, ParserStateType::Object },

                                        ParserStateTransition(ParserInputSymbol::CloseBrace, ParserStateType::PostObject)
                                                                .SetStack(ParserStackSymbol::Object, ParserStackSymbol::None)
                                                                .SetValueAction(ParserValueAction::Pop, {}),

                                        { ParserInputSymbol::DoubleQuote, ParserStateType::PropertyString }
                                    },
                                    { ParserInputSymbol::None, ParserStateType::Error } );
}

Thanks!

Answer 1

I’m going to review your code in the order you’ve presented it in the opening post, simply because that’s the most orderly way to go about it from my perspective. I’ll mention overarching design issues along the way, and then include a section at the end collecting them all together. And I’ll be answering most, if not all, of the questions along the way. I hope that won’t be too confusing for you!

Also, do note that I can’t really give a full and comprehensive review of the entire design, because, well, that’s just too big a job for a Saturday morning. I’ll try and give some big-picture suggestions, of course. But this review won’t be anything close to “complete”.

So let’s start at the top!

main.cc

I’m so glad you included a usage example; I’ve seen far too many people post entire libraries here without a single usage example. In my opinion, the usage example is the most important part of a library’s documentation. If a library doesn’t incude good, clear, practical usage examples, I won’t even look at it.

The big question that jumps out at me from your usage example is: why have you chosen not to use the standard input pattern? For virtually every type T, if I want to read it from a stream, I do:

auto t = T{};
if (input >> t)
    // use t
else
    // handle error

So why have you chosen to do:

auto parser = JsonReader{};
auto parsed_json = parser.ParseJsonString(std::cin);

if (parsed_json.has_value())
    // retrieve value as `auto value = std::move(parsed_json).value();` then use it
else
    // handle error

instead of:

auto value = JValue{JValueType::Null}; // Could this not just be JValue{}?
if (std::cin >> value)
    // use value
else
    // handle error

That would allow me to write more generic code (as shown in the first code block above, with T).

I guess what I’m asking is:

• Is there any purpose to the JsonReader object outside of the actual parsing? Is there any purpose to it once parsing is done? (I know you use it for debug output… but that’s kind of a library development thing, not something that users of the library should use, right?)
• Is there any reason not to use the error detection/reporting built into standard streams? You use parsed_json.has_value() to check whether parsing succeded or not… doesn’t that just give the same information as bool{std::cin}? (Except the stream can give much more information, like whether parsing failed because the content was bad (badly formatted JSON) or the stream was bad (like a network disconnection). Not to metion that exceptions could (and should) be used.)

It’s not that your design using a state machine to parse JSON is wrong—quite the opposite. It just seems to me that the entire JsonReader class should be an implementation detail hidden within a standard extractor like auto operator>>(std::istream&, JValue&).

JsonReader.h && JsonReader.cc

These are both cool. I could nitpick some criticisms, but they’re really more matters of taste than things that are really wrong with your code.

• Not a fan of using .h for C++ headers. .h to me (and to most IDE’s, file managers, etc.) means a C header. In this case, I’d recommend .hh to go with .cc.
• Not a fan of default arguments, generally. They hide the complexity of a call, so that you may be invisibly constructing a half-dozen expensive objects and have no clue, and they can cause a lot of problems with overloading. In this case, I’d probably design the class with two or three constructors: one with no arguments, one that takes the two streams by ref, and maybe a third that only takes the error stream by ref. Within the class, I’d only keep pointers to the streams that default to nullptr (or maybe the error stream would default to std::cerr), and only do debug/error output if the respective pointer is non-null. There’d be no need for the debug_output_ flag then.
• Also not a fan of the using declarations at namespace scope. I mean, they’re within the implementation file, which is certainly better than being in the header. But I don’t see any benefit to using something like std::string unqualified. (Plus, you don’t actually use any of those types unqualified at namespace scope; you could just as well include those using statements within the functions, if you really, really wanted them.)
• And, not a fan of declaring/defining multiple variables on a single line (like: int line_num = 1, char_num = 0, line_char_start = 0;). Every style guide I’ve ever used forbids this.

There is a dire lack of commenting throughout your code. You don’t need to say what every single line is doing, but you should at least explain the gross outline of what’s going on. For example, JsonReader::ParseJsonString() is essentially a while loop that does all the work, followed by a short block to actually extract the result. I have puzzled that out by meticulously reading the code… but I shouldn’t have had to. There should be a comment explaining that the while loop will only exit when the input is exhausted, or there’s an error, and that value_stack must have only a single item in it at that point, and so on. And that’s just the beginning: what, for example, is ProcessState() doing to input? Is it reading a single character? Possibly several? It doesn’t even help to read ProcessState()’s code, because that just leads to another question—what is GetNextTransition() doing to input, etc..

Comments should be telling me what all the relevant state at each point is expected to be. I shouldn’t have to guess (or utter a curse, sigh, set a bookmark, then go digging through an entire library of code to find where a specific function/type is defined, only to have to read that function/type’s code, then come back to the bookmark… HOPEFULLY… though it’s just as likely I’ll be forced down a further rabbit hole, requiring another curse, sigh, bookmark, etc.). Basically, the moment I have to stop reading a function’s code and go digging elsewhere for answers, that’s when I’d put the “FAILED” stamp on a code review.

Digging a little deeper… do you really need has_error? I mean, the moment you detect an error, you log it then break out of the loop, and then ultimately just return the empty optional. Why not just return the empty optional right away and simplify the rest of the function? You also won’t need the else in the loop anymore.

Similarly, if value_stack.GetSize() > 1, you can just return right away. I know there’s a case to be made for having a single exit point for functions at the end, but when that’s being done at the expense of creating a complex spaghetti mess of flags and conditionals to worm around error states, you’d be doing yourself (and your code’s readers) a favour to relax that rule a bit, at least for errors that require bailing immediately.

result = std::move(value_stack.RemoveRootValue());

You don't need the move() here. RemoveRootValue() already returns an rvalue. Moves are automatic in C++ wherever possible. Generally, you only need to explicitly ask for a move when an object will continue to exist afterwards. So:

x = get_thing();

won’t need a move() (assuming get_thing() doesn’t return a reference), because the “thing” returned from get_thing() is a temporary that ceases to exist by the time the statement ends. Meanwhile:

// y = get_thing();
x = y;

would need a move(), because y will continue to exist after the assignment statement.

Put altogether, JsonReader::ParseJsonString could be simplified to something like this:

optional<JValue> JsonReader::ParseJsonString(std::istream& input) 
{
    // Set up the variables you need for the loop.
    auto current_state_type = ParserStateType::Start;
    // ...
    
    // Input loop.
    do
    {
        // Trace statement.
        if (debug_output_)
            // ...
        
        // Error check.
        if (current_state_type == ParserStateType::Error)
        {
            error_output_stream_ << // ...
            
            return {}; // or return std::nullopt;
        }
        
        // Actual work of loop.
        auto next_transition_container = ProcessState(input, current_state_type, value_stack, machine_stack);
        // ...
    } while (!finished_input);
    
    // Parsing done, but was it done completely?
    if (value_stack.GetSize() > 1)
    {
        error_output_stream_ << // ...
        
        return {};
    }
    
    // Now we're really done.
    return value_stack.RemoveRootValue();
}

This isn’t necessarily “correct” (meaning the original code isn’t necessarily “wrong”), but there’s a lot less cognitive burden keeping track of error states. Errors break the flow the moment they’re found, they’re not trucked around for dozens of lines through branches, loops, and other control flow.

JValue.h & JValue.cpp

This is the part of the library that I think is, by far, the most important. It’s also the part I think is the most problematic.

For any of this input stuff to be really useful, it has to read the JSON data into a C++ object that is really useful. I always tell my students that in C++, it’s all about the types: if you get the types right, everything else just falls into place. If you get them wrong….

Fundamentally, a JSON value is nothing more than a sum of:

• null
• boolean
• number
• string
• array (just a vector of JSON values)
• object (a map of string->JSON values)

This is a somewhat recursive type because arrays and objects can hold JSON values. But otherwise… that’s just:

using json_value = std::variant<
    std::monostate,
    bool,
    double,
    std::string,
    json_array,
    json_object
>

The only “tricky” part here is defining json_array and json_object before json_value.

Now, to be clear, I do NOT recommend doing that. A JSON value is a distinct type, and should not be handwaved away with a slapdash alias of a vocabulary type. You should definitely have a JSON value class. But internally, the class needs to be little more than just that variant. Really, this is all you need:

class json_value
{
    using value_type = std::variant<
        std::monostate,
        bool,
        long double,
        std::string,
        // As of C++17, vectors can use incomplete types,
        // so this is cool:
        std::vector<json_value>,
        // Maps (and hash maps) cannot use incomplete types, so we
        // can't use std::map or std::unordered_map. However, Boost's
        // containers allow incomplete types, so:
        boost::container::map<std::string, json_value>
        // You could also use boost::container::flat_map, which would
        // probably be more efficient. Or it might even be worthwhile
        // to roll your own (flat) map that accepts incomplete types
        // for the values. Or maybe you just need an "incomplete pair"
        // type. (The trick you used, with a vector for the values and
        // a map with string key->index into the vector is clever, but
        // probably overkill, and not very efficient. You could
        // probably get away with just a vector of JValue and a vector
        // of std::string, and make sure the indexes match.)
    >;

    value_type _value;
    
public:
    // You need an interface, of course, but it doesn't need to be OTT.
    // Just take a look at std::variant - it has, like, 3 or 4 member
    // functions, and maybe a half-dozen non-member functions.
};

JValue as you’ve written it already more-or-less apes std::variant… except much less efficiently. For example, if the JValue only holds a bool, you still have to pay for constructing and trucking around:

• a string (for the non-existent name; actually, I’m baffled about the existence of this member at all—you seem to duplicate the name in both _name and children_name_indexes_[name])
• a vector (for the non-existent children)
• a hash map (for the non-existent member names)
• a JValueType discriminator, even though the variant already knows that it holds a bool

And of course, you have to be careful to keep all these various moving compments in sync.

But the biggest problem is how clunky the user interface is. JValue seems so unnecessarily hard to use.

Let’s start right at the beginning. A JSON value is one of:

• null
• boolean
• number
• string
• array
• object

So what’s it like creating a JValue in each of these cases?

// null
auto v = JValue{JValueType::Null};

Ehhh. I mean… could be worse? But it also could just be: auto v = JValue{};. I mean, why not default to null anyway?

// boolean
auto v = JValue{true};
auto v = JValue{false};

Cool.

// number
auto v = JValue{6.28};

Cool… but… did you try this: v = JValue{0};?

// string
auto v = JValue{"char array"};
auto v = JValue{"std::string"s};

Cool… but… did you try this: v = JValue{"std::string_view"sv};?

All that’s left are arrays and objects… and here’s where things get tragic. If I have a vector of strings, I would like to simply be able to do:

auto v = JValue{strings};

and get a JValue that has type JValueType::Array, with all the elements being the strings from the vector. But I can’t do that. I have to do this:

auto v = JValue{JValueType::Array};

// Nope:
// std::copy(begin(strings), end(strings), std::back_inserter(v));

// Nope (AddArrayChild() wants an rvalue for some reason):
// std::for_each(begin(strings), end(strings), [&v](auto&& string) { v.AddArrayChild(string); });

// Works, but it shouldn't (I'll explain later):
// std::for_each(begin(strings), end(strings), [&v](auto&& string) { v.AddArrayChild(std::string{string}); });

std::for_each(begin(strings), end(strings), [&v](auto&& string) { v.AddArrayChild(JValue{string}); });

Given a vector of things that can be converted to JValue (std::string or double or int or bool or even JValue), why can’t I do this?:

auto v = JValue{items};

// or:
auto v = JValue(begin(items), end(items))?

Even cooler would be if the constructor were smart enough to detect if the value type of items can work with a structured binding like auto [k, v] = value_type{};, and that the type of k is convertible to std::string, and if so, create an object value:

auto items_1 = std::vector{"a"s, "b"s, "c"s};
auto items_2 = std::vector{
    std::tuple{"n0"s, 0},
    std::tuple{"n1"s, 1},
    std::tuple{"n2"s, 2}
};

auto v_1 = JValue{items_1}; // = [ "a", "b", "c" ]
auto v_2 = JValue{items_2}; // = { "n0" : 0, "n1" : 1, "n2" : 2 }

I’m not advocating for any particular interface; I’m just pointing out options. The key point I want to make is that JValue is both too difficult and too easy to construct. It’s too difficult for the reasons I mentioned above. And it’s too easy because the constructors are not marked explicit, as they almost certainly should be. There doesn’t seem to be any good reason to allow implicit conversions to JSON values.

Speaking of too difficult:

JValue Clone() const { return JValue(*this); }

When I saw this function, I was absolutely baffled at first. Normally you don’t see “clone” functions in non-polymorphic types. And the type is totally copyable—it has copy ops, and they’re even used in Clone(). Until I saw the comments further below in the code I was scratching my head about the point of this function.

Nor does the logic in the comment make much sense either. You seem to have a strange fear of copying in C++. You mention concerns about “excessive copying” in your questions, and your code has a bunch of peculiar (mis)uses of rvalue references that often don’t even do anything, and ironically, will probably make code less efficient.

Don’t treat the language like an enemy you have to fight or outsmart. And, most importantly, don’t write code that assumes users are incompetent. Yes, definitely defend against reasonable mistakes. But… when I want a copy of a value, that’s not a mistake. And if I do make a copy I didn’t need or want… well, frankly, that’s my problem, not a problem with the type being “too easy to copy”. “Accidental” copying isn’t something you should be assuming people are doing that often—the standard library doesn’t make that assumption, for example: classes like std::vector and std::list can be very expensive to copy… but you don’t see them making life more difficult for users by deleting or hiding their copy constructors.

Don’t punish competent users of your library at the expense of coders who don’t really know what they’re doing. Let the latters’ code be slow and inefficient… that way they’ll be motivated to learn from their mistakes.

I note that you discovered yourself how clunky and hard to use your type is: you’re forced to have its functions take JValue&& parameters because copying doesn’t work (not mention the need for CopyChildren() and the fact that you’re forced to manually write the copy ops). That should be a sign: if your class is even frustrating to use in its own interface… maybe it’s time to reassess.

std::optional<std::string> GetStringValue() const;
std::optional<double>      GetNumberValue() const;
std::optional<bool>        GetBooleanValue() const;

This may be more opinionated than “standard thinking”, but I think this is an abuse of std::optional.

While it may “work” for this context, this doesn’t really seem to fit the semantics of std::optional. When I see std::optional used as a return type, that says to me “this function is getting a value that might be there, but might not, and (this is the important part) that is not an error”. For example, a class that holds a person’s name might have a middle_name() function that gets the person’s middle name. But it’s perfectly acceptable for a person to have no middle name, so that function could possibly return std::optional<std::string>.

But in the case of a JSON value, it’s not perfectly cool if you ask for the string value and there’s none there. If that happens… you’ve screwed up. You failed to check for whether the value type is string or not. Optional is the wrong semantic for that. The string value is not “optional”; if the value type is string, then it MUST be there, and if the value type is not string, then it MUST NOT be there.

And in fact, std::optional is not just wrong semantically here… it actually makes code less efficient. Because if the string value is there, then it has to be copied into the optional object. That copy is completely unnecessary.

I always tell my students to look to the standard library. See what it does in cases similar to what you’re doing, and figure out why. Chances are, there are damn good reasons.

In this case, the closest analogue to JValue is a std::variant like the one I described above. So, okay, what does std::variant do? Does it return a std::optional for the alternative you ask for? No, it does not. If you ask for a type or index, and the variant doesn’t hold that alternative, it simply throws a std::bad_variant_access. If it does hold the alternative you want, you get a reference… no copying necessary. (There’s also std::get_if, that returns a pointer or nullptr, but again, no copying.)

This is what code with your current interface looks like:

if (auto&& str = val.GetStringValue(); str)
    // now we can work with *str, which is a copy of the string in val
else
    // val didn't hold a string

By contrast, suppose GetStringValue() returned a std::string const&, and throws if the value isn’t a string type:

if (val.GetValueType() == JValueType::String)
    // now we can work with val.GetStringValue(), which is a reference - no copying is done
else
    // val didn't hold a string

Not that different! However, it can be much more efficient, because it avoids copying a string.

std::string GetName() const { return name_; }

As I mentioned, I’m not really sure of the point of _name. But in any case, this accessor makes an unnecessary copy. You could return a std::string const&, and also make it noexcept.

bool JValue::AddArrayChild(JValue&& value)
{
    bool success = false;

    if (this->value_type_ == JValueType::Array)
    {
        success = true;
        //move() here is superfluous, but leaving it just in case `value` changes to a regular value in the future
        this->children_.emplace_back(std::move(value)); 
    }

    return success;
}

Couple things here.

First, the comment is wrong. std::move() is absolutely not superfluous there. If this weren’t a member function, it wouldn’t compile without the std::move(), because the copy constructor is private. (If you don’t believe me, you can try commenting out Clone() and deleting the copy ops to see for yourself.)

You see, value is an rvalue reference… but it is not an rvalue. An rvalue reference takes (ie, binds to) rvalues… but it is not an rvalue itself. The function can only be called with rvalue arguments… but within the function, the argument is an lvalue.

The easiest way to understand when something is an rvalue is to ask: “can this be used after this point?”. In that function, if you did this->children_.emplace_back(value);, could value be used again after that line? Why yes, yes it could. You could even repeat that line to add two copies of value to children_. Therefore, value is not an rvalue.

The second thing is: this function really shouldn’t be taking its argument as an rvalue reference. What’s the point? The general rule for (non-template) function parameters is:

• If the function is only using the parameter’s value, and not taking it (for example, only inspecting or viewing it), take the parameter as const&.
• If the function is taking the parameter’s value (for example, taking the value for an object’s data member, or otherwise storing it somewhere), take the parameter by value.
• (RARE! Prefer returning.) If the function is changing the parameter’s value, take it by & (non-const).

Note there’s nothing there about &&. That’s because the only time you should ever use && parameters is in special cases for optimization purposes. (The rule is different for function templates, where && is a forwarding reference. But that’s not relevant here.)

Finally… for what this function actually does, it’s sure complicated. That’s because of all the acrobatics you do with the success flag. Is that really necessary? Why not:

bool JValue::AddArrayChild(JValue value)
{
    if (value_type_ == JValueType::Array)
    {
        children_.emplace_back(std::move(value));
        return true;
    }

    return false;
}

Or even better, in my opinion:

bool JValue::AddArrayChild(JValue value)
{
    if (value_type_ != JValueType::Array)
        return false;

    children_.emplace_back(std::move(value));
    return true;
}

That seems to require the least amount of cognitive overhead. Once the check is done at the top of the function, you know that everything’s kosher from there on out; you don’t need to be thinking, “okay, at this line, are we in a state where we’re dealing with an error or failure or not?” or keeping track of scopes or anything.

All of the same comments apply to JValue::AddObjectChild(), with one extra issue: exception safety. Consider the meat of the function:

value.name_ = name;
// If the above throws an exception, no problem.

this->children_.emplace_back(std::move(value));
// If the above throws an exception, also no problem.

this->children_name_indexes_[name] = this->children_.size() - 1;
// But what if *this* throws? Now you have an element in children_ that
// is unaccounted for in children_name_indexes_. Your object is broken.

This is the kind of headache that sometimes arises when you try to spread your class’s invariants across multiple data members. You should always aim for at least the strong exception guarantee: the function will either succeed, or, if it fails (particularly with an exception), it will have no (meaningful) effect.

In this case, a potential fix is to wrap the last line in a try block that pops the back off the children_ vector in the catch (and then rethrows), or to use some kind of “on fail” mechanism that does the same.

//returns true if the element exists, false otherwise
bool RemoveChild(size_t index);
//returns true if the element exists, false otherwise
bool RemoveChild(const std::string& name);

I’m really not a fan of this kind of interface. If you try to remove an index or name that doesn’t exist, that’s not just an “oh, well, it happens” kind of thing… you have screwed up. Something is seriously wrong your code and its logic. You should have that fact thrown in your face, so that you know and can fix it. It shouldn’t be something you can just ignore, especially by default.

This same idea applies to the add functions as well, and to the accessors. I would call this class’s interface bad, because if I screw up, the class simply… covers that up. Mistakes vanish into return values that can just be ignored, and disappear into the æther. That’s not good; that’s very, very bad.

I think a good interface is one that doesn’t reward lazy or sloppy coding. Quite the opposite, I think a good interface is one that rewards good practices, and punishes stupidity mercilessly. If you do something dumb, then the program should straight-up crash. It should do so loudly and dramatically. It should produce an error message, a core dump, and play a fart sound through the speakers.

What am I talking about? Well, for example, I think the remove functions should look like this:

auto JValue::RemoveChild(std::size_t index) -> void
{
    children_name_indexes_.erase(
        std::find_if(begin(children_name_indexes), end(children_name_indexes),
            [index](auto&& item)
            {
                return std::get<1>(item) == index;
            }));
    
    children_.erase(children_.begin() + index);
}

auto JValue::RemoveChild(std::string const& name) -> void
{
    auto const i = children_name_indexes_.find(name);
    
    children_.erase(children_.begin() + std::get<1>(*i));
    children_name_indexes_.erase(i);
}

“But Indi!” you say, “if you try removing an out-of-bounds index or a non-existent name with that code, that’s UB!” Fine. Then just don’t do that.

“But–but accidents!” Okay, if you accidentally remove an out-of-bounds index or a non-existent name, that should trigger some kind of panic that you can’t miss—like a crash—that would prompt you to use your debugger, find the problem, and fix it… you should not be shipping the program to your clients with that kind of bug hidden in it. At most you could maybe add some asserts in the functions that check that the index/name is valid… but those should disappear in release mode. I don’t want to pay for checks that should never fail, and no well-written code should ever cause those checks to fail.

If you decide you want to write code that potentially allows for non-existent indexes or names, then you should pay for it:

// q is some potentially non-existent index/name

// index:
if (q >= 0 and q < val.GetNumberOfChildren())
    val.RemoveChild(q);
else
    // handle the error case

// name:
if (val.HasProperty(q))
    val.RemoveChild(q);
else
    // handle the error case

But when I know for sure the name/index is valid, I don’t want to pay for those unnecessary checks. I just want to do val.RemoveChild(q).

As I mentioned, this same kind of thinking applies to the add functions as well. Adding a child to a value that isn’t an array isn’t an “oops” that should just be shrugged off. That’s a sign of a serious logic error in your code. AddArrayChild() should straight-up throw or terminate, or crash due to UB if you try to add a child to a non-array value. Personally, I’d recommend just making it UB, at least in release mode, so that programmers that don’t screw up don’t pay for the checks.

Whew, I think that’s it for JValue. That was a lot, but like I said, I think JValue is the most critical part of your code. If JValue is done right, everything else becomes simple.

I’m going to skip down to ParserValueStack, because, honestly, all of the state machine stuff looks fine to me. My opinion is that it’s over-engineered for parsing JSON—JSON is a pretty simplistic format, after all—but that doesn’t make it “wrong” or “bad”.

ParserValueStack.h & ParserValueStack.cc

There are some problems with this class, mostly related to Unicode, and unnecessary copying of strings.

std::stringstream property_name_;
std::stringstream scalar_value_;  

I’m not sure why you need string streams for these. All you do with them is append characters and strings, both tasks that std::string handles just fine. You don’t actually do anything that requires a stream.

Worse, using string streams mean you end up having to make a bunch of unnecessary copies. Check out what’s going on in PushJValue:

void ParserValueStack::PushJValue(JValueType type)
{
    optional<JValue> new_value;

    if (type == JValueType::Array || type == JValueType::Object || type == JValueType::Null)
    {
        new_value.emplace(type);
    }
    else
    {
        string accumulated_chars = this->scalar_value_.str();
        // This makes a copy of the string in scalar_value_.

        if (type == JValueType::String)
            new_value.emplace(accumulated_chars);
            // This makes *ANOTHER* copy of the same string.
        else if (type == JValueType::Number)
            new_value.emplace(std::stod(accumulated_chars));
            // This is okay.
        else if (type == JValueType::Boolean)
            new_value.emplace(accumulated_chars == "true");
            // This is okay.
    }

    //add the new value to the top of the stack
    this->value_stack_.emplace(this->property_name_.str(), std::move(new_value.value()));
    // This makes a copy of the string in property_name_.

    //clear the accumulated values
    this->property_name_.str("");
    this->scalar_value_.str("");
}

As long as you’re using string streams, copies are unavoidable (C++20 fixes this).

Instead, suppose property_name_ and scalar_value_ were strings. All the places you use operator<<, just use operator+= instead. And PushJValue() could become:

void ParserValueStack::PushJValue(JValueType type)
{
    optional<JValue> new_value;

    if (type == JValueType::Array || type == JValueType::Object || type == JValueType::Null)
    {
        new_value.emplace(type);
    }
    else
    {
        if (type == JValueType::String)
            new_value.emplace(std::move(scalar_value_));
            // No copying, just a move. This is fine because
            // scalar_value_ is never used again (until it's
            // reset).
        else if (type == JValueType::Number)
            new_value.emplace(std::stod(scalar_value_));
            // This is okay.
        else if (type == JValueType::Boolean)
            new_value.emplace(scalar_value_ == "true");
            // This is okay.
    }

    //add the new value to the top of the stack
    this->value_stack_.emplace(std::move(this->property_name_), std::move(new_value.value()));
    // No copying, just a move. Also fine because property_name_ is
    // not used again.

    //clear the accumulated values
    this->property_name_ = std::string{};
    this->scalar_value_ = std::string{};
    // These variables may or may not have been moved from
    // (property_value_ is definitely moved-from, scalar_value_ might
    // be). Either way, we can reset them this way safely.
}

Another benefit from doing it this way is that it makes exception safety easier. This function still isn’t entirely exception safe (if value_stack_.emplace() throws, scalar_value_ might be moved-from, which would be bad). But it’s definitely closer. Those assignments at the end are totally no-fail.

Before I get into the Unicode stuff, there’s one more function I want to call attention to:

void ParserValueStack::CollectUnicodeCodePoint(char input_char)
{
    std::stringstream ss;
    ss << input_char;

    //convert the hex char to a number       
    unsigned char hex_num;
    ss >> std::hex >> hex_num;

    //each hex digit represents 1/2 a byte, so shift by 4
    this->unicode_code_point_ = (this->unicode_code_point_ << 4) | (hex_num & 0x0F);
}

This function is exceptionally inefficient for what it does. Consider: you have a character input_char that is one of 0–9, a–f, or A–F (we’re assuming it MUST be one of those, because the function does no error checking), and all you want to do is convert that to the value 0–9 (if it’s a digit) or 10–15 (if it’s a letter). For that, you construct a stream (which is a whole lot of baggage) that does both input and output (more baggage) that constructs a string internally, and then you use the stream’s conversion operator.

Not only is that wildly expensive for the task… it might not even work. If the global locale isn’t what you think it is, you could get weird results. What you should do is ss.imbue(std::locale::classic()); before doing anything else.

All you want to do is a simple, non-locale-aware conversion. std::from_chars() is actually built for that kind of thing, with JSON specifically in mind. Unfortunately, it won’t help much if you’re working one character at a time.

For the digits, things are easy: the standard guarantees that digits are contiguous. So you can just do input_char - '0'. For the letters, which are not guaranteed to be contiguous… things are trickier, but not difficult. You have options. You could create a static constexpr lookup table. You could static_assert that the letters are contiguous, then simply do input_char - 'A' or input_char - 'a'. Whatever floats your boat.

You might end up with something like this:

void ParserValueStack::CollectUnicodeCodePoint(char input_char)
{
    auto hex_num = 0uL; // we'll worry about the right type for this later

    if (input_char <= '0' and input_char >= '9')
    {
        hex_num = input_char - '0'; // might need a static_cast, but we'll worry about that later
    }
    else
    {
        static constexpr auto hex_map = std::array<std::tuple<char, unsigned char>, 12>{
            {'a', 10},
            {'b', 11},
            // ...
            {'A', 10},
            {'B', 11},
            // ...
        };

        for (auto const [c, v] : hex_map)
            if (c == input_char)
                hex_num = v;
    }

    unicode_code_point_ = (unicode_code_point_ << 4) | (hex_num & 0x0FuL);
}

No heap allocations, and even if the lookup is required, the entire lookup table can fit in a single cache line (it’s 24 bytes), and the lookup can possibly be vectorized.

But now we come to the Unicode problem, and… well, Unicode in C++ is a fucking quagmire.

The first problem is that you seem to assume that wchar_t is a Unicode type. It’s not. In fact, I believe on Windows it can’t even hold a complete Unicode value (though it can on Linux). Basically, wchar_t is a mistake. Never use it. Forget it even exists.

That extends to everything associated with wchar_t, like std::wctomb(). That function does not do what you think it does; it does not convert from UTF-32 to UTF-8 (or maybe it does? it depends).

The type you should use for unicode_code_point_ is not wchar_t, it’s char32_t.

But that’s only half the issue.

You see, you seem to be assuming that a std::string holds UTF-8 (judging by the variable name utf_chars). Not so, unfortunately. That’s not the case on Windows (as far as I know; I don’t mess with Windows anymore).

Okay, but whatever, right? Whatever the encoding of std::string, you just use c32rtomb() and that’s that, right?

Yes… but no. I mean, yes, it is correct that you can convert UTF-32 to std::string bytes using c32rtomb(). The problem is that your code doesn’t take into account that std::string’s encoding might be state-dependent. Because of that, the generated code should be be correct, but fugly.

I think it will be easier to illustrate the problem than try to explain it. Let’s assume that you changed unicode_code_point_ to be char32_t, and rewrite TranslatUnicodeCodePoint() accordingly:

std::string ParserValueStack::TranslatUnicodeCodePoint()
{
    auto state = std::mbstate_t{};
    auto chars = std::string(MB_CUR_MAX, char{});

    auto num_bytes = std::c32rtomb(chars.data(), unicode_code_point_, &state);
    // should check that num_bytes != -1, but whatever
    chars.resize(num_bytes);

    return chars;
}

This is basically the same as what you currently have, just using std::c32rtomb() instead of std::wctomb().

The issue is that you’re doing the conversion one character at a time. This isn’t a problem at all if std::string’s encoding isn’t state-dependent. But if it is, things still “work”… just not very well.

For example, suppose the encoding of std::string is an old-school JIS (Japanese ASCII) encoding, and the input is ABC\u30A2\u30A3\u30A4DEF (“ABCアイウDEF”). What you’d like to get is “ABC{shift-out}123{shift-in}DEF”—the “1” there is shifted to “ア”, “2” there is shifted to “イ”, and the “3” there is shifted to “ウ”. If you just ditch the shift state, then TranslatUnicodeCodePoint() might convert “\u30A2” to “{shift-out}1{shift-in}”. Then, next, “\u30A3” becomes “{shift-out}2{shift-in}”, giving you “ABC{shift-out}1{shift-in}{shift-out}2{shift-in}”… and “\u30A4” becomes “{shift-out}3{shift-in}”, giving you “ABC{shift-out}1{shift-in}{shift-out}2{shift-in}{shift-out}3{shift-in}”.

You can probably fix this by building the string up out of char32_t values as a std::u32string, then converting the whole thing to a std::string in one shot, rather than character by character. But frankly, I don’t know if it’s worth caring about. It’s 2020, and Unicode is the way of future; shifted encodings are probably ancient history now.

So, all good?

Well, no. (See? Quagmire.)

The problem is that JSON only allows for 16-bit Unicode values. So how does one handle code points outside of the 16-bit range? By using surrogates. For example pile-of-poo (💩) has hex code 1F4A9. To encode that in a JSON string, you’d need to do "\uD83D\uDCA9".

See the problem? Your code works with individual characters and assumes the characters are complete. It will read \uD83D, try to convert that with std::c32rtomb(), which will fail, because that’s not a valid code point. The next read of \uDCA9 will similarly fail.

So maybe you should try using char16_t values and std::string16_t? No, that will only shift the problem. Now you could handle "\uD83D\uDCA9" correctly, but not "💩".

Quagmire.

What’s the best solution here? 🤷🏼

I can only offer you some suggestions:

• Don’t work character-by-character. That’s almost never the right thing to do when dealing with Unicode, or any character encoding other than trivial ASCII stuff. (“Character” doesn’t even make sense in most contexts.)
• Watch out for surrogates. Once you’ve collected a potential Unicode code point, check to see if it matches 0b1101'10??'????'???? or 0b1101'11??'????'????. If so, there should be a matching pair to go along with it. If there’s not a matching partner, you have an invalid Unicode sequence (you can also get those from other misuses of Unicode escapes). You should decide how to handle those.

What a huge mess Unicode created, eh?

Questions

Okay, that should probably do it for the code review… now for the questions.

• Is this good, clean, idiomatic C++17?

  Sure, looks okay to me.

• Is the project layout and class structure what you would expect?

  I could mostly find what I was looking for without too much digging, so yes.

• I have some header-only classes, is that okay?

  Definitely. There is actually good reason to try to make the entire library header-only (which is something that wouldn’t be too hard to achieve in your case). There’s not really anything to be gained from implementation files, except faster compilation, and less recompiling when details are tweaked.

• Are things named right?

  I won’t say no.

• Is there any code in headers which should be in implementation files, or vice versa?

  🤷🏼 “Should be” according to whose rules or what requirements?

  There are reasons to prefer an entirely header-only library, but there are also cons. For something as simple as a JSON library, it could probably be completely header-only with no real cons.

• Are rvalues/lvalues, references, const, auto, and other C++ features being used correctly?

  No. There seems to be some confusion about lvalues and rvalues, and references thereof, but I don’t see any real const issues. Same for auto.

  The one major feature I think isn’t being used correctly has to do with the disabling of copying in what is (or should be) a regular value type.

• I wrote Java-esq getters and setters for private members - is this the idiomatic C++ way for accessors/mutators?

  Sure. I mean, if you need a getter and setter for a private member, it’s worth asking whether it should be private at all. If it won’t cause the invariants to break, you might as well make it public. But if it needs to be private, and its needs setters and getters, then so be it.

• Performance - any issues I've overlooked?

  I haven’t run any tests, but I am highly suspicious of the overall performance of the design. Parsing a JSON string character by character—updating a state machine with every one—seems like the most painfully slow and tedious way to do it.

  Indeed, the idea of a literal state machine to parse JSON seems like overengineering to me. It seems to me that if you wrote a JSON parser in the simplest and most straightforward way, you’d end up with a logical PDA that uses the function call stack as the stack. And it would probably be orders of magnitude faster, because it’s not literally pushing and popping states from a dynamically-allocated stack, but merely calling and returning from functions.

• Any excessive copying or other issues?

  Generally no.

• I'm not using pointers anywhere - all memory is either on the stack or managed by STL classes (like vector), is that okay?

  Pointers are not just about memory. They’re also useful for representing optional stuff—in some ways better than std::optional.

  This doesn’t come up in your design, but consider a function that takes an optional parameter, like, say, a function that joins a sequence of strings with an optional delimiter. So the function could be like: std::string join(std::vector<std::string> items, std::optional<std::string> delimiter). If you already have the delimiter string lying around, or you read it from somewhere, you have to copy it into an std::optional to use it with this function. By contrast, if the function was: std::string join(std::vector<std::string> items, std::string const* delimiter), you could simply point to the delimiter string. Or pass nullptr if you don’t want one. No copies are needed.

  It seems like you’ve misunderstood advice about modern C++. It’s not “no raw pointers”. It’s “no owning raw pointers”. Non-owning pointers are still okay (mostly). It’s actually more complicated than that, but basically, raw pointers do have their uses. You don’t seem to have come across any of those uses in your code, and that’s fine.

Summary

All in all, I’d say this is good code.

It may be a bit much (and, by extension, a bit slow) for parsing something as simple as JSON, but the neat thing about your design is that’s not really a fatal shortcoming. You can basically take all of the guts of your library, simply replace the various states, and have a perfectly functional parser for any format… including ones much more complex than JSON.

The one weak link is the JValue class, which should be the centrepiece of your library, but instead feels kludgy and difficult to work with. If that was made more user-friendly, it would really help make the parsing facilities more attractive. (And it would probably make them easier to write, too.)

And, of course, there’s the whole Unicode mess… but that’s not really your fault. All of the complexity comes from the poor support in C++, and the fact that crap like UTF-16 even exists. Fixing that could be a project all on its own.

My suggestion for a possible next step would be to extract all of the JSON-specific stuff out of the automaton code, so that the machine itself is completely abstract, and could be used for any purpose—not just parsing, either. I think you’ve got a good idea here, and good code structure, so you have a solid base to build from.

Addendums

Writing an extractor

If you’re not worrying about throwing custom exceptions—which complicates things a lot—or different character types or any of that other fun stuff that makes IOstreams a nightmare, writing a custom extractor is actually pretty simple. You’ve already done 95% of the work already, in fact!

IOstreams recognizes two different types of “error”: “bad” and “fail”.

“Bad” means the stream is broken. Like, the hard-drive failed while reading a file, or the network disconnected while downloading data; that kind of thing. Or something else went wonky in the internals of the stream (like the buffer was a null pointer, and so on). It’s considered an unrecoverable error (because even if you can recreate, reopen, or reconnect the stream, you generally won’t be where you were before—you’d have to start parsing all over).

“Fail” is what you’re looking for; it means that the attempted extraction failed. For example, attempting to extract an int from “xyz” will fail. In your case, attempting to extract JSON data from malformed JSON would fail.

To set the fail bit, you just do:

stream.setstate(std::ios_base::failbit);

Note that depending on the stream’s setup, this might throw an exception. That’s not a problem—if it happens, it means the user specifically asked for it to happen—but be aware of it for exception safety reasons.

So your extractor could be as simple as:

auto operator>>(std::istream& in, JValue& val) -> std::istream&
{
    // You can check that the stream is good before bothering to try
    // any parsing. It's not necessary, of course; you already handle
    // stream errors within the state machine.
    if (in)
    {
        JsonReader parser;
        if (auto parsed_json = parser.ParseJsonString(in); parsed_json)
            // A neat side effect of parsing into a temporary and then
            // moving into the out parameter is that you automatically
            // get the strong exception guarantee. If any exceptions
            // are thrown anywhere, val is not touched.
            val = std::move(*parsed_json);
        else
            in.setstate(std::ios_base::failbit);
    }

    return in;
}

That’s pretty much it.

Now, since I got on the subject of IOstreams, I do have a couple of suggestions for making your library play a little nicer with IOstreams….

To do your input, you read a character at a time from the stream. That’s fine… but… the mechanism you choose to use is operator>>(char).

The reason this is a problem is because operator>> is a formatted input function. As you probably already noticed, that means it skips whitespace… which is why you had to use input >> std::noskipws; in JsonReader::ParseJsonString() (which you then fail to revert, which could annoy users when they find their stream suddenly no longer ignoring whitespace after reading JSON).

Since you handle all your whitespace manually, you’re better off using an unformatted input function. istream::get() is what the doctor ordered.

In ParserStatesManager::GetNextTransition(), just replace:

else
{
    char c = '\0';
    if (input >> c)
    {

with:

else
{
    char c = '\0';
    if (input.get(c))
    {

and you’re golden. Now you can remove the input >> std::noskipws; line in JsonReader::ParseJsonString(), and not have to worry about keeping track of the stream’s whitespace-skipping state.

Answer 2

Bug:

const JValue& operator[](const std::string& name) const 
{ 
     return this[children_name_indexes_.at(name)]; 
}

This won't do what you want. This is a plain UB nonsense. It should've been:

const JValue& operator[](const std::string& name) const 
{ 
     return (*this)[children_name_indexes_.at(name)];
     // or
     // return operator[](children_name_indexes_.at(name)); 
}

About auto usage:

auto stack_symbol = (machine_stack.GetSize() > 0) ? machine_stack.GetTop() : ParserStackSymbol::None;   

auto next_transition_container = states_manager_.GetNextTransition(current_state_type, input, stack_symbol);
const auto& next_transition = next_transition_container.transition;

All of these should be named types instead of auto. Abusing auto makes harder to reason about types one works with.

It is helpful to use auto for user-doesn't-care types like incomprehensible iterators or in template cases when one doesn't know the type at all. Usually, it is fine to use for-loop though for(auto&& elem : some_vector_or_map) as it is normally clear what type that is.

Error Reporting Mechanism:

JsonReader(std::ostream& error_output_stream = std::cerr, 
           std::ostream& debug_output_stream = std::cout, bool debug_output = false);

It is fine to use some tools for debugging but users shouldn't be bothered with debug/error output streams. Throw an exception that describes what's the problem. Nobody likes exceptions but this the best way to deal with this problem. At most add an option to obtain the reported error without any throws.

If you write it for a specific library then use their logger class instead of std::cout or std::ostream. As these aren't good for logging in general. If any multithreaded code does multi-threaded printing you will end up with corrupted logs and std::ostream doesn't have any suitable API to deal with it.

Main Issue: API

boost utilizes their property_tree for parsing/storing json.

In this property tree one can easily access value stored even in grand-grand-children.

  int a = boost_aptree.get<int>("a.long.path.to.the.value");

In your case it would have to be something like:

  int a = (int)aparsedJson["a"]["long"]["path"]["to"]["the"]["value"].GetNumberValue().value();

It is not even so horrible when you write it knowing the path but what about when you want to forward the path to some other place?

Also if one wants to make it safe from exceptions and obtain in optional form in boost one would have to write just get_optional instead of get and in your case it would be a nightmare of a bunch of lines.

Same is true for setting values and not just getting them.

I believe boost also utilizes some smart lookup mechanism separate from the tree structure while in yours each node has its own lookup (the unordered_map) and it is only one level deep - which is totally inefficient as when there are several child-levels to the json. It has to apply a string lookup for each level of depth.

Summary:

Overall I agree with @indi answer, I just added a couple of points he missed or I felt he didn't address enough. Overall, the code is written well in a clean way.