1493532 - mork compile warnings with GCC 8.2

Status: NEW

(MailNews Core :: Database, enhancement)

Description

[:aceman]

c-c finally seems to be able to compile with gcc 8.2 on Linux.

There are some new compile warnings, of which only these are from c-c:
warning: comm/db/mork/src/morkConfig.h:128:59 [-Wclass-memaccess] ‘void* memmove(void*, const void*, size_t)’ writing to an object of non-trivially copyable type ‘class morkCell’; use copy-assignment or copy-initialization inste
ad
warning: comm/db/mork/src/morkConfig.h:129:59 [-Wclass-memaccess] ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘class morkCell’; use assignment or value-initialization instead
warning: comm/db/mork/src/morkConfig.h:129:59 [-Wclass-memaccess] ‘void* memset(void*, int, size_t)’ clearing an object of type ‘class morkRow’ with no trivial copy-assignment; use value-initialization instead

We should fix them before they fail the build due to -Werror (warnings as errors) forced on us from m-c.

Comment 1

[:aceman]

Jorg, would you know what to do here?
Copy/clear each member of the object instead of memset/move of its memory?
Are there better ways?

Comment 2

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #0)
> We should fix them before they fail the build due to -Werror (warnings as
> errors) forced on us from m-c.
That's not going to happen since all platform use clang, why are you still messing around with gcc ;-(

That said, the warnings you pasted are non-conclusive since they point to these lines:
#define MORK_MEMMOVE(dest,src,size)  memmove(dest,src,size)
#define MORK_MEMSET(dest,byte,size)  memset(dest,byte,size)

Are we talking about the invocation here:
comm/db/mork/src/morkRow.cpp
822 MORK_MEMMOVE(cell, next, after * sizeof(morkCell));
That's moving as many cells as 'after' specifies, so generally more than one.

MORK_MEMSET is used a bunch of times to zero-out some stuff:
Could be more than one here:
MORK_MEMSET(p, 0, inSize);
MORK_MEMSET(newCells, 0, size);
MORK_MEMSET(ioMapKey, 0, (inKeyCount * sMap_KeySize));

(In reply to :aceman from comment #1)
> Jorg, would you know what to do here?
I wouldn't touch this stuff, so I'd WONTFIX it.

> Copy/clear each member of the object instead of memset/move of its memory?
If the compiler doesn't allow you to bulk copy bytes to copy/move multiple objects, then you need to copy/move them one-by-one in a loop :-( - Looks like Mork isn't using the C++ concept of a constructor and is in instead allocating memory to create objects and is copying bytes around to fill them with content. If that's the case, we're going to have a hard time bringing this up to C++ speed, so again, I wouldn't touch it.

Comment 3

[:aceman]

(In reply to Jorg K (GMT+2) from comment #2)
> That's not going to happen since all platform use clang, why are you still
> messing around with gcc ;-(

Did I mention clang does not work for me for some reason? Also, checking with multiple compilers is always good, similar to testing m-c code in Thunderbird contexts may find new bugs :)
 
> Are we talking about the invocation here:
> comm/db/mork/src/morkRow.cpp
> 822 MORK_MEMMOVE(cell, next, after * sizeof(morkCell));
> That's moving as many cells as 'after' specifies, so generally more than one.

> > Copy/clear each member of the object instead of memset/move of its memory?
> If the compiler doesn't allow you to bulk copy bytes to copy/move multiple
> objects, then you need to copy/move them one-by-one in a loop :-( - Looks
> like Mork isn't using the C++ concept of a constructor and is in instead
> allocating memory to create objects and is copying bytes around to fill them
> with content. If that's the case, we're going to have a hard time bringing
> this up to C++ speed, so again, I wouldn't touch it.

morkCell is does seem to have a constructor:
  morkCell() : mCell_Delta( 0 ), mCell_Atom( 0 ) { }

  morkCell(const morkCell& c)
  : mCell_Delta( c.mCell_Delta ), mCell_Atom( c.mCell_Atom ) { }

What is this other function? Isn't it to intialize the new object from another one? May that be used for copying?