HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Parsing expressions using idiomatic C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
usingidiomaticparsingexpressions

Problem

I'm a C++ programmer using SPOJ problems to learn C(99). This is my solution to the problem CMEXPR, where the goal is to read a valid expression (+, -, *, / only) from input and write it on output with superfluous parentheses removed.

The code yields correct answer according to the online judge (got an AC). My question is whether there are any C++-isms in the code and if so, what would be a more idiomatic C way of expressing them. Basically, I don't want to write C++ code in C (just like I don't like C code written in C++).

```
#include
#include
#include
#include

//#define TESTING

#ifdef TESTING
#define TEST_ASSERT(maCond) do { if (!(maCond)) { printf("%d: %s\n", __LINE__, #maCond); exit(1); } } while (0)
#define TEST_ASSERT_MSG(maCond, maFormat, maMsg) \
do { \
if (!(maCond)) { printf("%d: %s: '" maFormat "'\n", __LINE__, #maCond, (maMsg)); exit(1); } \
} while (0)
#define IF_TESTING(...) __VA_ARGS__
#else
#define TEST_ASSERT(maCond) do {} while (0)
#define TEST_ASSERT_MSG(maCond, maFormat, maMsg) do {} while(0)
#define IF_TESTING(...)
#endif

enum ParsingContext
{
CONTEXT_FIRST_TERM
, CONTEXT_ADDITIVE_EXPRESSION
, CONTEXT_NONFIRST_TERM
};

struct ExprNode
{
char type;
struct ExprNode *child[2];
};

struct NodeMemory
{
struct ExprNode data[MAX_NODE_COUNT];
struct ExprNode *end;
};

struct ExprNode createNode(struct NodeMemory mem, char type)
{
struct ExprNode *node = mem->end++;
TEST_ASSERT(mem->end - mem->data type = type;
IF_TESTING(
node->child[0] = node->child[1] = NULL;
)
return node;
}

struct ExprNode* moveToChild(
struct NodeMemory * restrict mem
, struct ExprNode * restrict node
, size_t idxChild
, char newParentType
)
{
struct ExprNode * result = createNode(mem, newParentType);
result->child[idxChild] = node;
return result;
}

char readChar(const char restrict restrict in)
{
char c = **in;
++*in;
return c;
}

void writeChar(char restrict restrict out, cha

Solution

These comments are not really related to writing "idiomatic C", merely things I noticed in your code.

-
Your handling of commas in lists is odd:

enum ParsingContext {
    CONTEXT_FIRST_TERM
    , CONTEXT_ADDITIVE_EXPRESSION
    , CONTEXT_NONFIRST_TERM
};

struct ExprNode* moveToChild(
    struct NodeMemory * restrict mem
    , struct ExprNode * restrict node
    , size_t idxChild
    , char newParentType
    )


I have never seen things written like that and don't see any advantage
over the normal use:

enum ParsingContext {
    CONTEXT_FIRST_TERM,
    CONTEXT_ADDITIVE_EXPRESSION,
    CONTEXT_NONFIRST_TERM
};

struct ExprNode* moveToChild(struct NodeMemory * restrict mem,
                             struct ExprNode * restrict node,
                             size_t idxChild,
                             char newParentType)


-
Your macros and their invocation should allow a semicolon to be placed where
expected. Otherwise, syntax-aware editors can get confused and mess up the
indentation:

IF_TESTING(
        node->child[0] = node->child[1] = NULL;
        )
        return node;


Better to move the ; out of the bracket:

IF_TESTING((node->child[0] = node->child[1] = NULL));
    return node;


-
Your asserts that check against a list of chars could be simpler with
strchr (also I find the placement of the && here to be distracting, but I
know some people like it that way):

TEST_ASSERT_MSG(
   **in != ')'
   && **in != '+'
   && **in != '-'
   && **in != '*'
   && **in != '/'
   && **in != '\n'
   , "%c", **in
   );


Neater (?):

TEST_ASSERT_MSG(!strchr(")+-*/\n", **in), "%c", **in);


-
I always find passing double pointers to be awkward and confusing, so I
avoid it if possible. In this case it seems that you could read and write
the input/output string directly to/from stdin/out as you go, replace
readChar and writeChar with fgetc and fputc and pass
stdin/stdout around in instead of in and out (or just use the
stdin/stdout globals).

-
Note that using an assert to check for exceeding the available memory is
incorrect. This should be an explicit if and exit.

TEST_ASSERT(mem->end - mem->data <= MAX_NODE_COUNT);

Code Snippets

enum ParsingContext {
    CONTEXT_FIRST_TERM
    , CONTEXT_ADDITIVE_EXPRESSION
    , CONTEXT_NONFIRST_TERM
};

struct ExprNode* moveToChild(
    struct NodeMemory * restrict mem
    , struct ExprNode * restrict node
    , size_t idxChild
    , char newParentType
    )
enum ParsingContext {
    CONTEXT_FIRST_TERM,
    CONTEXT_ADDITIVE_EXPRESSION,
    CONTEXT_NONFIRST_TERM
};

struct ExprNode* moveToChild(struct NodeMemory * restrict mem,
                             struct ExprNode * restrict node,
                             size_t idxChild,
                             char newParentType)
IF_TESTING(
        node->child[0] = node->child[1] = NULL;
        )
        return node;
IF_TESTING((node->child[0] = node->child[1] = NULL));
    return node;
TEST_ASSERT_MSG(
   **in != ')'
   && **in != '+'
   && **in != '-'
   && **in != '*'
   && **in != '/'
   && **in != '\n'
   , "%c", **in
   );

Context

StackExchange Code Review Q#32040, answer score: 3

Revisions (0)

No revisions yet.