On Tue, Feb 16, 2021 at 7:26 PM Sebastian Andrzej Siewior <bige...@linutronix.de> wrote: > > That should be part of the commit message. You can always rewind to the > commit message that introduce something and check if the commit message > made sense or ignored a detail which made it wrong (or so).
No, it shouldn't. Commit messages are used to justify changes in the past, but they shouldn't be used as a replacement for documenting the present. The reason `in_interrupt()` is removed should be in the commit message; while the reason the precondition for `cond_resched()` is fulfilled should be in the code (assuming one finds the comment necessary, see below). > So it was needed once, it is not needed anymore. That was my arguing in > v1 about. No word about general removing in_interrupt() from drivers. Not sure what you mean by this. > This is not a fix. It just removes not needed code. Also I don't think It isn't a *bug* fix, yes, but it is a fix because the removal should have happened when the code was refactored. We can call it a cleanup, if you will. > that this is a good idea to add a comment to avoid someone to backtrack > / double check something. If you rely on a comment instead of double > checking that you do is indeed correct you will rely one day on a stale > comment and commit to a bug. Sure, comments and documentation may contain bugs like anything else. That does not mean comments and documentation are useless and we should remove all of them. In fact, the kernel lacks quite a lot of documentation. On the stale point: one-liner comments contiguous to the line they talk about and function docs are not as prone to get outdated as out-of-line docs like `Documentation/`. > To give you another example: If I would have come along and replaced > GFP_ATOMIC with GFP_KERNEL would you ask for a comment? Yes, I would, because if someone thought `GFP_ATOMIC` was needed at some point, then it is likely there is something non-obvious going on. Of course, it depends on the case. For instance, in the case of our patch, having a comment is not a big deal because it is fairly clear, specially for `charlcd_write()`. And `cond_resched()` has a `might_sleep()` annotation which helps catching future bugs. So if you don't add a comment, I will still take the patch since it is good patch. > Anyway, I'm posting a patch with changes as ordered in a jiffy. It is not an order :-) i.e. don't feel pressured that you need to sign off on the comment change -- I can submit the comment on my own later on. Cheers, Miguel
