Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1754223004:
Move aec_resampler.c to be built using C++ (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 9 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMoved the file aec_resampler.c to be built using C++.
The steps involved were:
1) Change file name to .cc from .c.
2) Update the build files accordingly.
3) Remove the extern header file inclusion.
4) Change the casts in aec_resampler.cc to static_cast
and reinterpret_cast.
The changes are bitexact.
The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding
CL that did the above for aec_core.c, ..., and if the
namespaces in all the aec_core -related files can be changed
at the same time that will simplify things.
BUG=webrtc:5201
Committed: https://crrev.com/88950cfbf9bf3a7a1f2d3353ce9e649e0a68bbf8
Cr-Commit-Position: refs/heads/master@{#11867}
Patch Set 1 : Corrected casts #
Total comments: 4
Patch Set 2 : Changed reinterpret_cast to static_cast #
Messages
Total messages: 27 (13 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Moved file to be built using C++ BUG= ========== to ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG= ==========
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
Description was changed from ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG= ========== to ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG=webrtc:5201 ==========
lgtm https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_resampler.cc (right): https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_resampler.cc:113: AecResampler* obj = reinterpret_cast<AecResampler*>(resampInst); Why not static_cast here?
https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_resampler.cc (right): https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_resampler.cc:113: AecResampler* obj = reinterpret_cast<AecResampler*>(resampInst); On 2016/03/03 15:18:34, the sun wrote: > Why not static_cast here? Not sure why it is needed. The cl upload wanted this type of cast for this. I've used the same in the past when casting the return values of malloc. Googling about this shows that this is discussable though. But I think we should follow what the cl upload script suggests.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754223004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754223004/40001
https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_resampler.cc (right): https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_resampler.cc:113: AecResampler* obj = reinterpret_cast<AecResampler*>(resampInst); On 2016/03/03 19:14:53, peah-webrtc wrote: > On 2016/03/03 15:18:34, the sun wrote: > > Why not static_cast here? > > Not sure why it is needed. The cl upload wanted this type of cast for this. I've > used the same in the past when casting the return values of malloc. Googling > about this shows that this is discussable though. But I think we should follow > what the cl upload script suggests. I think the CL should be consistent with itself. I see no reason for this to be reinterpret_cast and line 69 to be static_cast. I would have used reinterpret_cast for all of them, but am not sure precisely what is the best choice.
On 2016/03/03 19:19:46, the sun wrote: > https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/aec/aec_resampler.cc (right): > > https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/aec/aec_resampler.cc:113: AecResampler* obj = > reinterpret_cast<AecResampler*>(resampInst); > On 2016/03/03 19:14:53, peah-webrtc wrote: > > On 2016/03/03 15:18:34, the sun wrote: > > > Why not static_cast here? > > > > Not sure why it is needed. The cl upload wanted this type of cast for this. > I've > > used the same in the past when casting the return values of malloc. Googling > > about this shows that this is discussable though. But I think we should follow > > what the cl upload script suggests. > > I think the CL should be consistent with itself. I see no reason for this to be > reinterpret_cast and line 69 to be static_cast. > > I would have used reinterpret_cast for all of them, but am not sure precisely > what is the best choice. As far as I've understood, a reinterpret_cast is stronger than static_cast in that it can cast anything to anything so from that perspective it makes sense to use static_cast whenever possible. Looking at the code again I cannot see why reinterpret_cast was suggested by git cl. Furthermore, it definitely works also by changing to static_cast. I think your argument of consistency is very convincing! If the current CL lands, I'll create a new CL changing this cast to static_cast, otherwise I'll patch it to that right away.
On 2016/03/03 20:06:03, peah-webrtc wrote: > On 2016/03/03 19:19:46, the sun wrote: > > > https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/aec/aec_resampler.cc (right): > > > > > https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/aec/aec_resampler.cc:113: AecResampler* obj = > > reinterpret_cast<AecResampler*>(resampInst); > > On 2016/03/03 19:14:53, peah-webrtc wrote: > > > On 2016/03/03 15:18:34, the sun wrote: > > > > Why not static_cast here? > > > > > > Not sure why it is needed. The cl upload wanted this type of cast for this. > > I've > > > used the same in the past when casting the return values of malloc. Googling > > > about this shows that this is discussable though. But I think we should > follow > > > what the cl upload script suggests. > > > > I think the CL should be consistent with itself. I see no reason for this to > be > > reinterpret_cast and line 69 to be static_cast. > > > > I would have used reinterpret_cast for all of them, but am not sure precisely > > what is the best choice. > > As far as I've understood, a reinterpret_cast is stronger than static_cast in > that it can cast anything to anything so from that perspective it makes sense to > use static_cast whenever possible. Looking at the code again I cannot see why > reinterpret_cast was suggested by git cl. Furthermore, it definitely works also > by changing to static_cast. > > I think your argument of consistency is very convincing! If the current CL > lands, I'll create a new CL changing this cast to static_cast, otherwise I'll > patch it to that right away. sgtm!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_resampler.cc (right): https://codereview.webrtc.org/1754223004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_resampler.cc:113: AecResampler* obj = reinterpret_cast<AecResampler*>(resampInst); On 2016/03/03 19:19:46, the sun wrote: > On 2016/03/03 19:14:53, peah-webrtc wrote: > > On 2016/03/03 15:18:34, the sun wrote: > > > Why not static_cast here? > > > > Not sure why it is needed. The cl upload wanted this type of cast for this. > I've > > used the same in the past when casting the return values of malloc. Googling > > about this shows that this is discussable though. But I think we should follow > > what the cl upload script suggests. > > I think the CL should be consistent with itself. I see no reason for this to be > reinterpret_cast and line 69 to be static_cast. > > I would have used reinterpret_cast for all of them, but am not sure precisely > what is the best choice. Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1754223004/#ps60001 (title: "Changed reinterpret_cast to static_cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754223004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754223004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754223004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754223004/60001
Message was sent while issue was closed.
Description was changed from ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG=webrtc:5201 ========== to ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG=webrtc:5201 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG=webrtc:5201 ========== to ========== Moved the file aec_resampler.c to be built using C++. The steps involved were: 1) Change file name to .cc from .c. 2) Update the build files accordingly. 3) Remove the extern header file inclusion. 4) Change the casts in aec_resampler.cc to static_cast and reinterpret_cast. The changes are bitexact. The CL will be followed with another CL where a proper (webrtc) namespace is introduced. The reason for not having it in this CL is that this was missed in the corresponding CL that did the above for aec_core.c, ..., and if the namespaces in all the aec_core -related files can be changed at the same time that will simplify things. BUG=webrtc:5201 Committed: https://crrev.com/88950cfbf9bf3a7a1f2d3353ce9e649e0a68bbf8 Cr-Commit-Position: refs/heads/master@{#11867} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/88950cfbf9bf3a7a1f2d3353ce9e649e0a68bbf8 Cr-Commit-Position: refs/heads/master@{#11867} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
