Live data from Hacker News

The Linux codebase has over 3k TODO comments, many from over a decade ago

todos.tickgit.com

131–140 of 301 posts

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#131
post #100

I use TODO in my code all the time as shorthand for: "This code is functional, but if you are going to do another iteration you may want to consider the following improvement or optimization." It's not at all meant to be to be like an item in a TODO list. Code would be a terrible place to keep that.

The lead programmer at my very first coding job taught me to put a string of at least three hash marks in a comment, to denote “this is a thing which is okay during development but absolutely must be changed/fixed before shipping this product”. The more essential the change/fix, the more hashes. Made it really easy to do a global search for them, and you could easily filter the results to only show the most important…

> “this is a thing which is okay during development but absolutely must be changed/fixed before shipping this product”.

These should not reach the master codebase and should thus be fixed before you merge your code into it.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#132

Earlier quoted context omitted.

Nice! Isn't this exactly assert in C++? (the following is from https://en.cppreference.com/w/cpp/error/assert ) #ifdef NDEBUG #define assert(condition) ((void)0) #else #define assert(condition) /*implementation defined*/ #endif ... assert(("There are five lights", 2 + 2 == 5)); ... test: test.cc:10: int main(): Assertion `((void)"There are five lights", 2+2==5)' failed. Take it as a token that you are doing something…

Assert's run-time. This is compile-time (i.e. all instances are guaranteed to trigger during build). C++ equivalent would be a static_assert, but it has a totally different use case like this: static_assert(sizeof(StructUsedInSomeBinaryProtocol) == SomeConstantExpectedByTheOtherSide, " ")

We used to do static asserts with a preprocessor macro that typedefed an array with size expressed as: (your condition) ? 1 : -1. Point being, static asserts have the use case you want them to have; I see no reason to not have a TODO macro that expands to static_assert(false, "...").

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#133
post #14
post #2

In my opinion, any comment prefixed by TODO, XXX, or the like should not survive past the review stage. Either fix the problem immediately or accept that it's going to be wonky forever. Edit: Amending here to avoid replying to ten different threads individually. 1. Long-term improvements should be managed by a ticket tracking system. The code is not the correct place to manage that. 2. Most of the disagreements below…

Serious question as someone who drops XXX occasionally: There are all sort of places where you want to note "there's probably a better way to do this" or "maybe look at this edge case" or whatever. Not all of those are important enough to address in the next review/sprint/whatever...why isn't it better to leave them in and keep that tribal knowledge around rather that delete them and loose that insight? Maybe a bette…

I prefer to minimize tribal knowledge of all forms. This does incentivize deletion over "it might be somewhere/in someone's mind", but it also incentivizes moving the knowledge out of the jungle and into something longer lived and searchable in a broader context. How much I prefer the minimization depends on the size/stage of the company, though... At early startups for instance, most tribal knowledge will take care of itself by being forgotten -- and no harm either since the subject matter it applied to is no longer relevant, that was x months ago and nearly everything has changed.

For larger companies, though, it's really nice to be able to pull up documentation (glorious if you can even use a public Google search instead of some intranet lookup or README) about something, instead of having to go on a safari to try and find someone who knows someone who might remember why X, find out they don't remember (or don't remember enough/don't have time for you until Later), and having to figure it out from scratch yourself like you hoped to avoid. Not having to do that for everything is such a nice experience that I'd like to encourage more of it where I can.

Coming to the specifics you highlighted, "there's probably a better way to do this", "maybe look at this edge case", to me those are topics for the code review. They can be rephrased as questions for the reviewer(s): "Do you think this could be done in a better way?", "Should we try and test this edge case or does it matter?" The answers are in the review, and possibly in follow up work items tracked by something.

Code reviews can contain a lot of useful knowledge. If your commits aren't trivially linked to a review, it's worthwhile to spend the extra 10 seconds and manually link them at the end for posterity. If you aren't doing reviews, or the reviews are crap, oh well, you asked and no one answered/cared and that's now visible. Don't leave the question in the code.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#134
post #38

Earlier quoted context omitted.

That's a terrible advice. When I worked at Google, many code reviews added more TODOs because the reviewer identified a potential source of problem but also correctly decided that fixing it right there was not the best use of developers' time. When code has potential issues, I want it to be marked with TODO which basically says "The original developer was not an idiot, but was working with limited resource and the be…

> That's a terrible advice. When I worked at Google, many code reviews added more TODOs because the reviewer identified a potential source of problem but also correctly decided that fixing it right there was not the best use of developers' time. I'm sure Google is able to adopt a working ticketing system. If you already have an issue tracker then it makes absolutely no sense to keep a separate out-of-band ticketing r…

From the point of view of being in the middle of working with some piece of code, it's the issue tracker that's out-of-band! Also, a lot of those TODO entries tend to not be good entries into issue tracker (do you want to track "add extra error checks foobar at line 213 in quux.c"), and they tend to be very localized to the area of code they're left in - which means that, unless your issue tracker supports some way of encoding coordinates in files that survives changes to those files, future people modifying a piece of code are very unlikely to see those notes if they're not left in the code itself.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#135
post #61

Earlier quoted context omitted.

> This code is functional, but if you are going to do another iteration you may want to consider the following improvement or optimization. I assign the same meaning to TODO or FIXME markers but a lot of people see this is as a measure of poor quality so instead I write regular comments of the form "this could be made better by doing X and Y".

That may work but I find those comments tend to get lost over time. TODO/XXX/FIXME support are built into IntelliJ which makes stuff like that super easy to find. If some random dev looks and decides its low quality because of those words...well I don't care because its a silly metric to judge on.

Intellj also supports custom "patterns".

"Intellj supports it" is a pretty meaningless argument anyway.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#136

TODO is vital for my development process. Sure, maybe there's better programmers who don't use or need TODO, but for me, it's a critical method for the following reasons. 1. It prevents my "flow" from being sidetracked by micro-optimizations that are probably too early to consider necessary anyway. 2. It helps me to retain my short term memory on the code I am working on. If I branched out at each TODO to implement s…

I use a macro to create an unit test for reason 1 and 2 and name tests with what to test_conditions_expectedResult and add assertTrue(false) as body. I forget to check for todos before I commit, but a failing test is harder to miss

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#137
post #13
post #2

In my opinion, any comment prefixed by TODO, XXX, or the like should not survive past the review stage. Either fix the problem immediately or accept that it's going to be wonky forever. Edit: Amending here to avoid replying to ten different threads individually. 1. Long-term improvements should be managed by a ticket tracking system. The code is not the correct place to manage that. 2. Most of the disagreements below…

I am hesitant to write this, because in general I agree, but let's say a product manager stops a feature short of completed in some sense, even though there are some aspects which would make the feature more robust, secure or optimized, the code gets released and one would need a tombstone as a visible nuisance and indicator for these aspects, wouldn't it? I mean the best place to communicate important ideas and warn…

If you've let a PM stop a feature short of something you as a dev think is rather important, you've already failed to some extent as a professional. There are ways out of such a mess, but it's better to avoid it to begin with, and if it really couldn't be helped, to have a paper trail if not only for yourself then for the benefit of others.

Coming to the project as a new hire, I'd find a "TODO[3 years ago]: maybe add a cache for X to speed things up on this flow" comment less illuminating than "We think this will probably start to fall over at some scale point because we're not using DB connections wisely (and maybe want a cache layer) but our PM didn't give us time [or 'we weren't able to stop the PM shipping before we had time'] to perf test and rework the flow that was patterned after all the other DB-conn-happy logic".

(Not that either comments are that useful -- guess why I'm reading this part of the code?)

But I'm mature enough to think, coming across such code without any of those two comments, that the latter scenario is more likely than "Argh stupid devs, stupid this-dev-in-particular who I can see with git-blame, probably never even thought of a cache! I don't even need to see if there was discussion in the review!" That sort of thought only comes with direct experience dealing with people who repeatedly show themselves as professional failures who should at least retire into management but even better should find a different career field...

Comments like "NOTICE: Don't optimize this loop or you'll introduce a timing attack!" are always welcome in my book. Though if whatever you're warning about can be enforced with a unit test, that's even better insurance against someone naively or even idiotically changing it.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#138
post #2

In my opinion, any comment prefixed by TODO, XXX, or the like should not survive past the review stage. Either fix the problem immediately or accept that it's going to be wonky forever. Edit: Amending here to avoid replying to ten different threads individually. 1. Long-term improvements should be managed by a ticket tracking system. The code is not the correct place to manage that. 2. Most of the disagreements below…

IMHO TODOS help code quality by informing the reader of the weaknesses in the code and considerations with which the code was developed.

As a result if it’s out of scope of the ticket then it’s better to have them left in the code then lower code quality by removing them.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#139
post #100

Earlier quoted context omitted.

The lead programmer at my very first coding job taught me to put a string of at least three hash marks in a comment, to denote “this is a thing which is okay during development but absolutely must be changed/fixed before shipping this product”. The more essential the change/fix, the more hashes. Made it really easy to do a global search for them, and you could easily filter the results to only show the most important…

> “this is a thing which is okay during development but absolutely must be changed/fixed before shipping this product”. These should not reach the master codebase and should thus be fixed before you merge your code into it.

I think if multiple people are working on a code base which will not ship for years, then having those can be acceptable.

Obviously if “next release” is a deploy from master in 2 weeks then it isn’t.

Re: The Linux codebase has over 3k TODO comments, many from over a decade ago

#140

Earlier quoted context omitted.

That may work but I find those comments tend to get lost over time. TODO/XXX/FIXME support are built into IntelliJ which makes stuff like that super easy to find. If some random dev looks and decides its low quality because of those words...well I don't care because its a silly metric to judge on.

Intellj also supports custom "patterns". "Intellj supports it" is a pretty meaningless argument anyway.

Yeah I know I could define custom patterns and all that...but why? "Intellj supports it" means it makes my and the people on my teams lives easier. Instead of deciding on some arbitrary other thing we just use what the tool defaults to. Hardly a meaningless argument.
Post reply on HN