SourceForge has been redesigned. Learn more.
Close

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

closed
nobody
None
4
2014-11-15
2014-06-09
No

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.

1 Attachments

Discussion

  • Cary R.

    Cary R. - 2014-06-09

    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

      Larry Doolittle - 2014-06-09

      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.

        Cary R. - 2014-06-09

        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.

    Cary R. - 2014-06-09

    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

    Larry Doolittle - 2014-06-10

    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.

    Cary R. - 2014-06-10
    • status: open --> closed-accepted
     
  • Cary R.

    Cary R. - 2014-06-10

    Second patch applied.

     
  • Cary R.

    Cary R. - 2014-06-10
    • status: closed-accepted --> open
     
    • Larry Doolittle

      Larry Doolittle - 2014-06-10

      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.

    Cary R. - 2014-06-10

    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.

    Cary R. - 2014-06-10

    Okay I missed that. Patch applied.

     
  • Cary R.

    Cary R. - 2014-06-10
    • status: open --> closed
     

Log in to post a comment.