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

Issue 5163581322559488: Issue 1283 - wrong usage of memset, fix sizeof, make proper initializing (Closed)

Created:
Oct. 8, 2014, 2:49 p.m. by sergei
Modified:
Oct. 17, 2014, 3:35 p.m.
Visibility:
Public.

Patch Set 1 #

Patch Set 2 : fix diff direction #

Patch Set 3 : fix usage of GetLocaleInfo #

Total comments: 7

Patch Set 4 : remove intializating call #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -67 lines) Patch
M src/plugin/AdblockPlusClient.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/NotificationMessage.cpp View 1 3 chunks +3 lines, -3 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 12 chunks +29 lines, -30 lines 1 comment Download
M src/plugin/PluginSettings.cpp View 1 1 chunk +6 lines, -11 lines 0 comments Download
M src/plugin/PluginSystem.cpp View 1 2 3 1 chunk +23 lines, -20 lines 1 comment Download
M src/shared/Communication.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 7
sergei
Oct. 8, 2014, 3:07 p.m. (2014-10-08 15:07:22 UTC) #1
Eric
Sergei, the entire DIFF seems like it's got original and revision reversed. It probably needs ...
Oct. 8, 2014, 4:59 p.m. (2014-10-08 16:59:14 UTC) #2
sergei
On 2014/10/08 16:59:14, Eric wrote: > Sergei, the entire DIFF seems like it's got original ...
Oct. 8, 2014, 9:38 p.m. (2014-10-08 21:38:46 UTC) #3
Eric
Almost all good. http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/plugin/PluginSystem.cpp File src/plugin/PluginSystem.cpp (right): http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/plugin/PluginSystem.cpp#newcode13 src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 9> localeLanguage; I think that ...
Oct. 9, 2014, 1:49 p.m. (2014-10-09 13:49:57 UTC) #4
sergei
http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/plugin/PluginSystem.cpp File src/plugin/PluginSystem.cpp (right): http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/plugin/PluginSystem.cpp#newcode13 src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 9> localeLanguage; On 2014/10/09 13:49:57, Eric wrote: > ...
Oct. 13, 2014, 9:48 a.m. (2014-10-13 09:48:11 UTC) #5
Eric
LGTM http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/plugin/PluginSystem.cpp File src/plugin/PluginSystem.cpp (right): http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/plugin/PluginSystem.cpp#newcode13 src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 9> localeLanguage; On 2014/10/13 09:48:11, sergei wrote: ...
Oct. 14, 2014, 6:33 p.m. (2014-10-14 18:33:36 UTC) #6
Oleksandr
Oct. 17, 2014, 7:09 a.m. (2014-10-17 07:09:07 UTC) #7
LGTM. The comment I have is optional, really. I'm fine with committing this as
is.

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
src/plugin/PluginClass.cpp:905: int classNameLength = GetClassNameW(hTabWnd,
className.data(), className.size());
In other places of the code we use std::wstring in situations like this.
Like:

std::wstring className;
className.resize(MAX_PATH);
GetClassName(..., &className[0]...);

I agree, it might be less pretty, but if we do the same here we could use the
std::wstring's operator "==" instead of wcscmp, and we would be more consistent
across the code, which would be preferable, IMHO. And it would add some
consistence as to how we treat these cases.

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
File src/plugin/PluginSystem.cpp (right):

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
src/plugin/PluginSystem.cpp:14: int res = GetLocaleInfoW(lcid,
LOCALE_SISO639LANGNAME, localeLanguage.data(), localeLanguage.size());
We could use std::wstring for localeLanguage here as well

Powered by Google App Engine
This is Rietveld