Hacker News Reader

Comment by jstanley | Hacker News Reader

The random idiom I got was:

> Idiom #137 Check if string contains only digits

> Set boolean b to true if string s contains only characters in range '0'..'9', false otherwise.

    char b = 0;
    for (int i = 0; i < strlen(s); i++) {
        if (! (b = (s[i] >= '0' && s[i] <= '9')))   break;
    }

I appreciate the funny assignment-and-test-and-early-break in one (although I'd hardly say it's idiomatic), but I could do without the quadratic strlen().

View on HN

That is one of the most confusing pieces of C code I have seen lately. And it fails on an empty string: I would expect it to set b to 1 for an empty string, but it sets it to 0. Of course that could easily be fixed by setting b to 1 at the top.

Also, code like this should always be put inside a function that returns a value, not just written inline. Making it a function allows simpler and more understandable code too.

The funniest part is that it is not necessary to call strlen() at all! The whole thing can be written in a single pass over the string. Here is how I would code it in C:

  int OnlyDigits( char str[] ) {
      for( int i = 0;  str[i] != '\0';  ++i ) {
          if( str[i] < '0' || str[i] > '9' ) return 0;
      }
      return 1;
  }

Try it here:

https://replit.com/@geary/OnlyDigitsC

You do realize it returns 1 for an empty string right? I mean it doesn't have any digits in it...

What about adding a check of str[0] == 0 -> return 0 Also, giving char str[] will make it char* str. Which can be null. This may cause reading a random memory location (possibly segfault or use-after-free)

edit: I get the comments but empty string still contains no digits. Given the regex would be ^[0-9]+ (+ instead of *) What I want to say is string has a numerical value or not. Empty string is NaN.

Empty string contains no non-digits. Therefore it is only digits.

“contains only digits” isn’t synonymous with “not (doesn’t have any digits)”

It is synonymous with “none of the characters is a non-digit”, which is true for the empty string.

> You do realize it returns 1 for an empty string right? I mean it doesn't have any digits in it...

Yes, and that was deliberate on my part, as it meets my expectation of what such a function should do in this edge case.

The problem statement was "Set boolean b to true if string s contains only characters in range '0'..'9', false otherwise."

To my mind, the question "is every character in the string a digit" should be equivalent to "are there any non-digits in the string" (with the answer inverted, of course).

Returning 0 (false) for the empty string makes those questions not equivalent. It makes the empty string a special case.

Of course the real problem is that the problem is under-specified. It should call out specifically what should happen for an empty string, because as illustrated here, this is something where reasonable people may disagree.

> Also giving char str[] will make it char* str. Which can be null. This may cause reading a random memory location (possibly segfault or use-after-free)

Well yes, of course. The point of my comment wasn't to write bullet-proof library-ready code, it was only to illustrate two things: code like this should always go in a function, and the entire task can be accomplished in a single pass through the string.

Thanks for keeping me on my toes!

Where did your choice of regex come from? The problem statement does not have any regex, and if you wanted to derive a regex from the problem statement you would probably need to get ^[0-9]*$ if you wanted to be correct.

I just checked godbolt [0]. gcc only calls strlen once even with -O0.

[0]:https://godbolt.org/z/j4o1915vE

For me the strlen call appears directly before loops backwards jump when set to -O0, resulting in a call every iteration as far as I can tell. However -O1 already seems to optimize it to a single call at the start of the function.

I find it every time funny that when using languages that want to give you "total control" over the execution of your code (mostly C/C++) you actually almost never know what code gets executed in the end.

It depends on the compiler, it's version, it's flags, and likely "the position of the moon".

Of course the compiler is only allowed to do transformations that the spec permits. But it's impossible for a human being to anticipate the exact outcome. It's more like: "Compiler, do something that has the same outcome as this code I show you here". The output can be than something that doesn't resemble the input even slightly!

There's obviously nothing wrong when the compiler is so smart that it sees some patterns and transforms your code into something much more efficient. Only that there's not much difference to what happens when you use a high level language. In both cases you in fact don't control the exact code that gets executed, and in both cases you rely on the smartness of your compiler to produce some efficient code, "whatever" you've written.

That's why I think it's mostly a function of the code-style how performant or efficient some language can be (to some extend of course). When you write low-level style code (even in a high level language) a smart compiler will (hopefully) create something like what you would get form writing your code in C/C++.

I think what matters is the intent. Here the intent is to compare against strlen at every turn of the loop.

When the compiler is optimizing it might very well realize that the parameter to strlen doesn't change and the output can be saved first.

If the intent is to compute the length only once, then we can save the length in a variable.

Of course; as others have pointed out; there is no need to call strlen at all in this example.

I guess that's true. Calling an expensive function effectively in the body of a loop like in this example is going to be an issue in every language.

It is difficult to optimise because you need the compiler to evaluate and prove at compile time that both the loop cannot affect the result of the function call and the function call will not affect the loop.

It's a common and easy optimisation to simply move function calls like this out of the loop.

For the cost of 1 line of code I've regularly seen 10%, 100%, 1000% speed ups.

It's actually one of the most common optimisations to do in non-compiled / "slow" languages if you know how functions are evaluated, you see that the cost of a simple getter function call can be the most expensive part of a loop.

Sure. But that's not really my point here.

It's great and sometimes even astonishing what GCC and LLVM can do.

Also it's clear that even the smartest compiler can't magically optimize any code.

My point was more about the fact that compilers for lower level languages like C/C++, exactly the two named, use the most "magic" possible and that it's therefore almost impossible to anticipate upfront how their generated code will look like. But C/C++ claim that you have the most possible control over the code. My point was that this is only true to some extend, and that you can get almost equally good generated code using a less low level language just by writing code in a style matching the usual low level languages. (Especially than you need to think about loop invariants and such like you said)!

So my point was more: The claim that you have "total control" over what happens at runtime when using a language like C/C++ is false.

Seeing this example and at the same time people discussing pages long (while using even de-compilers) given that source snippet how the generated code may or may not look like reminded me of that, like I said "funny", fact about the "total control" C/C++ gives you.

It's not an issue, of course. It's just an observation and I was reminded of it.

If you know what a compiler will optimise, might optimise and can't optimise, you can get an intuition for what the generated code will be executing, but even ASM is not "full control" and often not super useful because what you think is happening will shuffled around again by the CPU. The execution time for the same asm will vary significantly depending on architecture.

If you have a good mental model of modern CPUs, in a simple loop, you can estimate what you think the bottlneck of the function will be, either by counting the micro ops or the number of stack / heap memory reads or memory allocations, etc, to estimate what is really happening to work out how you can optimise it, otherwise you're just shooting in the dark trying random combinations of flags or code not understanding why something worked or didn't work.

At least in C/C++ that model works.

In slow languages like python/javascript/etc, doing simple operations doesn't translate down to the very low levels at ALL. Generally if you imagine the worst possible way you can think of for how something will execute in a simple loop and multiply it by 10, it might be close.

It's not unreasonable to assume the compiler will optimize it to a single call. Though I guess people who are capable of making that judgement won't need to look this idiom up on the internet.

Is there something in the C spec that allows optimizing to a single strlen call?

Absolutely. The gist of it:

"In the abstract machine, all expressions are evaluated as specified by the semantics. An actual implementation need not evaluate part of an expression if it can deduce that its value is not used and that no needed side effects are produced (including any caused by calling a function or accessing a volatile object)."

As I understand it, the strlen implementation ("calling a function") is typically going to come from another object file (at link time), so it’s not clear that when compiling this file that “calling strlen has no side effects” is information available to the compiler.

strlen is a standard function (in a hosted environment). So it must do exactly what the standard says it does, and the standard doesn't say it has side-effects. The compiler could very well use a built-in implementation of strlen, or even omit the call entirely if it had another way to deduce its would-be return value.

Object files are an implementation detail not known by the C standard.

> the standard doesn't say it has side-effects

The relevant question is "does the standard say that it does not have side-effects?" (is a pure function). @skissane's sibling comment to yours provides the explanation of how the compiler can deduce that it's a pure function.

The compiler can deduce that it is a pure function using the same logic by which it can deduce it is free to replace the call with a builtin: the function is defined in the standard. Everything @skissane said is implementation details and not particularly relevant (the compiler can do the optimization without the attribute they mentioned).

I think the standard would fall apart if you read it under the assumption that anything not explicitly forbidden can happen. Instead, you should read and find out what the side effects are (and same for undefined behavior, unspecified behavior, implementation defined behavior, etcetra).

"Accessing a volatile object, modifying an object, modifying a file, or calling a function that does any of those operations are all side effects, which are changes in the state of the execution environment." (There are more details if you care to dig in)

Of course nothing stops you or me from making extensions to the standard, but analysing things from the perspective that some implementation might extend strlen to have visible side effects goes too far into whataboutism for my taste, unless there are real world examples to make it a relevant point.

Don't know about the C spec, but this is a very popular optimisation. It's called Loop-invariant code motion.

We don't write code only for compilers, but for human readers as well. Why write code that makes a smart human wonder "Is that going to be quadratic? I'd better make sure the compiler optimizes it out!"

Smart human for you, someone obsessed with micro-optimization but lacking knowledge of compiler optimizations for me..

Writing it for human readers can be used to argue for the original implementation if you think of for(i=0;i<expr;i++) as the C idiom for "iterate expr times" and here you want expr to equal the length of the string, which is what the (obviously pure) function yields. No unnecessary variables and assignments -- no clutter. It perfectly describes the intent.

This conversation started when another developer was alarmed that the strlen() call in the for loop looked like it could be quadratic. I think we owe that dev the respect to not dismiss their concern as being "obsessed with micro-optimization but lacking knowledge of compiler optimizations".

I try to write code that is as plain and simple as possible, and make it obvious what it does and that it has no gotchas. I want my code to be understandable both for new developers, and for my future self, who will surely be less smart than I think I am today.

Here is the original code, with the loop body elided:

  for( int i = 0;  i < strlen(s);  i++ ) {
  }

It is trivial to rewrite this as:

  for( int i = 0, n = strlen(s);  i < n;  i++ ) {
  }

This is a very common idiom, and now it is perfectly clear what the code actually does. Obviously, it only calls strlen() once.

Of course, as I pointed out in another comment, if you're writing a loop that iterates over a C string, you never have to call strlen() at all! You can just use the canonical C string loop:

  for( int i = 0;  s[i] != '\0';  i++ ) {
  }

Or if you like brevity (which I like too):

  for( int i = 0;  s[i];  i++ ) {
  }

But still, it will be two passes through the string when it could be only one.

It's still asking for trouble though. Look at this minor variation: https://godbolt.org/z/ncjEejsxe

It shows what happens if you take away the compilers' ability to reason about 's': it could be a global variable, and 'foo' could be modifying it, so now the compiler has to call strlen on every iteration.

So even though 'strlen' gets optimized out in the original version, it's quite a maintenance hazard since any minor change could inadvertently change the complexity from linear to quadratic. It also doesn't get optimized in debug builds, making those far slower than necessary.

Not to mention that proper idiom for this task would be:

  int n = strspn(s,"0123456789");
  BOOL b = (s[n] == 0);

That is nice and simple, but it makes ten comparisons for each character in s, where only two are needed. Of course it would be a good approach if the set of characters you're testing against is not contiguous, unlike 0..9.

Good news then, it’s also linear time like the marginally faster but enormously grotesque for loop provided previously. I would take that clarity of intent a hundred times over squeezing a couple of comparisons out.

> it’s also linear time

You raise an interesting point. It got me thinking about how big-O notation has failed us in some ways: it teaches us to ignore constant factors.

In big-O, an algorithm that makes 1000 comparisons per element is no different from one that makes a single comparison per element. They are both linear time. But you can't deny that one of these will likely take 1000 times as long as the other.

Of course, like you, I favor simple and readable code over grotesque code that is hard to understand and mentally verify.

I think big-O makes sense as a kind of filter for the truly bad solutions.

When you have some big constant factor that's not good.

But when you have a big exponent that's not workable anymore, even for smallest input.

Sure, asymptotic time complexity only says anything about the asymptotic behavior. On the other hand, I have had far more cases of things blowing up due to cubic or even quadratic time complexities than I have for linear; linear is what you expect, if it takes a long time for a toy input, it'll take twice as long for a real input at double the size. Not so with the superlinear ones, your toy problem might work fine, but cubed? Ain't happening this millennium. This combined with the constant factor tending to also grow as the problem size grows really is a potent foot-gun.

Correct, but we're talking about "idiomatic" not "most efficient" solution. Input validation is usually not time-critical, and for these cases the code clearly showing the intent is much better than optimal one.

Indeed! I updated the implementation to hoist the quadratic strlen.

Reply on news.ycombinator.com