On Wed, May 22, 2019 at 1:52 PM Steven Rostedt <[email protected]> wrote: > > On Wed, 22 May 2019 11:58:10 +0200 > Miguel Ojeda <[email protected]> wrote: > > > +/* reset all but tr, trace, and overruns */ > > +static __always_inline void trace_iterator_reset(struct trace_iterator > > *iter) > > +{ > > + /* > > + * We do not simplify the start address to &iter->seq in order to let > > + * GCC 9 know that we really want to overwrite more members than > > + * just iter->seq (-Warray-bounds). > > This comment is fine for the change log, but here it is too specific. > Why does one care about GCC 9 when we are at version GCC 21? I care > more about why we are clearing the data and less about the way we are > doing it.
Since the code is not written the obvious way on purpose, the idea is to document why that is so -- otherwise the reader may wonder (and possibly re-introduce it back). Specifying when the warning started appearing tends to be clarifying, too. The commit message explains the change itself, but the comment explains why the current code is written like that. > A comment like: > > /* > * Reset the state of the trace_iterator so that it can read > * consumed data. Normally, the trace_iterator is used for > * reading the data when it is not consumed, and must retain > * state. > */ > > That is more useful than why we have the offset hack. That comment would be great in the function's description, in my opinion, and it is a great addition to have nevertheless. I re-used the existing comment for that to keep the change as minimal as possible (and nevertheless I am not qualified to write it since I have not studied the tracing code). In other words, I'm not saying there are no further improvements :-) Cheers, Miguel
