Issue 29332020: Noissue - Add tracing function 'ToHexLiteral()' (Closed)
DescriptionNoissue - Add tracing function 'ToHexLiteral()'
-----
Another review depends upon this one:
Noissue - Trace life cycle of 'CPluginClass'
https://codereview.adblockplus.org/29332677/
Patch Set 1 #
Total comments: 11
Patch Set 2 : address comments #
Total comments: 7
Patch Set 3 : fix error messages #
Total comments: 2
MessagesTotal messages: 14
Now that we have a functioning x64 build, this function has become ripe for review. I've been using this function to trace life cycle behavior, allowing examination of 'this' pointers, site pointers, and various interface pointers, etc. It's not used at present in any committed code, but we can get it in place now for some future change sets.
https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:266: typedef long long voidIntegral; what about uint64_t and uint32_t instead of `long long` and `long`? https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:283: return ss.str(); Do we really need it so complicated? It seems `basic_ostream& operator<<( const void* value );` does the same thing. The following should be enough, shouldn't it? { std::wstringstream ss; ss << p; return ss.str(); } https://codereview.adblockplus.org/29332020/diff/29332021/test/plugin/DebugTe... File test/plugin/DebugTest.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332021/test/plugin/DebugTe... test/plugin/DebugTest.cpp:32: ASSERT_EQ(expected_nullptr_literal, ToHexLiteral(nullptr)); As before, what about using EXPECT_* instead of ASSERT_* in this file ? https://codereview.adblockplus.org/29332020/diff/29332021/test/plugin/DebugTe... test/plugin/DebugTest.cpp:37: std::unique_ptr<int> p(new int); It can be simply int p = 0; without allocating of a memory and one can pass `&p` into as the function parameter.
https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:283: return ss.str(); On 2015/12/07 11:04:02, sergei wrote: > Do we really need it so complicated? It seems `basic_ostream& operator<<( const > void* value );` does the same thing. The following should be enough, shouldn't > it? > { > std::wstringstream ss; > ss << p; > return ss.str(); > } Alternatively there's also StringCbPrintf (aka sprintf) with "%p". Anyway, I think any of these can be added when we need actual tracing.
https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:266: typedef long long voidIntegral; On 2015/12/07 11:04:02, sergei wrote: > what about uint64_t and uint32_t instead of `long long` and `long`? Done. Do not, however, ask me to remove the 'static_assert' statements. Anything that's got behavior dependent upon a 'reinterpret_cast' needs sanity checking. https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:283: return ss.str(); On 2015/12/07 11:04:02, sergei wrote: > Do we really need it so complicated? It seems `basic_ostream& operator<<( const > void* value );` does the same thing. I doesn't do the same thing. The formatting is different. And, in addition, it's implementation-defined behavior. Insofar as I know, there's no standard for stream formatting of 'void *', nor does Microsoft provide documentation for it. Since you have previously expressed discomfort with relying upon undocumented behavior (see 'IDispatch'), I would think you would accept the use of standard, documented C++ to obtain reliable behavior. https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:283: return ss.str(); On 2015/12/07 12:00:50, Oleksandr wrote: > Alternatively there's also StringCbPrintf (aka sprintf) with "%p". It doesn't save code to use a C-style function that requires explicit buffer management. > Anyway, I > think any of these can be added when we need actual tracing. We already need actual tracing. I have code ready to post for review that's depends upon other reviews that have taken interminably long because of endless dithering and niggling about very small things. For example, this one https://codereview.adblockplus.org/29323561/ which is almost four months old now. That tracing code is still not ready to post because we haven't managed to finish that review yet. https://codereview.adblockplus.org/29332020/diff/29332021/test/plugin/DebugTe... File test/plugin/DebugTest.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332021/test/plugin/DebugTe... test/plugin/DebugTest.cpp:32: ASSERT_EQ(expected_nullptr_literal, ToHexLiteral(nullptr)); On 2015/12/07 11:04:02, sergei wrote: > As before, what about using EXPECT_* instead of ASSERT_* in this file ? There's no difference between these two when there's only one of them in the test. There's no need for niggling consistency on this point. Can we please stop wasting time going over this again and again? https://codereview.adblockplus.org/29332020/diff/29332021/test/plugin/DebugTe... test/plugin/DebugTest.cpp:37: std::unique_ptr<int> p(new int); On 2015/12/07 11:04:02, sergei wrote: > It can be simply int p = 0; without allocating of a memory and one can pass `&p` > into as the function parameter. Done.
https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332021/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:283: return ss.str(); On 2015/12/07 12:00:50, Oleksandr wrote: > Anyway, I > think any of these can be added when we need actual tracing. Actual tracing in new review: https://codereview.adblockplus.org/29332677/
https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:267: typedef uint64_t voidIntegral; What about using of `uintptr_t` but pay attention that its size can be different from the size of void*. https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:268: static_assert(sizeof(void*)==sizeof(voidIntegral),"WIN64: sizeof(long long) is not the same as sizeof(void*)"); Here and below the message string is inconsistent with the actual type which is uint64_t
https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:267: typedef uint64_t voidIntegral; On 2015/12/21 11:14:28, sergei wrote: > What about using of `uintptr_t` but pay attention that its size can be different > from the size of void*. Already changed it once at your suggestion. We are now squarely in bikeshedding territory. https://en.wikipedia.org/wiki/Parkinson's_law_of_triviality Look, we're using 'reinterpret_cast', so the exact type hardly matters much. It needs to be an unsigned integral type and it needs to be the same size as a pointer. By these criteria, 'uintptr_t' isn't even desirable. Its size is >= size of a pointer; it's not guaranteed to be the same size. https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:268: static_assert(sizeof(void*)==sizeof(voidIntegral),"WIN64: sizeof(long long) is not the same as sizeof(void*)"); On 2015/12/21 11:14:28, sergei wrote: > Here and below the message string is inconsistent with the actual type which is > uint64_t Done.
https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:267: typedef uint64_t voidIntegral; On 2015/12/21 12:51:59, Eric wrote: > On 2015/12/21 11:14:28, sergei wrote: > > What about using of `uintptr_t` but pay attention that its size can be > different > > from the size of void*. > > Already changed it once at your suggestion. We are now squarely in bikeshedding > territory. > https://en.wikipedia.org/wiki/Parkinson > > Look, we're using 'reinterpret_cast', so the exact type hardly matters much. It > needs to be an unsigned integral type and it needs to be the same size as a > pointer. By these criteria, 'uintptr_t' isn't even desirable. Its size is >= > size of a pointer; it's not guaranteed to be the same size. According to http://en.cppreference.com/w/cpp/language/reinterpret_cast it should work.
Can we move on? https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:267: typedef uint64_t voidIntegral; On 2015/12/21 13:04:47, sergei wrote: > According to http://en.cppreference.com/w/cpp/language/reinterpret_cast it > should work. OK. Great. I didn't say it wouldn't work. I said it was inferior to what we had. We have explicit 'static_assert' size checks. Very little matters after that.
https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332039/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:267: typedef uint64_t voidIntegral; On 2015/12/21 13:19:59, Eric wrote: > On 2015/12/21 13:04:47, sergei wrote: > > According to http://en.cppreference.com/w/cpp/language/reinterpret_cast it > > should work. > > OK. Great. I didn't say it wouldn't work. I said it was inferior to what we had. > > We have explicit 'static_assert' size checks. Very little matters after that. Less code less bugs, I think here it's proper place when we should use this rule.
On 2015/12/21 13:37:09, sergei wrote: > Less code less bugs, I think here it's proper place when we should use this > rule. The empty program consisting of zero lines of code also has zero defects. You haven't made an argument that your suggestion reduces the code. Changing the type to one that's not guaranteed to be the same length requires more checks to ensure that the conversions work as expected. It's not a net gain.
uintptr_t or not I'm fine with both. I agree this is 'bikeshedding territory' (thanks for the term!). LGTM.
LGTM https://codereview.adblockplus.org/29332020/diff/29332915/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332915/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:277: std::wstring ToHexLiteral(void const* p) How does it happen that in header it's `const void*` but in cpp file it's `void const* p`?
Message was sent while issue was closed.
https://codereview.adblockplus.org/29332020/diff/29332915/src/plugin/PluginDe... File src/plugin/PluginDebug.cpp (right): https://codereview.adblockplus.org/29332020/diff/29332915/src/plugin/PluginDe... src/plugin/PluginDebug.cpp:277: std::wstring ToHexLiteral(void const* p) On 2016/01/05 10:55:27, sergei wrote: > How does it happen that in header it's `const void*` but in cpp file it's `void > const* p`? Not using copy-paste, in all likelihood. I really don't recall; I wrote this code months ago. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
