Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

Long time ago I wrote C. Could anyone fill me in why the first code snippet is arg parsing the way it is?

int main(int argc, char* argv[]) {

  if (argc > 1) {

    char\* r_loc = strchr(argv[1], 'r');

    if (r_loc != NULL) {

      ptrdiff_t r_from_start = (r_loc - argv[1]);

      if (r_from_start == 1 && argv[1][0] == '-' && strlen(r_loc) == 1) {
        in_reverse = 1;
      } 

    }

  }

  ...
}

Why not

if (argc > 1 && strcmp(argv[1], "-r") == 0) {

    in_reverse = 1;
}

for example?





It doesn't even make sense to use strchr for determining the position of 'r', when the code checks that the position of '-' is at index 0.

Your solution is perfectly fine. Even if you don't have access to strchr for some reason, the original snippet is really convoluted.

You could just write (strlen(argv[1]) > 1 && argv[1][0] == '-' && argv[1][0] == 'r') if you really want to.


It could make some sense to use strchr, because in idiomatic UNIX tools, single character command line options can be clustered. But that also means that subsequent code should not be tested for a specific position.

And if you ever find yourself actually doing command line parsing, use getopt(). It handles all the corner cases reliably, and consistent with other tools.


Of course, `&&` in C is short-circuiting so it's safe without the `strlen()` too, as long as the argument is there i.e. not NULL.

Also, the use of a convoluted `if` to conditionally assign a literal boolean is a code smell (to me), I would drop the `if` and just use:

    in_reverse = argc > 0 && argv[1][0] == '-' && argv[1][1] == 'r';
if a more forward-thinking/strict check is not needed.

Your code actually has 2 bugs. The first I assume is just a typo and you meant to use [1][1] == ‘r’. The second one is that you would accept “-rblah” as well.

I suspect it was adopted from a bigger snippet that had support for parsing things like "-abc" as "-a -b -c", etc.

Not to mention the potential signed integer overflow in (*right - *left) and (*left - *right), which is undefined behavior. And even if you rely on common two's complement wraparound, the result may be wrong; for example, (INT_MAX-(-1)) should mathematically yield a positive value, but the function will produce INT_MIN, which is negative.

And then we have this "modern" way of spelling pointers, "const int* right" (note the space). In C, declaration syntax mirrors use, so it should be "const int *right", because "*right" is a "const int".

I feel too old for this shit. :(


    const int left = *(const int*)untyped_left, right = *(const int*)untyped_right;

    return in_reverse?
        (right > left) - (right < left)
      : (left > right) - (left < right);
I wonder if there is a way to actually do it with only arithmetic, without comparisons?



Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: