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.
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.
Cary -
On Mon, Jun 09, 2014 at 07:00:32PM +0000, Cary R. wrote:
That's pretty much what this patch does, indirectly.
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.
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.
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.
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().
Second patch applied.
OK, thanks for applying that patch!
I left out some second order changes, attached.
After that, vvp/ becomes free of -Wcast-align errors.
Cary -
On Tue, Jun 10, 2014 at 06:47:04PM +0000, Cary R. wrote:
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.
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.
Okay I missed that. Patch applied.