On Mon, Aug 26, 2013 at 03:57:49PM +0200, Michal Židek wrote:
> On 08/26/2013 03:28 PM, Simo Sorce wrote:
>> On Mon, 2013-08-26 at 14:48 +0200, Michal Židek wrote:
>>> about 3 months ago I renamed the SAFEALIGN macros (and added aliases
>>> for backward compatibility) from
>>> according to request in ticket
>>> But, now I think I used wrong new names for these macros, because the
>>> SAFEALIGN_BUF2VAR_<type> is not meant to only copy value from buffer
>>> variable, it can be used the other way around as well (it is basically
>>> just memcpy with bytes counting). So the original SAFEALIGN_COPY_<>
>>> IMO much better name for the macro.
>>> The second macro SAFEALIGN_VAR2BUF_<type> is used to SET the buffer at
>>> given address with specified value. This can be used if the
>>> value we copy to buffer is just expression (like 3 + 5), #DEFINEd
>>> constant or literal (so we can not get its address with &, so
>>> SAFEALIGN_COPY_<type> can not be used). The original name of this
>>> which was better than the new one. I wrongly interpreted the macros when
>>> creating the new names and realized it now when I wanted to use them.
>>> The reason why the name change was requested is most likely because
>>> might look like it should be used when setting uint32_t variable to
>>> a specific value taken from buffer. The parameter 'value' is
>>> expression of type uint32_t and not address to some uint32_t value
>>> in buffer. Another reason why people might have been confused with the
>>> old names was that it was not clear that with SAFEALIGN_SET the
>>> is not a pointer to value but the value itself. I think this was the
>>> reason why the ticket was created.
>>> So, I would prefer to change the names once again (the new ones are not
>>> used anywhere so far) to the original ones with one little difference.
>>> The SAFEALIGN_SET_<type> will be SAFEALIGN_SETMEM_<type>. It
>>> clear that we want to change memory at given address (and not set some
>>> variable of type <type>).
>> If you want to use something familiar then use 'MEMSET' just like the
>> memset(3) function. Although I am not sure that's what you wants.
>> Maybe what you really want is SAFEALIGN_ASSIGN() to mean you are
>> assigning the variable a value.
>>> With additional comments showing the expected types of all arguments,
>>> this should be clear enough and hopefully nobody will be confused in
>>> the future.
>>> Patch is attached.
>> +/* SAFEALIGN_SETMEM_INT64(void *dest, int64_t value, size_t *pctr)
>> + * Will store store given value to memory at address pointed by dest.
>> + * If pctr is not NULL, the value of *pctr will be raised by the
>> + * number of stored bytes (in this case sizeof(int64_t)). */
>> I would reword this as follow (not doesn't change just the name,
>> corrects also the language used).
>> /* SAFEALIGN_ASSIGN_INT64(void *dest, int64_t value, size_t *pctr)
>> * This macro will safely assign an int64 value to the memory
>> * location pointed by 'dest'. If the pctr pointer is not NULL, its
>> * value is incremented by sizeof(int64_t). */
> s/its value/value pointed by 'pctr'
> because it is not the pointer that is incremented. But other than that
> your version is much more readable, so thanks for correction :-)
> I do not have strong opinion on this, but doesn't
> involve the same problem as the original
> SAFEALIGN_SET_INT64 ?
> The problem was that it looked like you are setting a value
> to uint64_t variable, now it looks like you are ASSIGNing a
> value to int64_t variable (or at least the direction is not clear
> from the name). With the SETMEM it is more explicit that you are
> modifying the memory at specific address (I originally wanted to
> use MEMSET_INT64, but it looked like memsetting int64 variable as
> well, so I choose SETMEM).
I agree with Michal that we don't want to repeat the problem with the
old names. The confustion on what is being set or copied led to very
hard to find bugs..