[SSSD] [PATCH] Rename SAFEALIGN macros
Simo Sorce
simo at redhat.com
Mon Aug 26 13:28:12 UTC 2013
On Mon, 2013-08-26 at 14:48 +0200, Michal Židek wrote:
> Hi,
>
> about 3 months ago I renamed the SAFEALIGN macros (and added aliases
> for backward compatibility) from
>
> SAFEALIGN_COPY_<type>
> SAFEALIGN_SET_<type>
>
> to
>
> SAFEALIGN_BUF2VAR_<type>
> SAFEALIGN_VAR2BUF_<type>
>
> according to request in ticket
> https://fedorahosted.org/sssd/ticket/1772
>
> 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 to
> variable, it can be used the other way around as well (it is basically
> just memcpy with bytes counting). So the original SAFEALIGN_COPY_<> was
> 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 macro
> was
> SAFEALIGN_SET_<type>
> 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
> SAFEALIGN_SET_UINT32
> 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 'value'
> 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 makes more
> 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). */
HTH,
Simo.
P.S: should we start using docbook syntax for this type of comments?
I use them elsewhere and it make it possible to create documentation
quite easily.
--
Simo Sorce * Red Hat, Inc * New York
More information about the sssd-devel
mailing list