Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6012307226230784: Issue #1234 - std::wstring version of UnescapeUrl (Closed)

Created:
Aug. 1, 2014, 1:46 p.m. by Eric
Modified:
Oct. 7, 2014, 2:30 p.m.
Visibility:
Public.

Description

Issue #1234 - std::wstring version of UnescapeUrl Added a std::wstring version of UnescapeUrl as a standalone function. The existing version remains in place; refactoring all calls to this function simultaneously causes too many problems.

Patch Set 1 : #

Total comments: 7

Patch Set 2 : #

Total comments: 17

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -0 lines) Patch
M src/plugin/PluginClientBase.h View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M src/plugin/PluginClientBase.cpp View 1 2 3 4 1 chunk +27 lines, -0 lines 0 comments Download

Messages

Total messages: 15
Eric
Aug. 1, 2014, 1:56 p.m. (2014-08-01 13:56:41 UTC) #1
Oleksandr
http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp#newcode85 src/plugin/PluginClientBase.cpp:85: HRESULT hr = UrlUnescapeW(const_cast<wchar_t *>(url.c_str()), result.get(), & result_length, 0); ...
Aug. 5, 2014, 12:20 p.m. (2014-08-05 12:20:58 UTC) #2
sergei
http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp#newcode81 src/plugin/PluginClientBase.cpp:81: std::unique_ptr<wchar_t> result(new wchar_t[result_length]); // can throw bad_alloc It's better ...
Aug. 5, 2014, 12:53 p.m. (2014-08-05 12:53:03 UTC) #3
Eric
http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp#newcode81 src/plugin/PluginClientBase.cpp:81: std::unique_ptr<wchar_t> result(new wchar_t[result_length]); // can throw bad_alloc On 2014/08/05 ...
Aug. 5, 2014, 1:09 p.m. (2014-08-05 13:09:10 UTC) #4
Eric
All fixed. http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5668600916475904/src/plugin/PluginClientBase.cpp#newcode85 src/plugin/PluginClientBase.cpp:85: HRESULT hr = UrlUnescapeW(const_cast<wchar_t *>(url.c_str()), result.get(), & ...
Aug. 5, 2014, 5:53 p.m. (2014-08-05 17:53:01 UTC) #5
sergei
LGTM
Aug. 6, 2014, 8:15 a.m. (2014-08-06 08:15:00 UTC) #6
Felix Dahlke
http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp#newcode72 src/plugin/PluginClientBase.cpp:72: * When the code can tolerate exceptions better, I ...
Aug. 15, 2014, 3:32 p.m. (2014-08-15 15:32:29 UTC) #7
Eric
http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp#newcode72 src/plugin/PluginClientBase.cpp:72: * When the code can tolerate exceptions better, On ...
Sept. 29, 2014, 11:50 a.m. (2014-09-29 11:50:07 UTC) #8
Felix Dahlke
http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp#newcode83 src/plugin/PluginClientBase.cpp:83: * Casting away const here is harmless because we're ...
Sept. 30, 2014, 9:10 a.m. (2014-09-30 09:10:28 UTC) #9
Eric
http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp#newcode83 src/plugin/PluginClientBase.cpp:83: * Casting away const here is harmless because we're ...
Sept. 30, 2014, 3:22 p.m. (2014-09-30 15:22:21 UTC) #10
Felix Dahlke
http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/plugin/PluginClientBase.cpp#newcode83 src/plugin/PluginClientBase.cpp:83: * Casting away const here is harmless because we're ...
Sept. 30, 2014, 4:08 p.m. (2014-09-30 16:08:16 UTC) #11
Eric
On 2014/09/30 16:08:16, Felix H. Dahlke wrote: > That's up to you of course :) ...
Oct. 1, 2014, 6:55 p.m. (2014-10-01 18:55:14 UTC) #12
Felix Dahlke
On 2014/10/01 18:55:14, Eric wrote: > On 2014/09/30 16:08:16, Felix H. Dahlke wrote: > > ...
Oct. 1, 2014, 7:05 p.m. (2014-10-01 19:05:01 UTC) #13
Oleksandr
LGTM
Oct. 3, 2014, 6:45 p.m. (2014-10-03 18:45:33 UTC) #14
sergei
Oct. 6, 2014, 9:08 a.m. (2014-10-06 09:08:54 UTC) #15
LGTM

http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/...
File src/plugin/PluginClientBase.cpp (right):

http://codereview.adblockplus.org/6012307226230784/diff/5717271485874176/src/...
src/plugin/PluginClientBase.cpp:83: * Casting away const here is harmless
because we're not using the in-place modification mode of UrlUnescape
I would say that this comment could be here or may be even a little bit changed
like
// Casting away const here is required by the method signature
// for the case of in place modification, despite it's not our case
// and url will not be modified.
HRESULT hr = UrlUnescapeW(
/* src */ const_cast<wchar_t*>(url.c_str()),
/* dst */ result.get(),
/* dst size without terminating '\0' */ &result_length,
/* flags, 0 - don't modify src in place */ 0);

But the current version is also OK.

Firstly, const_cast indeed is attracting the attention, each time I see
const_cast I start to suspect something bad is going here.

Also I don't remember the entire Windows API and for this particular function
each time I have to read its documentation, especially here are attracting
const_cast and magic number 0. Since anyway I personally have to read the
documentation the comment seems to be redundant. It would be great if I don't
need to open the reference of the function or at least it can simplify the
reading if it explains the reason of the decision, so the everything I need is
mere to check whether it's truth or not. BTW, in most cases I would like to
avoid comments for arguments by proper naming.

Powered by Google App Engine
This is Rietveld