4 ms·

> For example, counter() keeps increments consistent No, not in the face of any sort of concurrency it doesn't...

smackeyacky 2y ago
It kinda does but perhaps not entirely. The increment will always end up in the right place afterwards but internally if you expect it to be +1 in the same thread you’ll sometimes be wrong
- masklinn 2y ago
  > The increment will always end up in the right place afterwards That is completely untrue.
- qzzi 2y ago
  In the right place, but maybe at the wrong time. You can expect +1 and always be right, when the value should be +10.
- OskarS 2y ago
  No, this is absolutely not true at all. Calling this function from multiple threads is undefined behaviour in C++ (unless synchronized using some other mechanism), you get NO guarantees what so ever on program behaviour. Best case scenario is that the loads and stores are interleaved, which leads to multiple threads returning the same value when calling counter(), which will guarantee crashes elsewhere in the program (the purpose of functions like these is to produce UNIQUE values, after all). But it's undefined behaviour in any case, it's just unacceptable to put in a C/C++ code base.
  - levodelellis 2y ago
    I'm the author of the article. How many comments are you going to write about how my usage is bad citing multi-threads when I said multi threading is out of scope? And how do you not understand this would be a how-to use thread locals correctly when you are dealing with multiple threads.
    - OskarS 2y ago
      I'm sorry you feel offended, but you did write an article online with a deliberately provocative title. If you do that, you need to be able to deal with criticism. And not considering concurrency when mutating globals in C/C++ is not acceptable (never mind good) practice. You can say "threads are out of scope" till your blue in the face, but if you write an article with the thesis "globals are good, actually", you have to be able to deal with people saying "they're dangerous because of thread safety". That is a legitimate criticism of your thesis. In addition, my other criticisms of your code (overflow and properly scoping statics) have nothing to do with concurrency.
      - levodelellis 2y ago
        Has anyone told you to never use atomics? Have you not heard lockless programming is hard? (I saw this recently https://wiki.libsdl.org/SDL2/CategoryAtomic https://wiki.libsdl.org/SDL2/CategoryAtomic,) Have you written multi-threaded programs? I written two large ones. The fact you're suggesting non-experts use atomic is insanity. As well as criticizing 'overflow' in an article showing minimum easy to understand code to be read by people using completely different languages
atoav 2y ago
The problem is however that the internalized advice is shortened to "don't use global variables" and not "avoid global mutable variables when using concurrency". Constant global variables can be very useful. Mutable global variables can be totally fine in a singlethreaded (e.g. typical embedded) program. I agree that you should still use them with caution, but every advice that says: "don't do $X" should come with instructions under which circumstances it is valid and under which it isn't — and how to get the intended behavior instead with e.g. message queues, locks or whatnot.
ossobuco 2y ago
> No, not in the face of any sort of concurrency it doesn't... I fail to see how does that relate to the article in question. The author provided an example for which counter() works well, he didn't claim that the same "pattern" would be good for 100% of the use cases.
- OskarS 2y ago
  There are many reasons why global variables are bad, but extremely high on the list (if not first) is concurrency issues. The fact that the author think it's acceptable in any C or C++ code base to put in code like: static int prv_counter; int counter() { return ++prv_counter; } Is insanity. Like, this is programmer malpractice. This function can only ever be called from one thread (not to mention: using `int` instead of `int64_t` is also a trivial mistake, this easily overflows). This is the kind of thing that enters a code base, works fine for long enough that everyone forgets about it, then causes horrible security issues and crashes. The idea of saying this is "good use" of a global variable is... this person should not be giving advice on good coding. If you want to do this (and you shouldn't, because global state is bad for 14 other reasons), at the very least, make it thread safe and not trivially overflowing: static std::atomic<int64_t> prv_counter { 0 }; int64_t counter() { return 1 + prv_counter.fetch_add(1, std::memory_order_relaxed); } (the 1+ is because the original author used pre-increment instead of post-increment) Like, this is not awesome, and you shouldn't do it, but it's at least not a total disaster. EDIT: actually, this is also bad, because the `prv_counter` is not really private at all. The better way to do that would be: int64_t counter() { static std::atomic<int64_t> prv_counter { 0 }; return 1 + prv_counter.fetch_add(1, std::memory_order_relaxed); } Three different serious issues in two lines of code, fun!
  - ossobuco 2y ago
    Except the author's example is single threaded, so for that specific case your implementation of counter is needlessly complex and would actually be confusing. The point is that there is no solution that works for all use cases. If you always attempt to write fully generalized code like that you'll end up with tons of unnecessary complexity. Solve the problem at hand, not some hypothetical. The author even specifies a rule that covers your case: "If you're using threads, global and static variables should be thread local. If they are not then the discussion becomes about sharing data across threads and synchronization, which is a different topic."
qzzi 2y ago
The point is obviously that the counter is centralized, and it relates to the previous example where is no concurrency. The need for synchronization when sharing data across threads is mentioned just below that.
flohofwoe 2y ago
The trend to overgeneralize is the same problem as 'global mutable state considered harmful' ;) In a well designed code base, only a very small part of the code should need to worry about concurrency and parallelism.

It kinda does but perhaps not entirely. The increment will always end up in the right place afterwards but internally if you expect it to be +1 in the same thread you’ll sometimes be wrong

masklinn 2y ago
> The increment will always end up in the right place afterwards That is completely untrue.
qzzi 2y ago
In the right place, but maybe at the wrong time. You can expect +1 and always be right, when the value should be +10.
OskarS 2y ago
No, this is absolutely not true at all. Calling this function from multiple threads is undefined behaviour in C++ (unless synchronized using some other mechanism), you get NO guarantees what so ever on program behaviour. Best case scenario is that the loads and stores are interleaved, which leads to multiple threads returning the same value when calling counter(), which will guarantee crashes elsewhere in the program (the purpose of functions like these is to produce UNIQUE values, after all). But it's undefined behaviour in any case, it's just unacceptable to put in a C/C++ code base.
- levodelellis 2y ago
  I'm the author of the article. How many comments are you going to write about how my usage is bad citing multi-threads when I said multi threading is out of scope? And how do you not understand this would be a how-to use thread locals correctly when you are dealing with multiple threads.
  - OskarS 2y ago
    I'm sorry you feel offended, but you did write an article online with a deliberately provocative title. If you do that, you need to be able to deal with criticism. And not considering concurrency when mutating globals in C/C++ is not acceptable (never mind good) practice. You can say "threads are out of scope" till your blue in the face, but if you write an article with the thesis "globals are good, actually", you have to be able to deal with people saying "they're dangerous because of thread safety". That is a legitimate criticism of your thesis. In addition, my other criticisms of your code (overflow and properly scoping statics) have nothing to do with concurrency.
    - levodelellis 2y ago
      Has anyone told you to never use atomics? Have you not heard lockless programming is hard? (I saw this recently https://wiki.libsdl.org/SDL2/CategoryAtomic https://wiki.libsdl.org/SDL2/CategoryAtomic,) Have you written multi-threaded programs? I written two large ones. The fact you're suggesting non-experts use atomic is insanity. As well as criticizing 'overflow' in an article showing minimum easy to understand code to be read by people using completely different languages

atoav 2y ago

The problem is however that the internalized advice is shortened to "don't use global variables" and not "avoid global mutable variables when using concurrency". Constant global variables can be very useful. Mutable global variables can be totally fine in a singlethreaded (e.g. typical embedded) program. I agree that you should still use them with caution, but every advice that says: "don't do $X" should come with instructions under which circumstances it is valid and under which it isn't — and how to get the intended behavior instead with e.g. message queues, locks or whatnot.

ossobuco 2y ago

> No, not in the face of any sort of concurrency it doesn't... I fail to see how does that relate to the article in question. The author provided an example for which counter() works well, he didn't claim that the same "pattern" would be good for 100% of the use cases.

OskarS 2y ago
There are many reasons why global variables are bad, but extremely high on the list (if not first) is concurrency issues. The fact that the author think it's acceptable in any C or C++ code base to put in code like: static int prv_counter; int counter() { return ++prv_counter; } Is insanity. Like, this is programmer malpractice. This function can only ever be called from one thread (not to mention: using `int` instead of `int64_t` is also a trivial mistake, this easily overflows). This is the kind of thing that enters a code base, works fine for long enough that everyone forgets about it, then causes horrible security issues and crashes. The idea of saying this is "good use" of a global variable is... this person should not be giving advice on good coding. If you want to do this (and you shouldn't, because global state is bad for 14 other reasons), at the very least, make it thread safe and not trivially overflowing: static std::atomic<int64_t> prv_counter { 0 }; int64_t counter() { return 1 + prv_counter.fetch_add(1, std::memory_order_relaxed); } (the 1+ is because the original author used pre-increment instead of post-increment) Like, this is not awesome, and you shouldn't do it, but it's at least not a total disaster. EDIT: actually, this is also bad, because the `prv_counter` is not really private at all. The better way to do that would be: int64_t counter() { static std::atomic<int64_t> prv_counter { 0 }; return 1 + prv_counter.fetch_add(1, std::memory_order_relaxed); } Three different serious issues in two lines of code, fun!
- ossobuco 2y ago
  Except the author's example is single threaded, so for that specific case your implementation of counter is needlessly complex and would actually be confusing. The point is that there is no solution that works for all use cases. If you always attempt to write fully generalized code like that you'll end up with tons of unnecessary complexity. Solve the problem at hand, not some hypothetical. The author even specifies a rule that covers your case: "If you're using threads, global and static variables should be thread local. If they are not then the discussion becomes about sharing data across threads and synchronization, which is a different topic."

qzzi 2y ago

The point is obviously that the counter is centralized, and it relates to the previous example where is no concurrency. The need for synchronization when sharing data across threads is mentioned just below that.

flohofwoe 2y ago

The trend to overgeneralize is the same problem as 'global mutable state considered harmful' ;) In a well designed code base, only a very small part of the code should need to worry about concurrency and parallelism.