#1271 Don't lie to the c++ compiler about some pointer alignment

[Larry Doolittle]

You can call this ugly, and you'd be right. But it is more literally correct,
and removes the following warnings from a clang-based build:
vvp/vpi_const.cc:196:34: warning: cast from 'char ' to 'p_vpi_vecval' (aka 't_vpi_vecval ') increases required alignment from 1 to 4 [-Wcast-align]
vvp/vpi_priv.cc:718:22: warning: cast from 'char ' to 'p_vpi_vecval' (aka 't_vpi_vecval ') increases required alignment from 1 to 4 [-Wcast-align]
vvp/vpi_priv.cc:783:22: warning: cast from 'char ' to 'p_vpi_vecval' (aka 't_vpi_vecval ') increases required alignment from 1 to 4 [-Wcast-align]
vvp/vpi_signal.cc:429:12: warning: cast from 'char ' to 's_vpi_strengthval ' (aka 't_vpi_strengthval ') increases required alignment from 1 to 4 [-Wcast-align]
vvp/vpi_signal.cc:484:26: warning: cast from 'char ' to 'p_vpi_vecval' (aka 't_vpi_vecval ') increases required alignment from 1 to 4 [-Wcast-align]
vvp/vpi_vthr_vector.cc:321:25: warning: cast from 'char ' to 's_vpi_vecval ' (aka 't_vpi_vecval ') increases required alignment from 1 to 4 [-Wcast-align]

It would be nice if there were a better way to do this, but I couldn't find it.
I won't be offended if you temporarily ignore the problem, or better still, put in a different patch.

[Cary R.]

Just thinking a bit. Should the function definition just be changed to be void and then update the places that need a char with a cast. To me the need_result_buf() and need_result_buf_void() functions could create confusion. If we have a single function that is then cast to the appropriate type that would seem to be a cleaner implementation.

[Larry Doolittle]

Cary -

On Mon, Jun 09, 2014 at 07:00:32PM +0000, Cary R. wrote:

Just thinking a bit. Should the function definition just be changed to be void and then update the places that need a char with a cast.

That's pretty much what this patch does, indirectly.

To me the need_result_buf() and need_result_buf_void() functions could create confusion. If we have a single function that is then cast to the appropriate type that would seem to be a cleaner implementation.

There is a single function, but since most of the use cases want
the result as (char *), I wrapped the function in a compatibility macro
to reduce the size of the patch. If you and Steve want a version with
35 casts added, instead of that compatibility macro, let me know and
I'll generate it.

The resulting code feel would match that of malloc(), which I guess is
a good thing.

• Larry

[Cary R.]

It's the indirectly that I was objecting to. Yes there is only a single function, but with the define it acts like two. I personally do not like using a define to create a function except for a few rare cases (e.g. see ivl_alloc.h which wraps existing functions instead of creating new ones). I don't know if we have any other examples of using a define to create an implicit function.

I would personally like to see the consistency. The size of the patch is mostly immaterial if it creates a cleaner interface.

[Cary R.]

Also note that this is enough of a change that the copyright date should be updated for any file that is not already marked 2014.

[Larry Doolittle]

Second try cleaning up cast-alignment problems surrounding need_result_buf().
This version is verbose and changes the prototype for need_result_buf().
But it is semantically (c++) correct, and makes need_result_buf() feel like malloc().

[Cary R.]

Second patch applied.

[Larry Doolittle]

OK, thanks for applying that patch!
I left out some second order changes, attached.
After that, vvp/ becomes free of -Wcast-align errors.

[Larry Doolittle]

Cary -

On Tue, Jun 10, 2014 at 06:47:04PM +0000, Cary R. wrote:

Does it make more sense to make rbuf a void * and then cast it to the appropriate type when assigning it to the structure field?

Not to me.

rbuf is used for more than just assigning to a structure field.

Adding a cast to lines of code like
vpip_vec4_to_dec_str(vvp_vector4_t(1024, real), rbuf, 1025, true);
sounds like step backwards in clarity.

• Larry

[Cary R.]

Does it make more sense to make rbuf a void * and then cast it to the appropriate type when assigning it to the structure field? This makes things more consistent.

[Cary R.]

Okay I missed that. Patch applied.