6
\$\begingroup\$

I'd like this reviewed.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define SWITCH(S) char *_S = S; if (0)
#define CASE(S) } else if (strcmp(_S, S) == 0) {switch(1) { case 1
#define BREAK }
#define DEFAULT } else {switch(1) { case 1

int main()
{
    char buf[256];

    printf("\nString - Enter your string: ");
    scanf ("%s", buf);

    SWITCH (buf) {
        CASE ("abcdef"):
            printf ("B1!\n");
            BREAK;
        CASE ("ghijkl"):
            printf ("C1!\n");
            BREAK;
        DEFAULT:
            printf ("D1!\n");
            BREAK;
    }
}
\$\endgroup\$
1
  • 8
    \$\begingroup\$ If you want a different syntax, use a different language. Don't try to bend C to your taste by misusing the preprocessor. It will just cause problems. \$\endgroup\$ Jan 9 '13 at 15:56
7
\$\begingroup\$

switch is usually assumed to have nearly O(1). I'm suggesting to introduce additional level for parsing. It is known as lexical analysis. You parse you fixed set of cases to enum values and then switch as you want.

enum { B1, C1, D1 } token_t;

token_t lexer(const char *s)
{
    // TODO: consider hash table here
    static struct entry_s {
        const char *key;
        token_t token;
    } token_table[] = {
        { "abcdef", B1 },
        { "ghijkl", C1 },
        { NULL, D1 },
    };
    struct entry_s *p = token_table;
    for(; p->key != NULL && strcmp(p->key, s) != 0; ++p);
    return p->token;
}

int main()
{
    char buf[256];

    printf("\nString - Enter your string: ");
    scanf ("%s", buf);

    switch(lexer(buf)) {
        case B1:
            printf ("B1!\n");
            break;
        case C1:
            printf ("C1!\n");
            break;
        case D1:
            printf ("D1!\n");
            break;
    }
}

Note that your SWITCH(S) will work only in C++ since C doesn't allow variables definition in the middle of blocks. Usually such macro uses something like:

#define BEGIN_SEPARATION(S, T) { const char *_tail = (T), *_sep = (S); \
    size_t _sep_len = strlen(_token); \
    for(const char *_next = strstr(_tail); _tail != NULL; _tail = (_next != NULL) ? _next + _sep_len : NULL) {
#define SEPARATION_TOKEN() (_next == NULL ? strdup(_tail) : strndup(_tail, _next - _tail))
#define END_SEPARATION } }

And as far as I know it's common practice to wrap macro arguments (i.e _S = (S);).

\$\endgroup\$
1
  • 2
    \$\begingroup\$ The code will compile just fine in standard C, which allows variable definitions anywhere. To me, it sounds like you are following the 23 year old, obsolete C90 standard. \$\endgroup\$
    – Lundin
    Jan 11 '13 at 7:43
3
\$\begingroup\$

I do like the idea but I'm not sure to really get the benefit of your code compared to (my C is a bit rusty so the code might not be correct) :

    if (strcmp(buf,"abcdef") == 0) {
        printf ("B1!\n");
    if (strcmp(buf,"ghijkl") == 0) {
        printf ("C1!\n");
    } else {
        printf ("D1!\n");
    }

Indeed, the code doesn't really seem easier to read but it's definitly harder to check when something wrong's happening. On top of that, the way you use brackets impose the fact that we have one BREAK per CASE/DEFAULT and this limits the expressiveness of your syntax. For instance, we don't have the following cool structures possible with a real switch :

// The fall-through (1)
case 0:
case 1: do_stuff_for1_and_2(); break

// The fall-through (1)
case 0: do_stuff_for_only1();
case 1: do_stuff_for1_and_2(); break

or

// The early return
case 0 : return FOO;
case 1 : return BAR;
default: return FOO_BAR;

or

// The normal-case with no break in the default
case 0: do_stuff(); break;
default: do_nothing();

I'm not quite sure about the kind of feedback you expected so I hope this answers your question.

As a side note, I'd choose a different variable name, maybe using __COUNTER__ (I've never used it) to generate unique name if you ever plan to nest the switches.

\$\endgroup\$
2
\$\begingroup\$

As others have pointed, out never try to re-invent the C language. It makes the code less readable and more error-prone, for no obvious benefits. Function-like macros in general are incredibly error-prone and dangerous (and a pain to debug), they should be avoided.

If you for some reason need to use function-like macros, you need to make them safe, properly encapsulate them with braces and parenthesis.

In addition, doing strcmp after strcmp in sequence like this, is very slow and inefficient, growing more inefficient with each "case" you add. This is unacceptable if program speed and random access are important.

So as for code review, I'd strongly recommend to forget this whole program as quickly as possible, nothing good will come out of it.

The proper way to write an algorithm that stores unknown, initially unsorted and completely random input strings, is to use a hash table.

\$\endgroup\$
2
\$\begingroup\$

Though your code works, it doesn't handle fall-through and won't be convertible to a jump table.

Since the strings must be constants anyhow, it would make sense to just alphabetize them and use a binary search (O(log n)) instead of an else-if tree (O(n))

#define MAGIC_DATA \
    MAP(abcdef) \
    MAP(ghijkl) \
    MAP(nopqrs) \
    MAP(tuvwxy)

#define MAP(x) ENUM_##x,
enum myenums { MAGIC_DATA };
#undef MAP
#define MAP(x) #x,
const char * mystrings[] = { MAGIC_DATA };
#undef MAP
#undef MAGIC_DATA

//different than standard bsearch - not void* and returns index
long my_bsearch(const char *key, const char **base, size_t nmemb){
    for (long bot=0, top=nmemb, i=top/2; bot<=top ; i=(bot+top)/2){
        int cmp=strcmp(key,base[i]);
        if (! cmp) return i; //match found
        else if (cmp>0) bot=i+1;
        else top=i-1;
    }
    return -1;
}

Now you can use them in a switch case like this:

int main(int argc, char **argv) {
    switch (my_bsearch(argv[1], mystrings, sizeof(mystrings)/sizeof(mystrings[0]))){
        case ENUM_abcdef : printf ("A1!\n"); break;
        case ENUM_ghijkl : printf ("B1!\n"); break;
        case ENUM_nopqrs : //fallthrough
        case ENUM_tuvwxy : printf ("D1!\n"); break;
        default : printf ("*!\n");
    }

    //or you can vectorize the result
    long i = my_bsearch(argv[1], mystrings, sizeof(mystrings)/sizeof(mystrings[0]));
    printf("%c1!\n",(i==-1)?'*':"ABDD"[i]);
    return 0;
}

This example only used strings without spaces, if you have spaces in the strings its only slightly different

#define MAGIC_DATA \
    MAP(abcdef, "a b c d e f", extra) \
    MAP(ghijkl, "g h i h k l", extra, more) \
    MAP(nopqrs, "m n o p q r", more) \
    MAP(tuvwxy, "s t u v w x", lots, of, random, things)

#define MAP(x,y,...) ENUM_##x,
enum myenums { MAGIC_DATA };
#undef MAP
#define MAP(x,y,...) y,
const char * mystrings[] = { MAGIC_DATA };
#undef MAP
//do similar stuff with __VA_ARGS__ here:
#undef MAGIC_DATA
\$\endgroup\$
1
\$\begingroup\$

I'd add another vote for "don't do this", you (or another) will suffer having to reread/debug it.

I'd try something along the lines of:

char* options[] = { "abcdef", "ghijkl", /* etc */ NULL };
switch ( index_of( options, buf ) )
{
    case 0: // abcdef
       break;
    case 1: // ghijkl
       break;
    /* etc */
}

Which is just another version of ony's answer. Slightly simpler (maybe), but either version will need keeping in step.

\$\endgroup\$
0
\$\begingroup\$

With C++, you can use constexpr functions in your case statements to (effectively) switch on (certain) strings.

I believe you will need at least C++11 to do this. You might need an even newer version of C++ (not sure about that).

Here is an example:

#include  <stdio.h>

constexpr unsigned long  s2n  ( const char * s )  {  return  s[0];  }

void  ape  ( const char * s )  {
  switch  ( s2n(s) )  {
    case  s2n("bat"):  printf ( "ape bat\n" );  break;
    case  s2n("cat"):  printf ( "ape cat\n" );  break;  }  }

int  main  ()  {
  ape ( "bat" );
  ape ( "cat" );
  return  0;  }

Expected output:

ape bat
ape cat

You probably want a more sophisticated (and safer) s2n() function than the trivial one I use above.

You can fit 8 bytes into a 64 bit integer.

Some platforms may have 128 bit integers, which would allow up to 16 bytes in your strings.

For a much more sophisticated example, one that apparently uses a sophisticated 128 bit hash function, please see:
https://github.com/xroche/stringswitch

Update

Fyi: I looked into the above mentioned xroche/stringswitch source code. There is an additional C++11 feature called "user defined literals". Using this feature might result in a further improvement to the readability of the code.

https://en.cppreference.com/w/cpp/language/user_literal

\$\endgroup\$
2
  • 6
    \$\begingroup\$ Note: Post/code is tagged as C. \$\endgroup\$ Sep 8 at 1:23
  • \$\begingroup\$ @chux - I found the page via a web search and, consequently, overlooked the C tag. I was looking for a way to switch on strings. I did read the OP's question (all four words of it) to see if C++ was explicitly excluded. I did see another poster claim (probably incorrectly, as it turns out) that the OP's code would only compile in C++. For my particular project, I am willing to switch from C to C++ to be able to switch on (short) strings. \$\endgroup\$
    – mpb
    Sep 8 at 6:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.