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>).
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.
Thanks Michal
On 08/26/2013 02:48 PM, 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>).
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.
Thanks Michal
I defined the same alias twice in the last patch. New patch is attached.
Michal
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.
On Mon, 2013-08-26 at 15:40 +0200, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 09:28:12AM -0400, Simo Sorce wrote:
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.
docbook or doxygen?
Doxygen, sorry.
Simo.
On Mon, Aug 26, 2013 at 09:55:37AM -0400, Simo Sorce wrote:
On Mon, 2013-08-26 at 15:40 +0200, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 09:28:12AM -0400, Simo Sorce wrote:
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.
docbook or doxygen?
Doxygen, sorry.
Simo.
We already do for public interfaces. For private interfaces, it's more of a nice to have in my opinion. But why not, if we're adding a comment anyway, we can also format it nicely..
On 08/26/2013 04:09 PM, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 09:55:37AM -0400, Simo Sorce wrote:
On Mon, 2013-08-26 at 15:40 +0200, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 09:28:12AM -0400, Simo Sorce wrote:
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.
docbook or doxygen?
Doxygen, sorry.
Simo.
We already do for public interfaces. For private interfaces, it's more of a nice to have in my opinion. But why not, if we're adding a comment anyway, we can also format it nicely..
I think we should comment our internal API a bit more, but doxygen is IMO overkill for this purpose, but I am not strictly against it.
Michal
On 08/26/2013 03:28 PM, Simo Sorce wrote:
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). */
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 SAFEALIGN_ASSIGN_INT64 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).
HTH, Simo.
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:
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). */
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 SAFEALIGN_ASSIGN_INT64 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..
On 08/26/2013 04:07 PM, Jakub Hrozek wrote:
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:
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). */
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 SAFEALIGN_ASSIGN_INT64 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..
New patch is attached.
Michal
On Mon, Aug 26, 2013 at 05:28:48PM +0200, Michal Židek wrote:
On 08/26/2013 04:07 PM, Jakub Hrozek wrote:
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:
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). */
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 SAFEALIGN_ASSIGN_INT64 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..
New patch is attached.
Michal
I like the new names.
ACK.
On Wed, Sep 04, 2013 at 05:55:40PM +0200, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 05:28:48PM +0200, Michal Židek wrote:
On 08/26/2013 04:07 PM, Jakub Hrozek wrote:
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:
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). */
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 SAFEALIGN_ASSIGN_INT64 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..
New patch is attached.
Michal
I like the new names.
ACK.
Pushed to master.
sssd-devel@lists.fedorahosted.org