4 ms·
Found it funny that one of their patches reduces a function to: static int should_we_balance(struct lb_env *env) { return 1; }
by m-app 10y ago
Found it funny that one of their patches reduces a function to:
static int should_we_balance(struct lb_env *env)
{
return 1;
}
- jhoechtl 10y agocertainly inlined and very likely optimized away altogether. Better to keep it that way, with a comment. Once the problem is fully understood, there may be room to enable it again with a more meaningful heuristics.
- chris_wot 10y agoUmmm... isn't that the job of version control?
- jimm 10y agoYes, in theory. In practice, you'd not only remove this function but all of its calls. When somebody down the road realizes that they want this function back, they have to (A) realize it's in the VC history and (B) not only get back the function but all the calling points. That is so much of a pain and potentially error-prone that leaving this function in for a while with a comment might be the more practical approach.
- pklausler 10y agoOn the other hand, it may no longer be called from every place where it should be. I've found that it's better to document what was wrong with the overall approach, scrape the dead code from the source base, and move on. Barnacles like these accumulate over time otherwise.
- andrewstuart2 10y agoNot really. It's a bad idea to bury past decisions that deserve some sort of "no trespassing," "thar be dragons," or "do not feed after midnight" monument off in some historic commit in version control. The problem is that VCSs are completely undiscoverable inside the flow of walking through code to see "where's the right place to implement this awesome feature idea I had." What you might not realize as you start implementing that feature is that it's already been implemented 5 times and removed. A comment at the end of a "go to definition" chain serves as a good dead-end indicator that can save lots of overhead and wasted effort.
- odonnellryan 10y agoNo. Replacing that with a magic number wouldn't be good. Maybe it could be a constant instead, but this is most likely good practice.
- chris_wot 10y agoUh?