[4/5] libavutil: Use an intermediate variable in AV_COPY*U
diff mbox

Message ID 1469706841-90972-4-git-send-email-martin@martin.st
State Committed
Commit 014773b66bdff4de24f384066d1a85d2a5bb6774
Headers show

Commit Message

Martin Storsjö July 28, 2016, 11:54 a.m. 
If AV_RN and AV_WN are macros with multiple individual reads and
writes, the previous version of the AV_COPYU macro would fail if
the reads and writes overlap.

This should not be any less efficient in any case, given a
sensibly optimizing compiler.
---
 libavutil/intreadwrite.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Diego Biurrun July 28, 2016, 6:59 p.m. | #1 
On Thu, Jul 28, 2016 at 02:54:00PM +0300, Martin Storsjö wrote:
> If AV_RN and AV_WN are macros with multiple individual reads and
> writes, the previous version of the AV_COPYU macro would fail if
> the reads and writes overlap.
> 
> This should not be any less efficient in any case, given a
> sensibly optimizing compiler.
> ---
>  libavutil/intreadwrite.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

OK

> --- a/libavutil/intreadwrite.h
> +++ b/libavutil/intreadwrite.h
> @@ -467,7 +467,11 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>  
> -#define AV_COPYU(n, d, s) AV_WN##n(d, AV_RN##n(s));
> +#define AV_COPYU(n, d, s)                                       \
> +    do {                                                        \
> +        uint##n##_t val = AV_RN##n(s);                          \
> +        AV_WN##n(d, val);                                       \
> +    } while (0)

Maybe add spaces around ## while you're at it.

Diego
Martin Storsjö July 31, 2016, 7:23 p.m. | #2 
On Thu, 28 Jul 2016, Diego Biurrun wrote:

> On Thu, Jul 28, 2016 at 02:54:00PM +0300, Martin Storsjö wrote:
>> If AV_RN and AV_WN are macros with multiple individual reads and
>> writes, the previous version of the AV_COPYU macro would fail if
>> the reads and writes overlap.
>> 
>> This should not be any less efficient in any case, given a
>> sensibly optimizing compiler.
>> ---
>>  libavutil/intreadwrite.h | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> OK
>
>> --- a/libavutil/intreadwrite.h
>> +++ b/libavutil/intreadwrite.h
>> @@ -467,7 +467,11 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>> 
>> -#define AV_COPYU(n, d, s) AV_WN##n(d, AV_RN##n(s));
>> +#define AV_COPYU(n, d, s)                                       \
>> +    do {                                                        \
>> +        uint##n##_t val = AV_RN##n(s);                          \
>> +        AV_WN##n(d, val);                                       \
>> +    } while (0)
>
> Maybe add spaces around ## while you're at it.

I'd rather not actually, I find code like this loses readability if you 
add too much spacing around the concatentation operators, and all of the 
occurrances in the file so far have no extra spaces.

// Martin

Patch
diff mbox 

diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
index f77fd60..cdf1ef4 100644
--- a/libavutil/intreadwrite.h
+++ b/libavutil/intreadwrite.h
@@ -467,7 +467,11 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
  * memory locations.
  */
 
-#define AV_COPYU(n, d, s) AV_WN##n(d, AV_RN##n(s));
+#define AV_COPYU(n, d, s)                                       \
+    do {                                                        \
+        uint##n##_t val = AV_RN##n(s);                          \
+        AV_WN##n(d, val);                                       \
+    } while (0)
 
 #ifndef AV_COPY16U
 #   define AV_COPY16U(d, s) AV_COPYU(16, d, s)