Re: Few cosmetic suggestions for commit 16828d5c (Fast Alter TableAdd Column...)

On Fri, Jun 15, 2018 at 12:54 AM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
>
> On 06/14/2018 02:01 PM, Alvaro Herrera wrote:
>>
>> On 2018-Jun-14, Amit Kapila wrote:
>>
>>> On Sun, Jun 3, 2018 at 5:08 AM, Amit Kapila <amit.kapila16@gmail.com>
>>> wrote:
>>>>
>>>> 2.
>>>> +/*
>>>> + * Structure used to represent value to be used when the attribute is
>>>> not
>>>> + * present at all in a tuple, i.e. when the column was created after
>>>> the
>>>> tuple
>>>> + */
>>>> +
>>>> +typedef struct attrMissing
>>>> +{
>>>> +   bool        ammissingPresent;   /* true if non-NULL missing value
>>>> exists
>>>> */
>>>> +   Datum       ammissing;      /* value when attribute is missing */
>>>> +} AttrMissing;
>>>> +
..
>>
>> We used to use prefixes for common struct members names to help
>> disambiguate across members that would otherwise have identical names in
>> different structs.  Our convention was to use _ as a separator.  This
>> convention has been partially lost, but seems we can use it to good
>> effect here, by renaming ammissingPresent to am_present and ammissing to
>> am_missing (I would go as far as suggesting am_default or am_substitute
>> or something like that).
>
> am_present and am_value perhaps? I'm not dogmatic about it.
>

+1.  Attached patch changed the names as per suggestion.

>
>
>
>> BTW I think "the result stored" is correct English.
>>
>
> Yes, it certainly is.
>

Okay.

How about attached?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com