Issue 5979857238360064: Issues #1163, #1173 - refactor CPluginUserSettings (Closed)
DescriptionIssues #1163, #1173 - refactor CPluginUserSettings
Added exception handlers around the bodies of entry points that need them.
Document the absence of handlers where they're not needed.
Refactored the control flows in the IDispatch::Invoke implementation of
CPluginUserSettings. Eliminate linear searches of string comparisons in both
GetIDsOfNames() and Invoke(). Replaced the linear search (a loop) in
GetIDsOfNames() by an unordered_map lookup. Replaced the linear search (an
if-else sequence) in Invoke() by a switch statement.
The code within the switch statement differs mostly in indentation from the
previous version. Exceptions:
* Fixed defect within case dispatchID_GetMessage. The type check for the
VARIANT argument was missing.
* Fixed a defect in case dispatchID_IsUpdate. The VARIANT_BOOL return value
had been returning a C++ bool.
* Swapped terms in yoda expressions.
* Added braces to conditions.
Patch Set 1 : #
Total comments: 11
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 1
Patch Set 4 : rebased to current tip #Patch Set 5 : Fixed defects involving pVarResult #
Total comments: 14
Patch Set 6 : nit fixes #
Total comments: 1
Patch Set 7 : rebase + add one final space character #
MessagesTotal messages: 19
Sorry, but the diff facility within this review tool does not show the changes only in indentation well.
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); It's outside the scope of this review, but since we are already re-factoring this, I wouldn't mind to get rid of that really long Invoke method below. I am thinking of something like this typedef (HRESULT JavaScriptMethodInvoke)(DISPPARAMS* pDispparams, VARIANT* pVarResult, EXCEPINFO* pExcepinfo, UINT* pArgErr) std::unordered_map<DISPID, JavaScriptMethodInvoke> m_methods; Then for each DISPID we could have a separate method, and all Invoke would do would just call something like: return m_methods[dispidMember]->second(pDispparams, pVarResult, pExcepinfo, pArgErr) Shall we put this into our issues list? http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:59: // Entry point, but no exception handler; nothing here can throw This comment and all the others concerning the exception safety are useful in this review, but would be weird to have in the actual code, IMHO. http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.h (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.h:3: #pragma once Do we really need both #pragma once and #ifndef above?
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); On 2014/08/17 22:52:01, Oleksandr wrote: > [...] I wouldn't mind to get rid of that really long Invoke method below. > [...] > Then for each DISPID we could have a separate method, and all Invoke would do > would just call something like: > return m_methods[dispidMember]->second(pDispparams, pVarResult, pExcepinfo, > pArgErr) Discussion points. 1. I wouldn't mind breaking up the Invoke code so that the central functionality of each method is implemented in its own function. 2. I would prefer to centralize all the type conversion and validation functions here so that the individual functions have interfaces with natural C++ types and not the Microsoft API conventions. 2a. For example, only some of the functions need to return a result. Defining 'pVarResult' as an argument for such functions loses the opportunity for the function signature to signify what it does. > Shall we put this into our issues list? It would be a low priority issue in every case I can think of. I don't see a need to plan to rewrite it yet. http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:59: // Entry point, but no exception handler; nothing here can throw On 2014/08/17 22:52:01, Oleksandr wrote: > This comment and all the others concerning the exception safety are useful in > this review, but would be weird to have in the actual code, IMHO. I would like to have some kind of documentation in the code that identifies each of the entry points in some way that can be searched with a regular expression. This is to allow code audit for issue #1173. It's a separate issue (for me) whether those comments persist in the code after #1173 is done. Personally, I'd leave them there because they have non-ordinary interactions with the external world. I'd prefer an in-language specification but that's not available, so I'll settle for a comment. (Note: C++11 generalized attributes could work, but they are not supported even in VS 2013.) I'm open with how to write such comments. Also, I'm also open to putting in a try-catch statement even though nothing can throw. That would remove the need for the second half of the comment. http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.h (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.h:3: #pragma once On 2014/08/17 22:52:01, Oleksandr wrote: > Do we really need both #pragma once and #ifndef above? It's not needed for correctness; the #ifndef guard does that. #once has better performance in the compiler, if what I've read online is true. The idea is that compiler doesn't even read the rest of the file, saving a bunch of I/O time during compilation. I'll also not that many (most?) of the Microsoft system headers use this same implementation pattern, with both #ifndef guards and #pragma once.
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); Creating a low priority issue in our issue tracker would enable developers from outside the team to just go and do it, if the feel like it. On 2014/09/29 18:45:41, Eric wrote: > On 2014/08/17 22:52:01, Oleksandr wrote: > > [...] I wouldn't mind to get rid of that really long Invoke method below. > > [...] > > Then for each DISPID we could have a separate method, and all Invoke would do > > would just call something like: > > return m_methods[dispidMember]->second(pDispparams, pVarResult, pExcepinfo, > > pArgErr) > > Discussion points. > 1. I wouldn't mind breaking up the Invoke code so that the central functionality > of each method is implemented in its own function. > 2. I would prefer to centralize all the type conversion and validation functions > here so that the individual functions have interfaces with natural C++ types and > not the Microsoft API conventions. > 2a. For example, only some of the functions need to return a result. Defining > 'pVarResult' as an argument for such functions loses the opportunity for the > function signature to signify what it does. > > > Shall we put this into our issues list? > > It would be a low priority issue in every case I can think of. I don't see a > need to plan to rewrite it yet. http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:59: // Entry point, but no exception handler; nothing here can throw In that case, it would seem less intrusive to have these comments right before the method declaration, IMHO. On 2014/09/29 18:45:41, Eric wrote: > On 2014/08/17 22:52:01, Oleksandr wrote: > > This comment and all the others concerning the exception safety are useful in > > this review, but would be weird to have in the actual code, IMHO. > > I would like to have some kind of documentation in the code that identifies each > of the entry points in some way that can be searched with a regular expression. > This is to allow code audit for issue #1173. > > It's a separate issue (for me) whether those comments persist in the code after > #1173 is done. Personally, I'd leave them there because they have non-ordinary > interactions with the external world. I'd prefer an in-language specification > but that's not available, so I'll settle for a comment. (Note: C++11 generalized > attributes could work, but they are not supported even in VS 2013.) > > I'm open with how to write such comments. > > Also, I'm also open to putting in a try-catch statement even though nothing can > throw. That would remove the need for the second half of the comment. http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.h (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.h:3: #pragma once I have never seen both versions used simultaniously, tbh. Nor do I understand why would we need both still. I think it's best to just use #ifndef On 2014/09/29 18:45:41, Eric wrote: > On 2014/08/17 22:52:01, Oleksandr wrote: > > Do we really need both #pragma once and #ifndef above? > > It's not needed for correctness; the #ifndef guard does that. > > #once has better performance in the compiler, if what I've read online is true. > The idea is that compiler doesn't even read the rest of the file, saving a bunch > of I/O time during compilation. I'll also not that many (most?) of the Microsoft > system headers use this same implementation pattern, with both #ifndef guards > and #pragma once.
http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.cpp:34: m.emplace(L"GetMessage", dispatchID_GetMessage); On 2014/10/02 20:36:53, Oleksandr wrote: > Creating a low priority issue in our issue tracker would enable developers from > outside the team to just go and do it, if the feel like it. I wrote an issue for the slightly larger task of cleaning up the Invoke implementations generally. See https://issues.adblockplus.org/ticket/1163 The subtask of breaking out each dispatch ID to its own function is in there. Let me know if this is sufficient. http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... File src/plugin/PluginUserSettings.h (right): http://codereview.adblockplus.org/5979857238360064/diff/5668600916475904/src/... src/plugin/PluginUserSettings.h:3: #pragma once On 2014/10/02 20:36:53, Oleksandr wrote: > I have never seen both versions used simultaniously, tbh. All four of the ATL library use this pattern, for example. atlbase.h, atlstr.h, atltypes.h, atlcom.h > Nor do I understand > why would we need both still. I think it's best to just use #ifndef We don't need both. #pragma once is only an optimization to improve compile times. Nonetheless, I'll get rid of it. The code here has rotted so badly I can't get a patch to apply. I'll be rewriting it. You'll see the code in the next iteration.
New patch set updates the code to account for changes since review was created.
http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.cpp:148: if (!pDispparams || !pExcepinfo) pExcepinfo can be nullptr, it's not an error. http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.cpp:169: if (pVarResult) Wouldn't it better here and below to have if (!pVarResult) { return E_POINTER; } CComBSTR key = pDispparams->rgvarg[0].bstrVal; ... BTW, sometimes there is even no test whether pVarResult is not nullptr. http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... File src/plugin/PluginUserSettings.h (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.h:1: #ifndef PLUGIN_USER_SETTINGS_H What does it improve? http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.h:14: CPluginUserSettings() // = default; If the implementation of the ctr is default and can be generated by the compiler and there is no reason to have it (there are no other ctrs) then let's just remove it.
http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.cpp:148: if (!pDispparams || !pExcepinfo) On 2015/01/09 15:42:54, sergei wrote: > pExcepinfo can be nullptr, it's not an error. I'm mostly trying to add an exception handler in this function, but I don't mind addressing a few small defects at the same time. I just checked and we're not even referencing pExcepinfo, much less properly using it for error conditions. http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.cpp:169: if (pVarResult) On 2015/01/09 15:42:54, sergei wrote: > Wouldn't it better here and below to have > if (!pVarResult) > { > return E_POINTER; > } No. It's not an error to ignore the return value. If pVarResult is null, there's no way to return a value from the invoked method, and thus no point in executing such a method unless it has side effects. Apropos, see below for SetLanguage, which is executed solely for its side effects and does not return a value. We could use this snippet if (!pVarResult) { return S_OK; } // ... if we want to remove a level of indentation for the code below it, but that's a high degree of change that I'd prefer not do in the present change set. http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... File src/plugin/PluginUserSettings.h (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.h:1: #ifndef PLUGIN_USER_SETTINGS_H On 2015/01/09 15:42:54, sergei wrote: > What does it improve? Consistency. All the other headers use #ifndef guards. Personally, I'd rather be using both an #ifndef guard (for correctness) and #pragma once (for efficiency). That's what the ATL headers do, for example. I raised this issue elsewhere and got some pushback from Oleksandr. http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.h:14: CPluginUserSettings() // = default; On 2015/01/09 15:42:54, sergei wrote: > If the implementation of the ctr is default and can be generated by the compiler > and there is no reason to have it (there are no other ctrs) then let's just > remove it. OK. This class shouldn't be copyable or movable, but VS2012 doesn't support the "= delete" syntax, nor the "= default" one that would appear here.
New patch set. Also added conformant license headers. See https://adblockplus.org/en/license-headers
http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.cpp:169: if (pVarResult) On 2015/01/09 16:25:24, Eric wrote: > On 2015/01/09 15:42:54, sergei wrote: > > Wouldn't it better here and below to have > > if (!pVarResult) > > { > > return E_POINTER; > > } > > No. It's not an error to ignore the return value. If pVarResult is null, there's > no way to return a value from the invoked method, and thus no point in executing > such a method unless it has side effects. > > Apropos, see below for SetLanguage, which is executed solely for its side > effects and does not return a value. > > We could use this snippet > if (!pVarResult) > { > return S_OK; > } > // ... > if we want to remove a level of indentation for the code below it, but that's a > high degree of change that I'd prefer not do in the present change set. What is about GetAppLocale and below? We should test whether `pVarResult` is not nullptr. http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/... src/plugin/PluginUserSettings.cpp:2: * This file is part of Adblock Plus <http://adblockplus.org/>, As I see we use `https` in other files, although on the web site there is `http`.
> http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/... > File src/plugin/PluginUserSettings.cpp (right): > > http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/... > src/plugin/PluginUserSettings.cpp:2: * This file is part of Adblock Plus > <http://adblockplus.org/>, > As I see we use `https` in other files, although on the web site there is > `http`. Felix was the one that put up that page. It's probably the right thing to publish the https: address, but Felix should be the one change it.
On 2015/02/02 13:51:45, Eric wrote: > > > http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/... > > File src/plugin/PluginUserSettings.cpp (right): > > > > > http://codereview.adblockplus.org/5979857238360064/diff/5728116278296576/src/... > > src/plugin/PluginUserSettings.cpp:2: * This file is part of Adblock Plus > > <http://adblockplus.org/>, > > As I see we use `https` in other files, although on the web site there is > > `http`. > > Felix was the one that put up that page. It's probably the right thing to > publish the https: address, but Felix should be the one change it. Yup, my bad - should be https. I'll change the header template.
Two new patch sets. The first is a rebase to tip, to account for a committed change set that fixed up some yoda conditions. I had already taken care of most of them, but four remained. The second is to correct some pre-existing defects involved not checking the result pointer for null before dereferencing it. http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5689792285114368/src/... src/plugin/PluginUserSettings.cpp:169: if (pVarResult) On 2015/01/30 16:01:40, sergei wrote: > What is about GetAppLocale and below? We should test whether `pVarResult` is not > nullptr. Indeed, there were four defect where 'pVarResult' was not checked against nullptr before dereferencing. Those are fixed in patch set 5. For the record, those defects were not introduced in this refactoring, but we shall take the opportunity to fix them while we're doing this one.
BUMP. The latest patch has languished for a week now with no comment from anybody. Don't we have an agreement with each other to do reviews in a timely fashion?
Looks great in all. Just some minor style nits from me. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:91: *ppvObj = static_cast<void *>(this); NIT: No space after void http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:141: if (item==methodIndex.end()) NIT: Space before and after == http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:224: int indx = pDispparams->rgvarg[0].lVal; NIT: Might be a good time to fix the typo: "index" http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:233: int curIndx = 0; NIT: Typo here as well http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:234: for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it) Nit: Space after 'for' http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:261: int indx = pDispparams->rgvarg[0].lVal; Nit: Another indx typo leftover and more below http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:271: for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it) Nit: Space after 'for'
All nits fixed. One nexus of really awful code not fixed as out of scope for the present change set. It would properly requires new unit tests, etc. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:91: *ppvObj = static_cast<void *>(this); On 2015/02/12 04:46:22, Oleksandr wrote: > NIT: No space after void Done. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:141: if (item==methodIndex.end()) On 2015/02/12 04:46:22, Oleksandr wrote: > NIT: Space before and after == Done. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:224: int indx = pDispparams->rgvarg[0].lVal; On 2015/02/12 04:46:22, Oleksandr wrote: > NIT: Might be a good time to fix the typo: "index" OK. Done. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:233: int curIndx = 0; On 2015/02/12 04:46:22, Oleksandr wrote: > NIT: Typo here as well Done. I should point out that the algorithm here is insane. GetFilterLanguageTitleList() is only used here, and it's constructing and returning std::map rather than std::vector. It's only ever numerically indexed, yet we're using a linear search to locate the container element. The associative index that std::map provides is never used. Using std::vector would be less code, more efficient in time, more efficient in space, and easier to understand. I'm not fixing it with this change set as out of scope. But ... I did have to vent. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:234: for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it) On 2015/02/12 04:46:22, Oleksandr wrote: > Nit: Space after 'for' Done. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:261: int indx = pDispparams->rgvarg[0].lVal; On 2015/02/12 04:46:22, Oleksandr wrote: > Nit: Another indx typo leftover and more below Done. http://codereview.adblockplus.org/5979857238360064/diff/5659645909663744/src/... src/plugin/PluginUserSettings.cpp:271: for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it) On 2015/02/12 04:46:22, Oleksandr wrote: > Nit: Space after 'for' Done.
Just one small nit. LGTM otherwise. http://codereview.adblockplus.org/5979857238360064/diff/5743868070854656/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5979857238360064/diff/5743868070854656/src/... src/plugin/PluginUserSettings.cpp:450: catch(...) Nit: Space between catch and (
LGTM
Patch set 7 differs from patch set 6 only by a single whitespace character. Rebase makes it look different. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
