Issue 10169005: Windows specific changes. All tests passing on Windows. (Closed)
|
Created:
April 9, 2013, 2:22 a.m. by Oleksandr Modified:
April 12, 2013, 1:38 p.m. Visibility:
Public. |
DescriptionWindows specific changes. All tests passing on Windows.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Previous patch comments addressed #
Total comments: 5
Patch Set 3 : Misscommunication fix #
Total comments: 1
Patch Set 4 : Comments address. WIN32 in #ifdef and whitespace #
MessagesTotal messages: 16
http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:7: #ifdef _WIN32 || _WIN64 I would prefer #if _MSC_VER_ here - it's cleaner, just in case we ever try compiling via gcc on Windows. http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:10: #include <functional> Are these two headers really required on Windows? What for? http://codereview.adblockplus.org/10169005/diff/1/src/GlobalJsObject.cpp File src/GlobalJsObject.cpp (right): http://codereview.adblockplus.org/10169005/diff/1/src/GlobalJsObject.cpp#newc... src/GlobalJsObject.cpp:54: function->Call(function, functionArguments.size(), &argv); No, this isn't right. You *copy* the first argument and then use the pointer to this copy. What if there is more than one argument? Ideally we would use functionArguments.data() here but that's C++11 again. So if you really want to handle empty vectors properly I would go with: v8::Handle<v8::Value> *argv = functionArguments.empty() ? 0 : &(functionArguments.front());
http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:10: #include <functional> On 2013/04/09 05:47:07, Wladimir Palant wrote: > Are these two headers really required on Windows? What for? <cctype> - for isspace <functional> - for not1 and ptr_fun http://codereview.adblockplus.org/10169005/diff/1/src/GlobalJsObject.cpp File src/GlobalJsObject.cpp (right): http://codereview.adblockplus.org/10169005/diff/1/src/GlobalJsObject.cpp#newc... src/GlobalJsObject.cpp:54: function->Call(function, functionArguments.size(), &argv); Ah. You're right. My bad. It's not that I *wanted* to explicitly handle the empty vectors case, its just that the test fails otherwise. On 2013/04/09 05:47:07, Wladimir Palant wrote: > No, this isn't right. You *copy* the first argument and then use the pointer to > this copy. What if there is more than one argument? > > Ideally we would use functionArguments.data() here but that's C++11 again. So if > you really want to handle empty vectors properly I would go with: > > v8::Handle<v8::Value> *argv = functionArguments.empty() ? 0 : > &(functionArguments.front());
http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:10: #include <functional> On 2013/04/09 05:47:07, Wladimir Palant wrote: > Are these two headers really required on Windows? What for? Then they should be added at the top of FilterEngine.cpp, right after algorithm. No need for it to be Windows-only either, sounds like they really should be there on Linux/Android as well.
http://codereview.adblockplus.org/10169005/diff/6002/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10169005/diff/6002/src/FilterEngine.cpp#new... src/FilterEngine.cpp:2: #include <memory> Miscommunication here apparently - #include <memory> is actually needed in the header file and that one needs #if defined(_MSC_VER) (still #include <tr1/memory> for gcc). Only cctype and functional should be included here.
http://codereview.adblockplus.org/10169005/diff/6002/include/AdblockPlus/Filt... File include/AdblockPlus/FilterEngine.h (left): http://codereview.adblockplus.org/10169005/diff/6002/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:7: #ifdef _WIN32 || _WIN64 Why not just "#ifdef WIN32"? That's what I always used, and I just verified that it works for x64 in MSVC. http://codereview.adblockplus.org/10169005/diff/6002/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:10: #include <functional> Is cctype and functional really needed in the header? We should only include in header what we absolutely have to. http://codereview.adblockplus.org/10169005/diff/6002/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10169005/diff/6002/src/FilterEngine.cpp#new... src/FilterEngine.cpp:2: #include <memory> On 2013/04/09 08:00:10, Wladimir Palant wrote: > Miscommunication here apparently - #include <memory> is actually needed in the > header file and that one needs #if defined(_MSC_VER) (still #include > <tr1/memory> for gcc). Only cctype and functional should be included here. Like I said in the header, "#ifdef WIN32" should work fine. http://codereview.adblockplus.org/10169005/diff/6002/src/GlobalJsObject.cpp File src/GlobalJsObject.cpp (right): http://codereview.adblockplus.org/10169005/diff/6002/src/GlobalJsObject.cpp#n... src/GlobalJsObject.cpp:53: v8::Handle<v8::Value> *argv = functionArguments.empty() ? 0 : &(functionArguments.front()); Declaration type should be "v8::Handle<v8::Value>*" for consistency (just a whitespace issue). Should also be *const, since I'm a const nazi. Why is this necessary? size() and operator[]() should work fine... I've seen Wladimir's comment about this in a mail but I don't see it here. Anyway, passing a pointer here is exactly what we want, an std::vector is guaranteed to store it's value in a single continuous array internally, so taking the pointer to the first element is how you convert a std::vector to an array. In constant time, too :)
Added some comments to the initial changes since the latest diffs didn't seem to include these, or the comments to it. http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:10: #include <functional> On 2013/04/09 06:43:48, Wladimir Palant wrote: > On 2013/04/09 05:47:07, Wladimir Palant wrote: > > Are these two headers really required on Windows? What for? > > Then they should be added at the top of FilterEngine.cpp, right after algorithm. > No need for it to be Windows-only either, sounds like they really should be > there on Linux/Android as well. If it compiles on Linux/Android without them, they shouldn't be there IMO. Unused includes make things pretty complex over time. http://codereview.adblockplus.org/10169005/diff/1/src/GlobalJsObject.cpp File src/GlobalJsObject.cpp (right): http://codereview.adblockplus.org/10169005/diff/1/src/GlobalJsObject.cpp#newc... src/GlobalJsObject.cpp:54: function->Call(function, functionArguments.size(), &argv); On 2013/04/09 05:54:21, Oleksandr wrote: > Ah. You're right. My bad. > > It's not that I *wanted* to explicitly handle the empty vectors case, its just > that the test fails otherwise. > > > On 2013/04/09 05:47:07, Wladimir Palant wrote: > > No, this isn't right. You *copy* the first argument and then use the pointer > to > > this copy. What if there is more than one argument? > > > > Ideally we would use functionArguments.data() here but that's C++11 again. So > if > > you really want to handle empty vectors properly I would go with: > > > > v8::Handle<v8::Value> *argv = functionArguments.empty() ? 0 : > > &(functionArguments.front()); > Ah, just saw the original change here, didn't show up in the other diff so I thought you had just replaced size() with empty() and the copy with a pointer. We can actually just pass 0 here if it's empty, so I vote for: function->Call(function, functionArguments.size(), functionArguments.size() ? functionArguments[0] : 0);
LGTM
http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:10: #include <functional> On 2013/04/09 08:18:06, Felix H. Dahlke wrote: > If it compiles on Linux/Android without them, they shouldn't be there IMO. > Unused includes make things pretty complex over time. We are using functions from these headers. It merely compiles because these headers are included somewhere, probably in algorithm.h. So they are by no means unused, if we omit them we expose ourselves to issues due to internal compiler changes.
On 2013/04/09 08:38:32, Wladimir Palant wrote: > http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... > File include/AdblockPlus/FilterEngine.h (right): > > http://codereview.adblockplus.org/10169005/diff/1/include/AdblockPlus/FilterE... > include/AdblockPlus/FilterEngine.h:10: #include <functional> > On 2013/04/09 08:18:06, Felix H. Dahlke wrote: > > If it compiles on Linux/Android without them, they shouldn't be there IMO. > > Unused includes make things pretty complex over time. > > We are using functions from these headers. It merely compiles because these > headers are included somewhere, probably in algorithm.h. So they are by no means > unused, if we omit them we expose ourselves to issues due to internal compiler > changes. AFAIK, the STL specifies what needs to be included in what header, my hunch is that MS didn't take that too seriously here. But well, I suppose a few unused headers won't kill us...
Since things got a bit messy with the inconsistent patch sets and I ended up commenting on issues that were already fixed, here's what I would still like to see changed: This nit pick: http://codereview.adblockplus.org/10169005/diff2/1:6002/src/GlobalJsObject.cpp And: http://codereview.adblockplus.org/10169005/diff/10001/include/AdblockPlus/Fil... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10169005/diff/10001/include/AdblockPlus/Fil... include/AdblockPlus/FilterEngine.h:7: #ifdef _MSC_VER I guess my comment got lost since the patch sets seem to be off, but this should be "#ifdef WIN32". Works fine on Windows 64 and also in compilers other than MSVC.
LGTM, ignoring the project file changes :)
On 2013/04/10 08:09:04, Felix H. Dahlke wrote: > LGTM, ignoring the project file changes :) Cool. I have just added an x64 build target to make sure it builds on x64 as well. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
