4 ms·

No need to be adversarial, we're talking about coding styles. I actually sometimes write very short helper functions that are actually meant to improve readabi

by simias 8y ago

No need to be adversarial, we're talking about coding styles. I actually sometimes write very short helper functions that are actually meant to improve readability by abstracting some details and making the code more self-documenting. For instance I have this code in my current application (a device driver): static unsigned get_machine_id(struct manager *mgr) { u32 conf = readl(mgr->regs + OFF_MACHINE_ID); return conf & 0xff; } It's called twice in the entire code. By you rule I ought to manually inline it but I think it'd actually make the code worse, not better. It's always a matter of balance, writing good code is an art. >Only noobs avoid copy&paste at all costs Only noobs rely on rules of thumb indiscriminately.

m0zg 8y ago
Where did I say anything about relying on anything "indiscriminately"? Avoiding repetition at all cost is definitely a mistake. Avoiding repetition where it's justifiable could be a sensible choice. Though in your case I'd just say: u32 machine_id = readl(mgr->regs + OFF_MACHINE_ID) & 0xff; and be done with it.
- Gibbon1 8y ago
  #define machine_id() (readl(mgr->regs + OFF_MACHINE_ID) & 0xff)
  - atq2119 8y ago
    Please no. C-style macros are too leaky.
- simias 8y ago
  >Where did I say anything about relying on anything "indiscriminately"? Well maybe I read a little too much into your comment but I interpreted it as "if you have small functions called only a small number of times in your code then you're a noob". I attempted to provide a counter-example. I'm sure that you didn't mean it quite as literally but there probably are a bunch of young, inexperienced coders browsing this thread and I think these rules of thumb can be pretty counterproductive when they reach impressionable minds. Then you end up with people cargo-culting weird ad-hoc rules without even understanding the reasoning behind them. >Avoiding repetition at all cost is definitely a mistake. Avoiding repetition where it's justifiable could be a sensible choice. I completely agree on that.
unwind 8y ago
Of course that should be const uint32_t conf = read1(mgr->regs + OFF_MACHINE_ID); and the 'mgr' argument should be const:ed, too. :) That makes it much more clear that this kind helper is just reading something out, and not poking stuff around. Always const all the things. Have you even bothered checking if your compiler inlines this? It sure looks like a very likely candidate.
- simias 8y ago
  I agree that the pointer to mgr should've been const, that's a mistake on my part. On the other hand "const"ing the local integer is a bit overkill IMO (I do think that languages like Rust are right to default to const, but doing the same thing in C adds a lot of noise IMO).
  - cmroanirgo 8y ago
    From the article (and follows my experience): "Const parameters and const functions are helpful in avoiding side effect related bugs, but the functions are still susceptible to changes in the global execution environment. Trying to make more parameters and functions const is a good exercise, and often ends in casting it away in frustration at some point. That frustration is usually due to finding all sorts of places that state could be modified that weren't immediately obvious -- places for bugs to breed. "
Gibbon1 8y ago
So your example isn't germane at all to the comment you replied to. The reason is that the function you quoted is an API call, it crosses a boundary in the code base. As such it separates two concerns. And actually reduces dependency. (Whatever function calls get_machine_id() shouldn't know anything about how that bit of sausage get made.) OP is talking about about tying disparate parts of code together via a function call. This creates dependency. Creating dependencies without a really good reason is BAD.
Novashi 8y ago
>It's always a matter of balance Depends on your language and environment, too. I'm more comfortable inlining C# web code than unsafe device driver code. But remember when you inline, you can comment around code and use more expressive variable names too. It's the same logical abstraction and similar levels of readability, just accomplished with different language features.

by simias 8y ago

m0zg 8y ago

Where did I say anything about relying on anything "indiscriminately"? Avoiding repetition at all cost is definitely a mistake. Avoiding repetition where it's justifiable could be a sensible choice. Though in your case I'd just say: u32 machine_id = readl(mgr->regs + OFF_MACHINE_ID) & 0xff; and be done with it.

Gibbon1 8y ago
#define machine_id() (readl(mgr->regs + OFF_MACHINE_ID) & 0xff)
- atq2119 8y ago
  Please no. C-style macros are too leaky.
simias 8y ago
>Where did I say anything about relying on anything "indiscriminately"? Well maybe I read a little too much into your comment but I interpreted it as "if you have small functions called only a small number of times in your code then you're a noob". I attempted to provide a counter-example. I'm sure that you didn't mean it quite as literally but there probably are a bunch of young, inexperienced coders browsing this thread and I think these rules of thumb can be pretty counterproductive when they reach impressionable minds. Then you end up with people cargo-culting weird ad-hoc rules without even understanding the reasoning behind them. >Avoiding repetition at all cost is definitely a mistake. Avoiding repetition where it's justifiable could be a sensible choice. I completely agree on that.

unwind 8y ago

Of course that should be const uint32_t conf = read1(mgr->regs + OFF_MACHINE_ID); and the 'mgr' argument should be const:ed, too. :) That makes it much more clear that this kind helper is just reading something out, and not poking stuff around. Always const all the things. Have you even bothered checking if your compiler inlines this? It sure looks like a very likely candidate.

simias 8y ago
I agree that the pointer to mgr should've been const, that's a mistake on my part. On the other hand "const"ing the local integer is a bit overkill IMO (I do think that languages like Rust are right to default to const, but doing the same thing in C adds a lot of noise IMO).
- cmroanirgo 8y ago
  From the article (and follows my experience): "Const parameters and const functions are helpful in avoiding side effect related bugs, but the functions are still susceptible to changes in the global execution environment. Trying to make more parameters and functions const is a good exercise, and often ends in casting it away in frustration at some point. That frustration is usually due to finding all sorts of places that state could be modified that weren't immediately obvious -- places for bugs to breed. "

Gibbon1 8y ago

So your example isn't germane at all to the comment you replied to. The reason is that the function you quoted is an API call, it crosses a boundary in the code base. As such it separates two concerns. And actually reduces dependency. (Whatever function calls get_machine_id() shouldn't know anything about how that bit of sausage get made.) OP is talking about about tying disparate parts of code together via a function call. This creates dependency. Creating dependencies without a really good reason is BAD.

Novashi 8y ago

>It's always a matter of balance Depends on your language and environment, too. I'm more comfortable inlining C# web code than unsafe device driver code. But remember when you inline, you can comment around code and use more expressive variable names too. It's the same logical abstraction and similar levels of readability, just accomplished with different language features.