:aceman Assignee
	Description • 6 years ago

Trying to compile Thunderbird using gcc 8 on Linux produces new compile warning:

mailnews/mime/src/mimepbuf.cpp: In function ‘MimePartBufferData* MimePartBufferCreate()’:
mailnews/mime/src/mimepbuf.cpp:69:32: warning: ‘void* memset(void*, int, size_t)’ clearing an object of type ‘MimePartBufferData’ {aka ‘struct MimePartBufferData’} with no trivial copy-assignment; use assignment
or value-initialization instead [-Wclass-memaccess]
memset(data, 0, sizeof(*data));
                            ^
mailnews/mime/src/mimepbuf.cpp:52:8: note: ‘MimePartBufferData’ {aka ‘struct MimePartBufferData’} declared here
struct MimePartBufferData
       ^~~~~~~~~~~~~~~~~~

struct MimePartBufferData
{
  char        *part_buffer;          /* Buffer used for part-lookahead. */
  int32_t     part_buffer_fp;        /* Active length. */
  int32_t     part_buffer_size;      /* How big it is. */

  nsCOMPtr <nsIFile> file_buffer;    /* The nsIFile of a temp file used when we
                                                          run out of room in the head_buffer. */
  nsCOMPtr <nsIInputStream> input_file_stream;    /* A stream to it. */
  nsCOMPtr <nsIOutputStream> output_file_stream;  /* A stream to it. */
};

Yes, it seems strange to memset the struct with zeros when it contains objects, e.g. nsCOMPtr. That seems to assume too much about the internal representation of the object.

	:aceman Assignee
	Comment 1 • 6 years ago

There are similar problems in m-c linked in bug 1411029.

	:aceman Assignee
	Comment 2 • 6 years ago

This should do it. We could make the struct have a proper constructor/destructor, but that would be for the c++ rewrite of mime.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7e68e4a8cfc56318a2d61e166b17f55feb87b319

	Jorg K (CEST = GMT+2)
	Comment 3 • 6 years ago

Comment on attachment 8990607 [details] [diff] [review]
1474183.patch

I'm not a total fan of using a C structure as a C++ class and operating on it with 'new' and 'delete'. It works since C++ doesn't distinguish between classes and structures, but I don't think it's the right way to go here in an essentially C library.

Why don't you follow one of the M-C bugs, for example:
https://hg.mozilla.org/mozilla-central/rev/907068074409#l1.41
https://hg.mozilla.org/mozilla-central/rev/a08737444fc3#l1.13
https://hg.mozilla.org/mozilla-central/rev/9e0ca8aa027c#l1.12
https://hg.mozilla.org/mozilla-central/rev/30e090a9e408#l1.12

	:aceman Assignee
	Comment 4 • 6 years ago

Yes, I thought about not introducing too much of C++, that is why I didn't add a constructor (but your first link is like that). BUt the library isn't plain C for a long time already, even this same struct contains nsCOMPtr objects and templates. So I don't see a need to stick to plain C syntax.
I do not understand the other links, e.g. https://hg.mozilla.org/mozilla-central/rev/9e0ca8aa027c#l1.12, that looks like still memsetting the whole struct just silencing the compiler. But I'm no expert in that area.

	Jorg K (CEST = GMT+2)
	Comment 5 • 6 years ago

Comment on attachment 8990607 [details] [diff] [review]
1474183.patch

(In reply to :aceman from comment #4)
> So I don't see a need to stick to plain C syntax.
Fair point.

> I do not understand the other links, e.g.
> https://hg.mozilla.org/mozilla-central/rev/9e0ca8aa027c#l1.12, that looks
> like still memsetting the whole struct just silencing the compiler.
Yes, you could do that, too. I can't try for you since on Windows about anything will compile ;-(

	:aceman Assignee
	Comment 6 • 6 years ago

(In reply to Jorg K (GMT+2) from comment #5)
> > I do not understand the other links, e.g.
> > https://hg.mozilla.org/mozilla-central/rev/9e0ca8aa027c#l1.12, that looks
> > like still memsetting the whole struct just silencing the compiler.
> Yes, you could do that, too. I can't try for you since on Windows about
> anything will compile ;-(

This will also compile on linux/gcc, but at version 8 it warns about it, because memsetting an object like nsCOMPtr to all zeros may not be correct. Also, I think it does NOT run its constructor (which may set some values to non-zeros), if any. I hope my 'new' does.

	Jorg K (CEST = GMT+2)
	Comment 7 • 6 years ago

(In reply to :aceman from comment #6)
> I hope my 'new' does.
Hope doesn't help, facts do:
https://stackoverflow.com/questions/5914422/proper-way-to-initialize-c-structs

Solution inspired by
https://stackoverflow.com/questions/1127396/struct-constructor-in-c

Now we *know* it's a constructor that will also do the right thing for the nsCOMPtr members.

	:aceman Assignee
	Comment 8 • 6 years ago

Comment on attachment 8990615 [details] [diff] [review]
1474183.patch - even better

Review of attachment 8990615 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is the same, using 'new', just adds a constructor, which I could do, but specifically didn't :)
So if you like it, I'm all for it, just rewrap the initializers into a column as is the style in other such constructors.

	Pulsebot
	Comment 9 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4b1a8163cf20
do not directly memset struct MimePartBufferData, initialize it properly. r=jorgk