patterncppMinor
Terminal ESC Sequence Decoder
Viewed 0 times
decoderterminalescsequence
Problem
The goal of this code is to read-in a stream of terminal ESC sequences, and break them down and fill a vector of parameterized sequences for processing later on.
For example
Colors for example
This way I can always check vector [0] for the control character, then the following for it's separated parameters.
Most of the Xterm sequences are not yet supported, but their sequences are noted in the case statements for later implementation.
I use a state machine with processing levels to move along each sequences
each level should move to the next expected control character.
Just looking for some general feedback on design or if anyone notices any issues or oversights on my part.
sequence_decoder.hpp
```
#ifndef SequenceParser_H
#define SequenceParser_H
#include "safe_queue.hpp"
#include "message_queue.hpp"
#include
#include
#include
#include
// Forward Deceleration
class Session;
typedef boost::shared_ptr session_ptr;
typedef boost::weak_ptr session_weak_ptr;
/**
* @class SequenceDecoder
* @author Michael Griffin
* @date 12/20/2015
* @file sequence_decoder.hpp
* @brief Processes incoming data into a message queue.
*/
class SequenceDecoder
{
public:
SequenceDecoder(session_ptr session);
~SequenceDecoder();
/**
* @brief Queue the Pasred Data back to the Session for Display
* @return
*/
bool sessionQueue();
// {Main Execution Method}
// Validate and Decode ESC Sequences
void decodeEscSequenceData(std::string &input_string);
private:
// Handle to Session for Sending Responses to Client.
session_weak_ptr m_weak_session;
// Holds Individual Sequences
MessageQueue m_message_queue;
// Sequence Parser State
enum
{
SEQ_NORMAL = 0, // Normal Text Data
SEQ_START = 1, // Start of ESC Seq
For example
ESC[5C would be filled in the vector as:Vector[0] = C
Vector[1] = 5Colors for example
ESC[0;44;32mVector[0] = m
Vector[1] = 0
Vector[2] = 44
Vector[3] = 32This way I can always check vector [0] for the control character, then the following for it's separated parameters.
Most of the Xterm sequences are not yet supported, but their sequences are noted in the case statements for later implementation.
I use a state machine with processing levels to move along each sequences
each level should move to the next expected control character.
Just looking for some general feedback on design or if anyone notices any issues or oversights on my part.
sequence_decoder.hpp
```
#ifndef SequenceParser_H
#define SequenceParser_H
#include "safe_queue.hpp"
#include "message_queue.hpp"
#include
#include
#include
#include
// Forward Deceleration
class Session;
typedef boost::shared_ptr session_ptr;
typedef boost::weak_ptr session_weak_ptr;
/**
* @class SequenceDecoder
* @author Michael Griffin
* @date 12/20/2015
* @file sequence_decoder.hpp
* @brief Processes incoming data into a message queue.
*/
class SequenceDecoder
{
public:
SequenceDecoder(session_ptr session);
~SequenceDecoder();
/**
* @brief Queue the Pasred Data back to the Session for Display
* @return
*/
bool sessionQueue();
// {Main Execution Method}
// Validate and Decode ESC Sequences
void decodeEscSequenceData(std::string &input_string);
private:
// Handle to Session for Sending Responses to Client.
session_weak_ptr m_weak_session;
// Holds Individual Sequences
MessageQueue m_message_queue;
// Sequence Parser State
enum
{
SEQ_NORMAL = 0, // Normal Text Data
SEQ_START = 1, // Start of ESC Seq
Solution
I see a number of things that may help you improve your code. First, though, let me say that the comments are excellent and greatly ease understanding the code. Keep that up!
Simplify the parser
The parser uses large switch statements and multiple named level parsers to decode. While the code is probably reasonably efficient (most of the bulk is comments), it does make things harder to read and maintain. An alternative approach would be to move from a code-centric to a data-centric model. In particular, if your compiler supports C++11, it seems that a
Another option would be to re-examine the various
Reduce the scope of variables
There are a number of member variables that do not really need to exist. For example,
Remember to
We don't have the code for
However, there does not appear to be a matching
Simplify the parser
The parser uses large switch statements and multiple named level parsers to decode. While the code is probably reasonably efficient (most of the bulk is comments), it does make things harder to read and maintain. An alternative approach would be to move from a code-centric to a data-centric model. In particular, if your compiler supports C++11, it seems that a
std::unordered_set would be a good match. If, for some reason, your compiler does not support such constructs, you could build your own trie structure.Another option would be to re-examine the various
switch statements. For example, the 354-line processSequenceLevel1() function could be replaced with this equivalent code:void SequenceDecoder::processSequenceLevel1()
{
static const std::string valid{"@ABCDEFGHIJKLMPSTXZ`abcdefghilmnpqrstux!"};
static const std::string nextlevel("? "};
if(m_sequence == ';' || std::isdigit(m_sequence)) {
return;
}
if (std::string::npos != valid.find(m_sequence) {
m_is_sequence_completed = true;
} else if (std::string::npos != nextlevel.find(m_sequence) {
++m_sequence_level;
} else {
m_is_invalid_sequence = true;
}
}Reduce the scope of variables
There are a number of member variables that do not really need to exist. For example,
m_is_sequence could instead be a local variable within decodeEscSequenceData instead of a member value. This makes the object less complex and the code easier to understand and maintain.Remember to
unlock resourcesWe don't have the code for
session.hpp but in several points in the code we see code like this:session_ptr session = m_weak_session.lock();However, there does not appear to be a matching
unlock() call anywhere. Unless this is implicitly handled by the session object, that could be a problem.Code Snippets
void SequenceDecoder::processSequenceLevel1()
{
static const std::string valid{"@ABCDEFGHIJKLMPSTXZ`abcdefghilmnpqrstux!"};
static const std::string nextlevel("? "};
if(m_sequence == ';' || std::isdigit(m_sequence)) {
return;
}
if (std::string::npos != valid.find(m_sequence) {
m_is_sequence_completed = true;
} else if (std::string::npos != nextlevel.find(m_sequence) {
++m_sequence_level;
} else {
m_is_invalid_sequence = true;
}
}session_ptr session = m_weak_session.lock();Context
StackExchange Code Review Q#119165, answer score: 2
Revisions (0)
No revisions yet.