1307812 - Taking the address of a temporary EmptyMSG() in KeyboardLayout.h

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[(Away)]

clang-cl fails to compile this header (though apparently MSVC doesn't mind):

 1:54.66 d:/build/msys/s/central/widget/windows/KeyboardLayout.h(576,27):  error(clang): taking the address of a temporary object of type 'const MSG' (aka 'const tagMSG') [-Waddress-of-temporary]
 1:54.66     return !memcmp(&aMSG, &EmptyMSG(), sizeof(MSG));
 1:54.66                           ^~~~~~~~~~~
 1:54.66 1 error generated.
 1:54.66 clang-cl.EXE: warning: falling back to C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\cl.exe

The implementation of EmptyMSG() with a runtime memset seems like overkill. It should be possible to declare a global and take advantage of the fact that it is zero-initialized.

Comment 1

[(Away)]

Attachment: Remove EmptyMSG()

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

Comment on attachment 8798066 [details] [diff] [review]
Remove EmptyMSG()

>+static const MSG sEmptyMSG;

You can define this in NativeKey. Than, you can keep IsEmptyMSG() being in the header.

>+// static
>+bool NativeKey::IsEmptyMSG(const MSG& aMSG)
>+{

Although, this should be in header file, you should write this as:

// static
bool
NativeKey::IsEmptyMSG(...)
{

Otherwise, looks okay to me.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

Comment on attachment 8798066 [details] [diff] [review]
Remove EmptyMSG()

And also, you don't initialize the struct before use.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

Cannot you to fix this with simpler patch, like

> return !memcmp(&aMSG, &EmptyMSG(), sizeof(MSG));

to

> MSG emptyMSG = EmptyMSG();
> return !memcmp(&aMSG, &emptyMSG, sizeof(MSG));

?

Comment 5

[(Away)]

Sure, that could fix the build error. But EmptyMSG() is doing a lot of unnecessary work -- sInitialized pattern, runtime memset -- when you can get a zero-initialized object from the language for free. I thought it would be better to refactor in such a way that EmptyMSG() is no longer needed.

You are right that I can simplify by keeping some things in the header file. Let's try a new patch.

Comment 6

[(Away)]

Attachment: Remove EmptyMSG()

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2417f5e29a8e
Re-work IsEmptyMSG() to avoid taking the address of a temporary. r=masayuki

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2417f5e29a8e