I've added an explanatory comment. Hopefully won't get stale.<div><br></div><div>Any other comments? I'd love to get this into the tree as we're seeing actual races in the wild due to it....</div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Tue, Aug 28, 2012 at 11:01 AM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The tests and the comment serve different purposes.<br>
<br>
The tests are just machine-checkable examples of the expected behavior<br>
written in a way amenable to being checked by the machine. The comment<br>
is written with the reader in mind, with the express purpose of<br>
communicating an understanding of what the code does; it can use the<br>
full power of natural language and other human-understandable<br>
notations (like pseudo-IR) to convey this.<br>
<br>
As for fearing that the comment will fall out of date. I think it's<br>
safe to say that the "big picture" is not going to change<br>
significantly without causing enough churn in the source code that the<br>
comment gets fixed. The current patch is a data point supporting this.<br>
You can also mitigate the problem by cross-referencing specific tests<br>
from the comment---incongruities between the test and the description<br>
will alert the reader that the comment has fallen out of date.<br>
<span class="HOEnZb"><font color="#888888"><br>
--Sean Silva<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Tue, Aug 28, 2012 at 1:18 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> On Tue, Aug 28, 2012 at 10:13 AM, Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> wrote:<br>
>><br>
>> -class CGBitFieldInfo {<br>
>> -public:<br>
>> -  /// Descriptor for a single component of a bit-field access. The entire<br>
>> -  /// bit-field is constituted of a bitwise OR of all of the individual<br>
>> -  /// components.<br>
>> -  ///<br>
>> -  /// Each component describes an accessed value, which is how the<br>
>> component<br>
>> -  /// should be transferred to/from memory, and a target placement,<br>
>> which is how<br>
>> -  /// that component fits into the constituted bit-field. The pseudo-IR<br>
>> for a<br>
>> -  /// load is:<br>
>> -  ///<br>
>> -  ///   %0 = gep %base, 0, FieldIndex<br>
>> -  ///   %1 = gep (i8*) %0, FieldByteOffset<br>
>> -  ///   %2 = (i(AccessWidth) *) %1<br>
>> -  ///   %3 = load %2, align AccessAlignment<br>
>> -  ///   %4 = shr %3, FieldBitStart<br>
>> -  ///<br>
>> -  /// and the composed bit-field is formed as the boolean OR of all<br>
>> accesses,<br>
>> -  /// masked to TargetBitWidth bits and shifted to TargetBitOffset.<br>
>><br>
>> Could you beef up the 2-liner comment you replaced this nice comment<br>
>> with to include the pseudo-IR for a bitfield access like this comment<br>
>> does? I think that's a very valuable piece of documentation to have.<br>
><br>
><br>
> Is it really a good idea to have this in the comment versus the tests? My<br>
> far is that the comment will slip out of date.<br>
</div></div></blockquote></div><br></div>