Closed Bug 1307812 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: away, Unassigned)

References

Details

(Whiteboard: tpi:+)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Remove EmptyMSG() (obsolete) — Splinter Review
Attachment #8798066 - Flags: review?(masayuki)
Priority: -- → P2
Whiteboard: tpi:+
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.
Attachment #8798066 - Flags: review?(masayuki) → review-
Comment on attachment 8798066 [details] [diff] [review]
Remove EmptyMSG()

And also, you don't initialize the struct before use.
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));

?
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.
Attachment #8798066 - Attachment is obsolete: true
Attachment #8798291 - Flags: review?(masayuki)
Attachment #8798291 - Flags: review?(masayuki) → review+
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2417f5e29a8e
Re-work IsEmptyMSG() to avoid taking the address of a temporary. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/2417f5e29a8e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.