Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943833002:
Use constexpr to avoid a static initializer (Closed)
|
Created:
3 years, 6 months ago by brucedawson Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse constexpr to avoid a static initializer
Floating-point calculations are not guaranteed to happen at compile time
unless you force the issue with constexpr. This initializer was found
by running tools\win\static_initializers on a canary build
chrome_child.dll. constexpr was added to kSilenceRms for consistency.
BUG=chromium:341941
Review-Url: https://codereview.webrtc.org/2943833002
Cr-Commit-Position: refs/heads/master@{#18684}
Committed: https://chromium.googlesource.com/external/webrtc/+/fde21162889a2f053bed72a962cbefbff9bf7369
Patch Set 1 #Patch Set 2 : Remove unneeded static #
Total comments: 3
Patch Set 3 : Restore static and more constexpr #Messages
Total messages: 18 (11 generated)
Description was changed from ========== Use constexpr to avoid a static initializer Floating-point calculations are not guaranteed to happen at compile time unless you force the issue with constexpr. This initializer was found by running tools\win\static_initializers on a canary build chrome_child.dll. BUG=chromium:341941 ========== to ========== Use constexpr to avoid a static initializer Floating-point calculations are not guaranteed to happen at compile time unless you force the issue with constexpr. This initializer was found by running tools\win\static_initializers on a canary build chrome_child.dll. Also remove 'static' because that is implied by const and constexpr. BUG=chromium:341941 ==========
brucedawson@chromium.org changed reviewers: + henrik.lundin@webrtc.org
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Minor tweak to improve VC++ code-gen. PTAL
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2943833002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/vad/vad_audio_proc.cc (right): https://codereview.webrtc.org/2943833002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/vad/vad_audio_proc.cc:36: constexpr float kFrequencyResolution = That constexpr (and const) imply internal linkage is probably not well enough known---consider e.g. that this declaration and that on the next line were both "static const". Could you put it in an unnamed namespace to make the internal linkage obvious? (Also, to reduce eyesore, give the kSilenceRms declaration the same treatment if you want.)
Thanks for you patch. Please, update along the lines that kwiberg points out. https://codereview.webrtc.org/2943833002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/vad/vad_audio_proc.cc (right): https://codereview.webrtc.org/2943833002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/vad/vad_audio_proc.cc:36: constexpr float kFrequencyResolution = On 2017/06/17 22:54:30, kwiberg-webrtc wrote: > That constexpr (and const) imply internal linkage is probably not well enough > known---consider e.g. that this declaration and that on the next line were both > "static const". Could you put it in an unnamed namespace to make the internal > linkage obvious? > > (Also, to reduce eyesore, give the kSilenceRms declaration the same treatment if > you want.) What kwiberg@ says.
Description was changed from ========== Use constexpr to avoid a static initializer Floating-point calculations are not guaranteed to happen at compile time unless you force the issue with constexpr. This initializer was found by running tools\win\static_initializers on a canary build chrome_child.dll. Also remove 'static' because that is implied by const and constexpr. BUG=chromium:341941 ========== to ========== Use constexpr to avoid a static initializer Floating-point calculations are not guaranteed to happen at compile time unless you force the issue with constexpr. This initializer was found by running tools\win\static_initializers on a canary build chrome_child.dll. constexpr was added to kSilenceRms for consistency. BUG=chromium:341941 ==========
https://codereview.webrtc.org/2943833002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/vad/vad_audio_proc.cc (right): https://codereview.webrtc.org/2943833002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/vad/vad_audio_proc.cc:36: constexpr float kFrequencyResolution = I added constexpr to kSilenceRms, to make it appear more consistent. I agree that removing static may cause some confusion so I put it back. I can't convince myself to put the declarations in an anonymous namespace that I know is unnecessary so I think restoring static is the right compromise.
Thanks! LGTM
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497979631530990,
"parent_rev": "26b16f7d52a486ed9daa64e9ce2a67f51c63307b", "commit_rev":
"fde21162889a2f053bed72a962cbefbff9bf7369"}
Message was sent while issue was closed.
Description was changed from ========== Use constexpr to avoid a static initializer Floating-point calculations are not guaranteed to happen at compile time unless you force the issue with constexpr. This initializer was found by running tools\win\static_initializers on a canary build chrome_child.dll. constexpr was added to kSilenceRms for consistency. BUG=chromium:341941 ========== to ========== Use constexpr to avoid a static initializer Floating-point calculations are not guaranteed to happen at compile time unless you force the issue with constexpr. This initializer was found by running tools\win\static_initializers on a canary build chrome_child.dll. constexpr was added to kSilenceRms for consistency. BUG=chromium:341941 Review-Url: https://codereview.webrtc.org/2943833002 Cr-Commit-Position: refs/heads/master@{#18684} Committed: https://chromium.googlesource.com/external/webrtc/+/fde21162889a2f053bed72a96... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/fde21162889a2f053bed72a96... |
