Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c > #93887 > unrolled thread

Generic read/write function - casting vs union

Back to article view | Back to comp.lang.c

Contents

  Generic read/write function - casting vs union Noob <root@127.0.0.1> - 2016-09-22 11:33 +0200
    Re: Generic read/write function - casting vs union Richard Heathfield <rjh@cpax.org.uk> - 2016-09-22 11:07 +0100
      Re: Generic read/write function - casting vs union Noob <root@127.0.0.1> - 2016-09-22 12:36 +0200
        Re: Generic read/write function - casting vs union Richard Heathfield <rjh@cpax.org.uk> - 2016-09-22 14:07 +0100
      Re: Generic read/write function - casting vs union Keith Thompson <kst-u@mib.org> - 2016-09-22 11:48 -0700
      Re: Generic read/write function - casting vs union raltbos@xs4all.nl (Richard Bos) - 2016-09-23 19:42 +0000
    Re: Generic read/write function - casting vs union BartC <bc@freeuk.com> - 2016-09-22 13:11 +0100
      Re: Generic read/write function - casting vs union Noob <root@127.0.0.1> - 2016-09-22 14:46 +0200
        Re: Generic read/write function - casting vs union David Brown <david.brown@hesbynett.no> - 2016-09-22 15:10 +0200
          Re: Generic read/write function - casting vs union Noob <root@127.0.0.1> - 2016-09-22 15:36 +0200
            Re: Generic read/write function - casting vs union Malcolm McLean <malcolm.mclean5@btinternet.com> - 2016-09-22 19:28 -0700
            Re: Generic read/write function - casting vs union David Brown <david.brown@hesbynett.no> - 2016-09-23 09:59 +0200
        Re: Generic read/write function - casting vs union Barry Schwarz <schwarzb@dqel.com> - 2016-09-22 20:11 -0700
        Re: Generic read/write function - casting vs union Richard Damon <Richard@Damon-Family.org> - 2016-09-23 11:13 -0400
    Re: Generic read/write function - casting vs union Tim Rentsch <txr@alumni.caltech.edu> - 2016-09-22 08:35 -0700
      Re: Generic read/write function - casting vs union Noob <root@127.0.0.1> - 2016-09-25 23:17 +0200
      Re: Generic read/write function - casting vs union Noob <root@127.0.0.1> - 2016-09-26 14:54 +0200
        Re: Generic read/write function - casting vs union boon <root@localhost.localdomain> - 2016-09-26 19:47 +0200
        Re: Generic read/write function - casting vs union Ben Bacarisse <ben.usenet@bsb.me.uk> - 2016-09-26 19:23 +0100
        Re: Generic read/write function - casting vs union Tim Rentsch <txr@alumni.caltech.edu> - 2016-09-27 13:10 -0700
        Re: Generic read/write function - casting vs union David Thompson <dave.thompson2@verizon.net> - 2016-10-16 18:08 -0400
    Re: Generic read/write function - casting vs union Anton Shepelev <anton.txt@gmail.com> - 2016-10-17 23:40 +0300
    Re: Generic read/write function - casting vs union albert@cherry.spenarnc.xs4all.nl (Albert van der Horst) - 2016-12-11 15:20 +0100
      Re: Generic read/write function - casting vs union supercat@casperkitty.com - 2016-12-11 09:38 -0800

Page 1 of 2  [1] 2  Next page →

#93887 — Generic read/write function - casting vs union

Hello everyone,

I'm required to implement a read() function and a write() function.
(The prototype of these functions is not under my control.)

  void  read(void *dev, void *buf, int len);
  void write(void *dev, const void *buf, int len);

There is a lot of code in common between the two implementations:

read:
COMMON PROLOG
do the actual read
COMMON EPILOG

write:
COMMON PROLOG
do the actual write
COMMON EPILOG

Therefore, to avoid code duplication, I implemented a generic
access() function:

void access(void *dev, void *buf, int len, int dir);
COMMON PROLOG
if (dir == READ)  do the actual read
if (dir == WRITE) do the actual write
COMMON EPILOG

then I implemented read() and write() as wrappers around access()

read  = access(dev, buf, len, READ);
write = access(dev, (void *)buf, len, WRITE);

As you can see, I had to cast away the constness of the write buffer,
because my generic function does change buf, only in the read case.
But I think there is no undefined behavior, because I do not modify
the const buf. Is that correct?


Some people frowned upon this cast, and suggested a union.

union buf { void *in; const void *out; };
void access(void *dev, union buf *buf, int len, int dir)
COMMON PROLOG
if (dir == READ) do the actual read on buf->in
if (dir == WRITE) do the actual write on buf->out
COMMON EPILOG

That does solve the issue "casting away const is bad", but it
seems not very idiomatic (to me). Do people consider this a
better solution than casting away const?


Something I didn't mention is that the access function actually
updates buf in the EPILOG.

  buf->in += len;

Is it *100%* guaranteed that incrementing buf->in is strictly
equivalent to incrementing buf->out ?

In other words, is this assert always true, even on DS9k?

  temp = buf->out;
  buf->in += len;
  assert(buf->in == temp + len);


Regards.

[toc] | [next] | [standalone]

#93888

On 22/09/16 10:33, Noob wrote:
> Hello everyone,
>
> I'm required to implement a read() function and a write() function.
> (The prototype of these functions is not under my control.)
>
>   void  read(void *dev, void *buf, int len);
>   void write(void *dev, const void *buf, int len);
>
> There is a lot of code in common between the two implementations:
>
> read:
> COMMON PROLOG
> do the actual read
> COMMON EPILOG
>
> write:
> COMMON PROLOG
> do the actual write
> COMMON EPILOG
>
> Therefore, to avoid code duplication, I implemented a generic
> access() function:
>
> void access(void *dev, void *buf, int len, int dir);
> COMMON PROLOG
> if (dir == READ)  do the actual read
> if (dir == WRITE) do the actual write
> COMMON EPILOG
>
> then I implemented read() and write() as wrappers around access()
>
> read  = access(dev, buf, len, READ);
> write = access(dev, (void *)buf, len, WRITE);
>
> As you can see, I had to cast away the constness of the write buffer,
> because my generic function does change buf, only in the read case.
> But I think there is no undefined behavior, because I do not modify
> the const buf. Is that correct?

Yes. Beware, however, of the name access(), as it is a common extension. 
(In fact, I'm pretty sure it's in POSIX.)

> Some people frowned upon this cast, and suggested a union.
>
> union buf { void *in; const void *out; };
> void access(void *dev, union buf *buf, int len, int dir)
> COMMON PROLOG
> if (dir == READ) do the actual read on buf->in
> if (dir == WRITE) do the actual write on buf->out
> COMMON EPILOG
>
> That does solve the issue "casting away const is bad", but it
> seems not very idiomatic (to me). Do people consider this a
> better solution than casting away const?

On this occasion, no. I'm not a fan of casting away const (or indeed 
casts in general), but in this case it's the lesser of two evils.

>
>
> Something I didn't mention is that the access function actually
> updates buf in the EPILOG.
>
>   buf->in += len;
>
> Is it *100%* guaranteed that incrementing buf->in is strictly
> equivalent to incrementing buf->out ?

You can't do pointer arithmetic on void *. :-)

But you can assign to an unsigned char *, and increment that instead.

-- 
Richard Heathfield
Email: rjh at cpax dot org dot uk
"Usenet is a strange place" - dmr 29 July 1999
Sig line 4 vacant - apply within

[toc] | [prev] | [next] | [standalone]

#93889

On 22/09/2016 12:07, Richard Heathfield wrote:

> On 22/09/16 10:33, Noob wrote:
> 
>> As you can see, I had to cast away the constness of the write buffer,
>> because my generic function does change buf, only in the read case.
>> But I think there is no undefined behavior, because I do not modify
>> the const buf. Is that correct?
> 
> Yes. Beware, however, of the name access(), as it is a common extension. 
> (In fact, I'm pretty sure it's in POSIX.)

The actual name is raw_access. (In any case, thanks for pointing it out.)

>> Something I didn't mention is that the access function actually
>> updates buf in the EPILOG.
>>
>>   buf->in += len;
>>
>> Is it *100%* guaranteed that incrementing buf->in is strictly
>> equivalent to incrementing buf->out ?
> 
> You can't do pointer arithmetic on void *. :-)

Right. Assume union buf { u8 *in; const u8 *out; }; instead
(with a suitable definition for u8 in scope).

> But you can assign to an unsigned char *, and increment that instead.

And it is 100% safe, because u8 * and const u8 * are "compatible" ?

Regards.

[toc] | [prev] | [next] | [standalone]

#93894

On 22/09/16 11:36, Noob wrote:
> On 22/09/2016 12:07, Richard Heathfield wrote:
>
>> On 22/09/16 10:33, Noob wrote:
>>
<snip>

>>> Something I didn't mention is that the access function actually
>>> updates buf in the EPILOG.
>>>
>>>   buf->in += len;
>>>
>>> Is it *100%* guaranteed that incrementing buf->in is strictly
>>> equivalent to incrementing buf->out ?
>>
>> You can't do pointer arithmetic on void *. :-)
>
> Right. Assume union buf { u8 *in; const u8 *out; }; instead
> (with a suitable definition for u8 in scope).
>
>> But you can assign to an unsigned char *, and increment that instead.
>
> And it is 100% safe, because u8 * and const u8 * are "compatible" ?

The pointer arithmetic is safe, yes.

-- 
Richard Heathfield
Email: rjh at cpax dot org dot uk
"Usenet is a strange place" - dmr 29 July 1999
Sig line 4 vacant - apply within

[toc] | [prev] | [next] | [standalone]

#93910

Richard Heathfield <rjh@cpax.org.uk> writes:
> On 22/09/16 10:33, Noob wrote:
[...]
>> Something I didn't mention is that the access function actually
>> updates buf in the EPILOG.
>>
>>   buf->in += len;
>>
>> Is it *100%* guaranteed that incrementing buf->in is strictly
>> equivalent to incrementing buf->out ?
>
> You can't do pointer arithmetic on void *. :-)
>
> But you can assign to an unsigned char *, and increment that instead.

Note that gcc permits pointer arithmetic on void* as an extension.
I don't recommend using that extension, but be aware that gcc will
not warn about such pointer arithmetic by default.  You can use
-pedantic or -pedantic-errors to disable the warning and get the
required diagnostics.

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
Working, but not speaking, for JetHead Development, Inc.
"We must do something.  This is something.  Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"

[toc] | [prev] | [next] | [standalone]

#93966

Richard Heathfield <rjh@cpax.org.uk> wrote:

> On 22/09/16 10:33, Noob wrote:

> >   void  read(void *dev, void *buf, int len);
> >   void write(void *dev, const void *buf, int len);

> > Therefore, to avoid code duplication, I implemented a generic
> > access() function:

> Yes. Beware, however, of the name access(), as it is a common extension. 
> (In fact, I'm pretty sure it's in POSIX.)

So are read() and write()...

Richard

[toc] | [prev] | [next] | [standalone]

#93890

On 22/09/2016 10:33, Noob wrote:

> I'm required to implement a read() function and a write() function.
> (The prototype of these functions is not under my control.)
>
>   void  read(void *dev, void *buf, int len);
>   void write(void *dev, const void *buf, int len);
>
> There is a lot of code in common between the two implementations:
>
> read:
> COMMON PROLOG
> do the actual read
> COMMON EPILOG
>
> write:
> COMMON PROLOG
> do the actual write
> COMMON EPILOG
>
> Therefore, to avoid code duplication, I implemented a generic
> access() function:
>
> void access(void *dev, void *buf, int len, int dir);
> COMMON PROLOG
> if (dir == READ)  do the actual read
> if (dir == WRITE) do the actual write
> COMMON EPILOG

Why not just create Prolog() and Epilog() functions that can be called 
from read() and write()? Or is there too much local context to be passed?

Then you don't have the rather unsatisfactory situation of two distinct 
operations ending up being handled by the same code before having to be 
disentangled again.

[I think I've been kill-filed by this OP but I'm posting anyway.]

-- 
Bartc

[toc] | [prev] | [next] | [standalone]

#93893

On 22/09/2016 14:11, BartC wrote:

> On 22/09/2016 10:33, Noob wrote:
> 
>> I'm required to implement a read() function and a write() function.
>> (The prototype of these functions is not under my control.)
>>
>>   void  read(void *dev, void *buf, int len);
>>   void write(void *dev, const void *buf, int len);
>>
>> There is a lot of code in common between the two implementations:
>>
>> read:
>> COMMON PROLOG
>> do the actual read
>> COMMON EPILOG
>>
>> write:
>> COMMON PROLOG
>> do the actual write
>> COMMON EPILOG
>>
>> Therefore, to avoid code duplication, I implemented a generic
>> access() function:
>>
>> void access(void *dev, void *buf, int len, int dir);
>> COMMON PROLOG
>> if (dir == READ)  do the actual read
>> if (dir == WRITE) do the actual write
>> COMMON EPILOG
> 
> Why not just create Prolog() and Epilog() functions that can be called 
> from read() and write()? Or is there too much local context to be passed?
> 
> Then you don't have the rather unsatisfactory situation of two distinct 
> operations ending up being handled by the same code before having to be 
> disentangled again.
> 
> [I think I've been kill-filed by this OP but I'm posting anyway.]

Smirk. Apparently, you're not KFed on this setup ;-)

As you allude to, there is some context between PROLOG and EPILOG.

PROLOG does several "setup" operations, which must be "torn down"
in the reverse order in EPILOG, with proper error-handling, meaning
we cannot tear down a resource that has not been set up.

PROLOG:
  alloc #1
  if (failure) goto L1;
  alloc #2
  if (failure) goto L2;
  alloc #3
  if (failure) goto L3;

EPILOG:
  tear_down(#3);
L3:
  tear_down(#2);
L2:
  tear_down(#1);
L1:
  return err;

Also I'm not a fan of separating PROLOG and EPILOG, because they
are obviously closely related, and it doesn't make sense to call
one without the other. But it's interesting to have other points
of view.

Regards.

[toc] | [prev] | [next] | [standalone]

#93895

On 22/09/16 14:46, Noob wrote:

> 
> As you allude to, there is some context between PROLOG and EPILOG.
> 
> PROLOG does several "setup" operations, which must be "torn down"
> in the reverse order in EPILOG, with proper error-handling, meaning
> we cannot tear down a resource that has not been set up.
> 
> PROLOG:
>   alloc #1
>   if (failure) goto L1;
>   alloc #2
>   if (failure) goto L2;
>   alloc #3
>   if (failure) goto L3;
> 
> EPILOG:
>   tear_down(#3);
> L3:
>   tear_down(#2);
> L2:
>   tear_down(#1);
> L1:
>   return err;
> 
> Also I'm not a fan of separating PROLOG and EPILOG, because they
> are obviously closely related, and it doesn't make sense to call
> one without the other. But it's interesting to have other points
> of view.
> 
> Regards.
> 


How portable does your code need to be?  If you can use gcc extensions,
you could consider using "cleanup" functions as a sort of C++
destructor.  Then your code becomes:

PROLOG:
	alloc #1 with cleanup function 1
	if (failure) return err;
	alloc #2 with cleanup function 2
	if (failure) return err;
	alloc #3 with cleanup function 3
	if (failure) return err;

...

EPILOG:
	return success

Your cleanup functions are called in reverse order, and only if your
code has got as far as declaring them.


Of course, you could also use C++ and RAII :-)

[toc] | [prev] | [next] | [standalone]

#93896

On 22/09/2016 15:10, David Brown wrote:

> On 22/09/16 14:46, Noob wrote:
> 
>> As you allude to, there is some context between PROLOG and EPILOG.
>>
>> PROLOG does several "setup" operations, which must be "torn down"
>> in the reverse order in EPILOG, with proper error-handling, meaning
>> we cannot tear down a resource that has not been set up.
>>
>> PROLOG:
>>   alloc #1
>>   if (failure) goto L1;
>>   alloc #2
>>   if (failure) goto L2;
>>   alloc #3
>>   if (failure) goto L3;
>>
>> EPILOG:
>>   tear_down(#3);
>> L3:
>>   tear_down(#2);
>> L2:
>>   tear_down(#1);
>> L1:
>>   return err;
>>
>> Also I'm not a fan of separating PROLOG and EPILOG, because they
>> are obviously closely related, and it doesn't make sense to call
>> one without the other. But it's interesting to have other points
>> of view.
> 
> How portable does your code need to be?  If you can use gcc extensions,

This code will be submitted to the Linux kernel. So C++ is out ;-)
They do use many gcc extensions (e.g. updating void pointers)
but git grep '((cleanup))' returns nothing for Linux 4.7

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

> cleanup (cleanup_function)
> 
> The cleanup attribute runs a function when the variable goes out of
> scope. This attribute can only be applied to auto function scope
> variables; it may not be applied to parameters or variables with
> static storage duration. The function must take one parameter, a
> pointer to a type compatible with the variable. The return value of
> the function (if any) is ignored.
> 
> If -fexceptions is enabled, then cleanup_function is run during the
> stack unwinding that happens during the processing of the exception.
> Note that the cleanup attribute does not allow the exception to be
> caught, only to perform an action. It is undefined what happens if
> cleanup_function does not return normally.

"Exceptions" means C++ right? (not C)

I'm not familiar with this attribute. Thanks for pointing it out.
Does that mean one has to define a function for every variable
that is to be allocated? And test in every function before
actually cleaning up?


> you could consider using "cleanup" functions as a sort of C++
> destructor.  Then your code becomes:
> 
> PROLOG:
> 	alloc #1 with cleanup function 1
> 	if (failure) return err;
> 	alloc #2 with cleanup function 2
> 	if (failure) return err;
> 	alloc #3 with cleanup function 3
> 	if (failure) return err;
> 
> ...
> 
> EPILOG:
> 	return success
> 
> Your cleanup functions are called in reverse order, and only if your
> code has got as far as declaring them.

Regards.

[toc] | [prev] | [next] | [standalone]

#93920

On Thursday, September 22, 2016 at 2:36:32 PM UTC+1, Noob wrote:
> On 22/09/2016 15:10, David Brown wrote:
> 

> > If -fexceptions is enabled, then cleanup_function is run during the
> > stack unwinding that happens during the processing of the exception.
> > Note that the cleanup attribute does not allow the exception to be
> > caught, only to perform an action. It is undefined what happens if
> > cleanup_function does not return normally.
> 
> "Exceptions" means C++ right? (not C)
> 
> I'm not familiar with this attribute. Thanks for pointing it out.
> Does that mean one has to define a function for every variable
> that is to be allocated? And test in every function before
> actually cleaning up?
> 
More or less.
If a subroutine you call throws an exception, standard C has no
way of dealing with that. However unless you own memory, or
have file handles or other resources dangling, that's unlikely
to be an issue. Whoever threw the exception (probably C++ but
could be another language with the same facilities) wants your
function to be aborted and the exception handled somewhere up
the call stack.
If you have resources, you should close or free them. But that's
all you should be doing, you shouldn't try to handle the exception
in any other way.

Incidentally avoid this anti-pattern

void free_object(OBJECT **obj)
{
  delete_object(*obj);
}

OBJECT *objectptr __attribute__ ((__cleanup__(free_object))) = createobject();


the exception is likely to be thrown by createobject itself, if it
can't get the memory or other bits and pieces it needs to create
the object. So you need to set objectptr to null, then call the
constructor, then test for null in the cleanup function.

[toc] | [prev] | [next] | [standalone]

#93931

On 22/09/16 15:36, Noob wrote:
> On 22/09/2016 15:10, David Brown wrote:
> 
>> On 22/09/16 14:46, Noob wrote:
>>
>>> As you allude to, there is some context between PROLOG and EPILOG.
>>>
>>> PROLOG does several "setup" operations, which must be "torn down"
>>> in the reverse order in EPILOG, with proper error-handling, meaning
>>> we cannot tear down a resource that has not been set up.
>>>
>>> PROLOG:
>>>   alloc #1
>>>   if (failure) goto L1;
>>>   alloc #2
>>>   if (failure) goto L2;
>>>   alloc #3
>>>   if (failure) goto L3;
>>>
>>> EPILOG:
>>>   tear_down(#3);
>>> L3:
>>>   tear_down(#2);
>>> L2:
>>>   tear_down(#1);
>>> L1:
>>>   return err;
>>>
>>> Also I'm not a fan of separating PROLOG and EPILOG, because they
>>> are obviously closely related, and it doesn't make sense to call
>>> one without the other. But it's interesting to have other points
>>> of view.
>>
>> How portable does your code need to be?  If you can use gcc extensions,
> 
> This code will be submitted to the Linux kernel. So C++ is out ;-)

So C++ is out, but gcc extensions are allowed.  There is an on-going
project to make the kernel compilation work with clang, so you'll annoy
people if the extension is gcc only, but clang supports most of gcc's
extensions so you are probably okay here.

But you may need to check with the relevant maintainers before being the
first to use a particular gcc extension in the kernel.

> They do use many gcc extensions (e.g. updating void pointers)
> but git grep '((cleanup))' returns nothing for Linux 4.7
> 
> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html
> 
>> cleanup (cleanup_function)
>>
>> The cleanup attribute runs a function when the variable goes out of
>> scope. This attribute can only be applied to auto function scope
>> variables; it may not be applied to parameters or variables with
>> static storage duration. The function must take one parameter, a
>> pointer to a type compatible with the variable. The return value of
>> the function (if any) is ignored.
>>
>> If -fexceptions is enabled, then cleanup_function is run during the
>> stack unwinding that happens during the processing of the exception.
>> Note that the cleanup attribute does not allow the exception to be
>> caught, only to perform an action. It is undefined what happens if
>> cleanup_function does not return normally.
> 
> "Exceptions" means C++ right? (not C)

Sort of.  gcc supports exceptions to some extent in C as well - mainly
with a view to allowing exceptions to pass through C compiled functions
on their way between two C++ functions.  But for a C only project (like
the kernel), "-fexceptions" is not enabled, so you don't have to worry
about this one.

> 
> I'm not familiar with this attribute. Thanks for pointing it out.
> Does that mean one has to define a function for every variable
> that is to be allocated? And test in every function before
> actually cleaning up?

I have not used it myself - I am just throwing it out here as a
suggestion that you can investigate.  So it may not be suitable, or it
may make your code really messy - if so, then at least you'll have
learned something new :-)

The cleanup function gets a pointer to the variable to be "cleaned" as
an argument - this may mean that you only need the one function for all
the cleanups.

And since you are in gcc extension mode, you might also use a local
nested function for your cleanup function to keep everything neatly in
the one place.

Have fun with the idea, anyway.

> 
> 
>> you could consider using "cleanup" functions as a sort of C++
>> destructor.  Then your code becomes:
>>
>> PROLOG:
>> 	alloc #1 with cleanup function 1
>> 	if (failure) return err;
>> 	alloc #2 with cleanup function 2
>> 	if (failure) return err;
>> 	alloc #3 with cleanup function 3
>> 	if (failure) return err;
>>
>> ...
>>
>> EPILOG:
>> 	return success
>>
>> Your cleanup functions are called in reverse order, and only if your
>> code has got as far as declaring them.
> 
> Regards.
> 

[toc] | [prev] | [next] | [standalone]

#93921

On Thu, 22 Sep 2016 14:46:37 +0200, Noob <root@127.0.0.1> wrote:

>On 22/09/2016 14:11, BartC wrote:

<snip>

>> Why not just create Prolog() and Epilog() functions that can be called 
>> from read() and write()? Or is there too much local context to be passed?
>> 
>> Then you don't have the rather unsatisfactory situation of two distinct 
>> operations ending up being handled by the same code before having to be 
>> disentangled again.
>
>As you allude to, there is some context between PROLOG and EPILOG.
>
>PROLOG does several "setup" operations, which must be "torn down"
>in the reverse order in EPILOG, with proper error-handling, meaning
>we cannot tear down a resource that has not been set up.
>
>PROLOG:
>  alloc #1
>  if (failure) goto L1;
>  alloc #2
>  if (failure) goto L2;
>  alloc #3
>  if (failure) goto L3;
>
>EPILOG:
>  tear_down(#3);
>L3:
>  tear_down(#2);
>L2:
>  tear_down(#1);
>L1:
>  return err;

This issue could be addressed by passing EPILOG an argument that tells
it where to start the cleanup operation.

>Also I'm not a fan of separating PROLOG and EPILOG, because they
>are obviously closely related, and it doesn't make sense to call
>one without the other. But it's interesting to have other points
>of view.

C abounds with closely related functions of this nature: fopen and
fclose, malloc and free, sin and cos, toupper and tolower, exp and
log, etc.  Another set specific to your application doesn't seem
unreasonable.  

-- 
Remove del for email

[toc] | [prev] | [next] | [standalone]

#93949

On 9/22/16 8:46 AM, Noob wrote:

> As you allude to, there is some context between PROLOG and EPILOG.
>
> PROLOG does several "setup" operations, which must be "torn down"
> in the reverse order in EPILOG, with proper error-handling, meaning
> we cannot tear down a resource that has not been set up.
>
> PROLOG:
>   alloc #1
>   if (failure) goto L1;
>   alloc #2
>   if (failure) goto L2;
>   alloc #3
>   if (failure) goto L3;
>
> EPILOG:
>   tear_down(#3);
> L3:
>   tear_down(#2);
> L2:
>   tear_down(#1);
> L1:
>   return err;
>
> Also I'm not a fan of separating PROLOG and EPILOG, because they
> are obviously closely related, and it doesn't make sense to call
> one without the other. But it's interesting to have other points
> of view.
>
> Regards.
>

One alternative, if you want to avoid the cast are willing to have 
prolog and epilog be seperate functions would be something like


struct resources {
   /* Info about resources here */

};
static int prolog(void* dev, struct resources* res) {
   alloc #1
   if (failure) return 1;
   alloc #2
   if (failure) return 2;
   alloc #3
   if (failure) return 3;

   return 0;
}

static int epilog(int err, void* dev, struct resources* res) {
   switch(err) {
   /* All cases intentionally fall through */
   case 0:
     tear_down #3
   case 3:
     tear_down #2
   case 2:
     tear_down #1
   case 1:
   }
}

void read(void* dev, void* buf, int len) {
   struct resources res;
   int err = prolog(dev, &res);
   if(err == 0) do read
   epilog(err, dev, &res);
}

prolog and epilog are made static functions so they are only accessible 
in that file, as other code should not be accessing them.

Having made them into separate functions, if some other operation (maybe 
a 'seek', or 'rewind' or 'update' operation) is added, it can use the 
same prolog/epilog operations to implement it, and that operation can 
just be added to the API. You don't need to touch the read or write 
routines. Having your 'access' routine (maybe better called something 
like doReadOrWrite, which should also be a static function) says that if 
some other operation needs any other parameters to define it, the 
signature of the common function needs to be changed, and read and write 
modified to pass some dummy value for that parameter. You also have, as 
you commented, the need for something like the cast to take away the 
const for the write routine.

Anticipating that there may be information about the resources that 
needs to be passed, from prolog to operation and epilog, I have added a 
resources struct for that data. If we wanted, we could even include the 
err flag in there, but I think being explicit about that is good, and 
makes it easier for prolog to signal the state.

An alternative to casting away the const is to pass two pointers to 
access, one a void *, and one a void const*, read passing a NULL to the 
void const* (which isn't used by the read operation) and write passing a 
NULL to the void * (which isn't used by the write operation).

[toc] | [prev] | [next] | [standalone]

#93906

Noob <root@127.0.0.1> writes:

> Hello everyone,
>
> I'm required to implement a read() function and a write() function.
> (The prototype of these functions is not under my control.)
>
>   void  read(void *dev, void *buf, int len);
>   void write(void *dev, const void *buf, int len);
>
> There is a lot of code in common between the two implementations:
>
> read:
> COMMON PROLOG
> do the actual read
> COMMON EPILOG
>
> write:
> COMMON PROLOG
> do the actual write
> COMMON EPILOG
>
> Therefore, to avoid code duplication, I implemented a generic
> access() function:
>
> void access(void *dev, void *buf, int len, int dir);
> COMMON PROLOG
> if (dir == READ)  do the actual read
> if (dir == WRITE) do the actual write
> COMMON EPILOG
>
> then I implemented read() and write() as wrappers around access()
>
> read  = access(dev, buf, len, READ);
> write = access(dev, (void *)buf, len, WRITE);
>
> As you can see, I had to cast away the constness of the write buffer,
> because my generic function does change buf, only in the read case.
> But I think there is no undefined behavior, because I do not modify
> the const buf.  Is that correct?
>
>
> Some people frowned upon this cast, and suggested a union.
>
> union buf { void *in; const void *out; };
> void access(void *dev, union buf *buf, int len, int dir)
> COMMON PROLOG
> if (dir == READ) do the actual read on buf->in
> if (dir == WRITE) do the actual write on buf->out
> COMMON EPILOG
>
> That does solve the issue "casting away const is bad", but it
> seems not very idiomatic (to me).  Do people consider this a
> better solution than casting away const?

At this point I thought I understood what you were asking but
then I noticed the text following.  I potentially have some
suggestions but please see below.

> Something I didn't mention is that the access function actually
> updates buf in the EPILOG.
>
>   buf->in += len;
>
> Is it *100%* guaranteed that incrementing buf->in is strictly
> equivalent to incrementing buf->out ?
>
> In other words, is this assert always true, even on DS9k?
>
>   temp = buf->out;
>   buf->in += len;
>   assert(buf->in == temp + len);

Something crucial has been left out, specifically the definitions
of the wrapper functions that call access().  Based on the above
outline it looks like read() and write() simply make (or would
make) a local union object, and then call access() with the
address of that object.  But if that is true there is no point to
incrementing buf->in (or buf->out) since the value is then just
thrown away.  (And the void* types for 'buf' in the original
prototypes do nothing to unmuddy the water.)  Can you please
supply all the relevant details?  Generally my answers are
better if I don't have to shoot in the dark.

[toc] | [prev] | [next] | [standalone]

#94054

On 22/09/2016 17:35, Tim Rentsch wrote:

> Something crucial has been left out, specifically the definitions
> of the wrapper functions that call access().  Based on the above
> outline it looks like read() and write() simply make (or would
> make) a local union object, and then call access() with the
> address of that object.  But if that is true there is no point to
> incrementing buf->in (or buf->out) since the value is then just
> thrown away.  (And the void* types for 'buf' in the original
> prototypes do nothing to unmuddy the water.)  Can you please
> supply all the relevant details?  Generally my answers are
> better if I don't have to shoot in the dark.

Hello Tim,

You're right that my example sounds fishy. I mixed two examples
into one, and the result is just confusing. I'll post about the
more interesting example tomorrow.

Regards.

[toc] | [prev] | [next] | [standalone]

#94082

On 22/09/2016 17:35, Tim Rentsch wrote:

> Something crucial has been left out, specifically the definitions
> of the wrapper functions that call access().  Based on the above
> outline it looks like read() and write() simply make (or would
> make) a local union object, and then call access() with the
> address of that object.  But if that is true there is no point to
> incrementing buf->in (or buf->out) since the value is then just
> thrown away.  (And the void* types for 'buf' in the original
> prototypes do nothing to unmuddy the water.)  Can you please
> supply all the relevant details?  Generally my answers are
> better if I don't have to shoot in the dark.

The crucial missing piece of information is that the contents
of the buffer need to be "unswizzled" because what is stored
on the device differs from what is shown to upper layers.

To recap, I'm supposed to implement

  void foo_read (void *dev, uint8_t *buf, int len);
  void foo_write(void *dev, const uint8_t *buf, int len);

The "elementary" read and write functions are

  void elem_read (void *dev, uint8_t *buf, int len);
  void elem_write(void *dev, const uint8_t *buf, int len);

If there were no swizzling requirement, I'd just say

  foo_read = elem_read;
  foo_write = elem_write;

and call it a day.


But actually I have to read a few bytes, skip a few,
read a few more, etc.

So foo_read looks like this:

void foo_read (void *dev, uint8_t *buf, int len)
{
  elem_read(dev, buf, 12); buf += 12;
  skip_forward(dev, 30);

  elem_read(dev, buf, 15); buf += 15;
  skip_forward(dev, 33);

  elem_read(dev, buf, 17); buf += 17;
  skip_forward(dev, 22);

  /*** snip ***/
}

and it turns out the write function is exactly the same
with elem_read changed to elem_write.

So it makes sense (in my opinion) to implement foo_access()
to avoid duplicating code between foo_read and foo_write.

I hope my description was clear (enough).


So my implementation looks more like this:

void foo_generic(void *dev, uint8_t *buf, int len, int dir)
{
  elem_dispatch(dev, buf, 12, dir); buf += 12;
  skip_forward(dev, 30);

  elem_dispatch(dev, buf, 15, dir); buf += 15;
  skip_forward(dev, 33);

  elem_dispatch(dev, buf, 17, dir); buf += 17;
  skip_forward(dev, 22);

  /*** snip ***/
}

And of course elem_dispatch calls either elem_read or elem_write
based on the dir parameter. But this solution requires casting
away the constness of the write buffer.

So it was suggested to me to use a union-based solution.

Regards.

[toc] | [prev] | [next] | [standalone]

#94096

On 09/26/2016 02:54 PM, Noob wrote:
> On 22/09/2016 17:35, Tim Rentsch wrote:
>
>> Something crucial has been left out, specifically the definitions
>> of the wrapper functions that call access().  Based on the above
>> outline it looks like read() and write() simply make (or would
>> make) a local union object, and then call access() with the
>> address of that object.  But if that is true there is no point to
>> incrementing buf->in (or buf->out) since the value is then just
>> thrown away.  (And the void* types for 'buf' in the original
>> prototypes do nothing to unmuddy the water.)  Can you please
>> supply all the relevant details?  Generally my answers are
>> better if I don't have to shoot in the dark.
>
> The crucial missing piece of information is that the contents
> of the buffer need to be "unswizzled" because what is stored
> on the device differs from what is shown to upper layers.
>
> To recap, I'm supposed to implement
>
>   void foo_read (void *dev, uint8_t *buf, int len);
>   void foo_write(void *dev, const uint8_t *buf, int len);
>
> The "elementary" read and write functions are
>
>   void elem_read (void *dev, uint8_t *buf, int len);
>   void elem_write(void *dev, const uint8_t *buf, int len);
>
> If there were no swizzling requirement, I'd just say
>
>   foo_read = elem_read;
>   foo_write = elem_write;
>
> and call it a day.
>
>
> But actually I have to read a few bytes, skip a few,
> read a few more, etc.
>
> So foo_read looks like this:
>
> void foo_read (void *dev, uint8_t *buf, int len)
> {
>   elem_read(dev, buf, 12); buf += 12;
>   skip_forward(dev, 30);
>
>   elem_read(dev, buf, 15); buf += 15;
>   skip_forward(dev, 33);
>
>   elem_read(dev, buf, 17); buf += 17;
>   skip_forward(dev, 22);
>
>   /*** snip ***/
> }
>
> and it turns out the write function is exactly the same
> with elem_read changed to elem_write.
>
> So it makes sense (in my opinion) to implement foo_access()
> to avoid duplicating code between foo_read and foo_write.
>
> I hope my description was clear (enough).

I think so.
Why not defining macros which would define functions depending on the 
constness of 'buf'?

Something like that:

#define DEFINE_FOO_RW(name, suffix, type)                         \
void name(void *dev, type buf, int len)                           \
{                                                                 \
     int len;                                                      \
                                                                   \
     len = 12;                                                     \
     elem_##suffix(dev, buf, len); buf += len;                     \
     skip_forward(dev, 30);                                        \
}

DEFINE_FOO_RW(foo_read, read, uint32_t *)
DEFINE_FOO_RW(foo_write, write, const uint32_t *)

[toc] | [prev] | [next] | [standalone]

#94097

Noob <root@127.0.0.1> writes:

> On 22/09/2016 17:35, Tim Rentsch wrote:
>
>> Something crucial has been left out, specifically the definitions
>> of the wrapper functions that call access().  Based on the above
>> outline it looks like read() and write() simply make (or would
>> make) a local union object, and then call access() with the
>> address of that object.  But if that is true there is no point to
>> incrementing buf->in (or buf->out) since the value is then just
>> thrown away.  (And the void* types for 'buf' in the original
>> prototypes do nothing to unmuddy the water.)  Can you please
>> supply all the relevant details?  Generally my answers are
>> better if I don't have to shoot in the dark.
>
> The crucial missing piece of information is that the contents
> of the buffer need to be "unswizzled" because what is stored
> on the device differs from what is shown to upper layers.
>
> To recap, I'm supposed to implement
>
>   void foo_read (void *dev, uint8_t *buf, int len);
>   void foo_write(void *dev, const uint8_t *buf, int len);
>
> The "elementary" read and write functions are
>
>   void elem_read (void *dev, uint8_t *buf, int len);
>   void elem_write(void *dev, const uint8_t *buf, int len);
>
> If there were no swizzling requirement, I'd just say
>
>   foo_read = elem_read;
>   foo_write = elem_write;
>
> and call it a day.
>
>
> But actually I have to read a few bytes, skip a few,
> read a few more, etc.
>
> So foo_read looks like this:
>
> void foo_read (void *dev, uint8_t *buf, int len)
> {
>   elem_read(dev, buf, 12); buf += 12;
>   skip_forward(dev, 30);
>
>   elem_read(dev, buf, 15); buf += 15;
>   skip_forward(dev, 33);
>
>   elem_read(dev, buf, 17); buf += 17;
>   skip_forward(dev, 22);
>
>   /*** snip ***/
> }
>
> and it turns out the write function is exactly the same
> with elem_read changed to elem_write.
>
> So it makes sense (in my opinion) to implement foo_access()
> to avoid duplicating code between foo_read and foo_write.
>
> I hope my description was clear (enough).
>
>
> So my implementation looks more like this:
>
> void foo_generic(void *dev, uint8_t *buf, int len, int dir)
> {
>   elem_dispatch(dev, buf, 12, dir); buf += 12;
>   skip_forward(dev, 30);
>
>   elem_dispatch(dev, buf, 15, dir); buf += 15;
>   skip_forward(dev, 33);
>
>   elem_dispatch(dev, buf, 17, dir); buf += 17;
>   skip_forward(dev, 22);
>
>   /*** snip ***/
> }
>
> And of course elem_dispatch calls either elem_read or elem_write
> based on the dir parameter. But this solution requires casting
> away the constness of the write buffer.

Hmmm...  You might consider passing both a pointer to const data and a
pointer to non-const data, and using whichever is NULL as the condition
to dispatch on:

void foo_generic(void *dev, uint8_t *rd_buf, const uint8_t *wr_buf, int len)
{
   elem_dispatch(dev, rd_buf, wr_buf, 12); buf += 12;
   skip_forward(dev, 30);

   /* ... */
}

void elem_dispatch(void *dev, uint8_t *rd_buf, const uint8_t *wr_buf, int len)
{
   if (rd_buf) elem_read (dev, rd_buf, len);
   else        elem_write(dev, wr_buf, len);
}

void foo_read(void *dev, uint8_t *buf, int len)
{
   foo_generic(dev, buf, 0, len);
}

void foo_write(void *dev, const uint8_t *buf, int len)
{
   foo_generic(dev, 0, buf, len);
}

<snip>
-- 
Ben.

[toc] | [prev] | [next] | [standalone]

#94165

Noob <root@127.0.0.1> writes:

> On 22/09/2016 17:35, Tim Rentsch wrote:
>
>> Something crucial has been left out, specifically the definitions
>> of the wrapper functions that call access().  Based on the above
>> outline it looks like read() and write() simply make (or would
>> make) a local union object, and then call access() with the
>> address of that object.  But if that is true there is no point to
>> incrementing buf->in (or buf->out) since the value is then just
>> thrown away.  (And the void* types for 'buf' in the original
>> prototypes do nothing to unmuddy the water.)  Can you please
>> supply all the relevant details?  Generally my answers are
>> better if I don't have to shoot in the dark.
>
> The crucial missing piece of information is that the contents
> of the buffer need to be "unswizzled" because what is stored
> on the device differs from what is shown to upper layers.
>
> [elaboration]
>
> I hope my description was clear (enough).

Yes I think so.  To be sure, let me play back what I understood you
to be saying (with some slight variations in some of the names).

There are some already defined, pre-existing low-level functions:

 extern void basic_read(  void *device,       uint8_t *area, int length );
 extern void basic_write( void *device, const uint8_t *area, int length );
 extern void skip_forward( void *device, int skip_amount );

You are tasked with writing two higher level functions that will
call the low-level functions:

 extern void swizzled_read(  void *device,       uint8_t *area, int length );
 extern void swizzled_write( void *device, const uint8_t *area, int length );

Because the two higher level functions are basically the same except
for whether reading or writing is done, you want to factor out the
common code so these two functions can be written as simple wrappers
calling a common routine (here the arguments READ and WRITE are
meant as constants, eg, enumeration names):

 void
 swizzled_read( void *device, uint8_t *buffer, int length ){
     swizzled_read_or_write( READ, device, buffer, length );
 }

 void
 swizzled_write( void *device, const uint8_t *buffer, int length ){
     swizzled_read_or_write( WRITE, device, buffer, length );
 }

and the question is how to arrange that.  Is that a fairly accurate
summary?  If not then I have misunderstood something.


> So my implementation looks more like this:
>
> void foo_generic(void *dev, uint8_t *buf, int len, int dir)
> {
>   elem_dispatch(dev, buf,  12, dir); buf += 12;
>   skip_forward(dev, 30);
>
>   elem_dispatch(dev, buf,  15, dir); buf += 15;
>   skip_forward(dev, 33);
>
>   elem_dispatch(dev, buf,  17, dir); buf += 17;
>   skip_forward(dev, 22);
>
>   /*** snip ***/
> }
>
> And of course elem_dispatch calls either elem_read or elem_write
> based on the dir parameter.  But this solution requires casting
> away the constness of the write buffer.
>
> So it was suggested to me to use a union-based solution.

IMO a union-based solution is an inferior choice.  Let's try going
forward on the swizzled_read_or_write() approach, for now being a
bit vague on just what types are used, and see what we get:

 static void
 swizzled_read_or_write( Direction way, Device d, Base buffer, int length ){
     read_or_write_then_skip( way, d, &buffer, 12, 30 );
     read_or_write_then_skip( way, d, &buffer, 15, 33 );
     read_or_write_then_skip( way, d, &buffer, 17, 22 );
     /* ... etc ... */
     (void) &length;  /* in what way is 'length' supposed to be used? */
 }

 static void
 read_or_write_then_skip( Direction way, Device d, Base *pb, int n, int skip ){
     Base buffer = *pb;
     assert( way == READ || way == WRITE );
     if(  way == READ  )  basic_read( d, buffer, n );
     else                basic_write( d, buffer, n );
     *pb += n;
     skip_forward( d, skip );
 }

This scheme works except in one case (for writes) a 'const' is
needed and in the other case (for reads) 'const' cannot be present.
The approach you describe casts away constness on the write() path.
Suppose we don't do that, but instead define Base this way, so
const is used in most places:

 typedef const uint8_t *Base;

Now everything works except the final call to basic_read().  There
we need to take out the constness that was added in the outermost
call:

 static void
 read_or_write_then_skip( Direction way, Device d, Base *pb, int n, int skip ){
     Base buffer = *pb;
     assert( way == READ || way == WRITE );
     if(  way == READ  )  basic_read( d, (uint8_t *) buffer, n );
     else                basic_write( d,             buffer, n );
     *pb += n;
     skip_forward( d, skip );

     /* Note: revised to cast argument to basic_read() */
 }

This alternative may be more satisfactory to you reviewers.  The
constness of the buffer for writing is preserved.  There is a cast
to take away constness of an argument to basic_read(), but that
constness was added in a higher level (but still interior) call.
To verify correctness, we need observe just a few facts:

  1.  The cast happens only when 'way == READ' is true;

  2.  The interior functions swizzled_read_or_write() and
      read_or_write_then_skip() are static, so their callers
      can be determined by examining just the one .c file;

  3.  The only function that calls swizzled_read_or_write()
      with a first argument of READ is the outer function
      swizzled_read();  and

  4.  The parameter of swizzled_read() that is used to supply
      the argument value that will eventually be casted is
      appropriately non-const.

Given what I understand of what you need to do, this approach is
what I suggest, along with the correctness argument summarized
above.  It is of course possible to find other ways to solve the
problem, including ones that don't use either unions or casts, but
they will likely be clunkier, harder to follow, or more error prone
(and possibly all three).  I don't endorse casting lightly;  in
this case I think using a cast is a better option than any other
approach I've looked at.

For completeness here is the whole source for my example.  (Please
ignore minor style issues, eg, typedef'ing a pointer type, which
some people find objectionable.)  This source should give a clean
compile as a single stand-alone file.


/****** Start here *********************************************************/

#include <stdint.h>

extern void basic_read(  void *device,       uint8_t *area, int length );
extern void basic_write( void *device, const uint8_t *area, int length );
extern void skip_forward( void *device, int skip_amount );

extern void swizzled_read(  void *device,       uint8_t *area, int length );
extern void swizzled_write( void *device, const uint8_t *area, int length );


/* Everything that follows is local to the .c file */
#include <assert.h>

typedef enum { READ, WRITE } Direction;
typedef void *Device;
typedef const uint8_t *Base;

static void swizzled_read_or_write( Direction, void *, Base, int );
static void read_or_write_then_skip( Direction, Device, Base *, int, int );


void
swizzled_read( void *device, uint8_t *buffer, int length ){
    swizzled_read_or_write( READ, device, buffer, length );
}

void
swizzled_write( void *device, const uint8_t *buffer, int length ){
    swizzled_read_or_write( WRITE, device, buffer, length );
}


static void
swizzled_read_or_write( Direction way, Device d, Base buffer, int length ){
    read_or_write_then_skip( way, d, &buffer, 12, 30 );
    read_or_write_then_skip( way, d, &buffer, 15, 33 );
    read_or_write_then_skip( way, d, &buffer, 17, 22 );
    /* ... etc ... */
    (void) &length;  /* in what way is 'length' supposed to be used? */
}

static void
read_or_write_then_skip( Direction way, Device d, Base *pb, int n, int skip ){
    Base buffer = *pb;
    assert( way == READ || way == WRITE );
    if(  way == READ  )  basic_read( d, (uint8_t *) buffer, n );
    else                basic_write( d,             buffer, n );
    *pb += n;
    skip_forward( d, skip );
}

/****** Finish here ********************************************************/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | comp.lang.c

csiph-web