[cfe-commits] PATCH: Large re-working of bitfield IR-gen, and a fix for PR13691

[Sean Silva]

The tests and the comment serve different purposes.

The tests are just machine-checkable examples of the expected behavior
written in a way amenable to being checked by the machine. The comment
is written with the reader in mind, with the express purpose of
communicating an understanding of what the code does; it can use the
full power of natural language and other human-understandable
notations (like pseudo-IR) to convey this.

As for fearing that the comment will fall out of date. I think it's
safe to say that the "big picture" is not going to change
significantly without causing enough churn in the source code that the
comment gets fixed. The current patch is a data point supporting this.
You can also mitigate the problem by cross-referencing specific tests
from the comment---incongruities between the test and the description
will alert the reader that the comment has fallen out of date.

--Sean Silva

On Tue, Aug 28, 2012 at 1:18 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Tue, Aug 28, 2012 at 10:13 AM, Sean Silva <silvas at purdue.edu> wrote:
>>
>> -class CGBitFieldInfo {
>> -public:
>> -  /// Descriptor for a single component of a bit-field access. The entire
>> -  /// bit-field is constituted of a bitwise OR of all of the individual
>> -  /// components.
>> -  ///
>> -  /// Each component describes an accessed value, which is how the
>> component
>> -  /// should be transferred to/from memory, and a target placement,
>> which is how
>> -  /// that component fits into the constituted bit-field. The pseudo-IR
>> for a
>> -  /// load is:
>> -  ///
>> -  ///   %0 = gep %base, 0, FieldIndex
>> -  ///   %1 = gep (i8*) %0, FieldByteOffset
>> -  ///   %2 = (i(AccessWidth) *) %1
>> -  ///   %3 = load %2, align AccessAlignment
>> -  ///   %4 = shr %3, FieldBitStart
>> -  ///
>> -  /// and the composed bit-field is formed as the boolean OR of all
>> accesses,
>> -  /// masked to TargetBitWidth bits and shifted to TargetBitOffset.
>>
>> Could you beef up the 2-liner comment you replaced this nice comment
>> with to include the pseudo-IR for a bitfield access like this comment
>> does? I think that's a very valuable piece of documentation to have.
>
>
> Is it really a good idea to have this in the comment versus the tests? My
> far is that the comment will slip out of date.