Code review comment for lp://staging/~daniel-nichter/drizzle/query-log-plugin

Cool. I will review it, and then Jenkins it.

On Sat, Jul 30, 2011 at 5:41 PM, Daniel Nichter <email address hidden> wrote:
> It occurred to me that Boost formatter is overkill.  So, the latest code (https://code.launchpad.net/~daniel-nichter/drizzle/query-log-plugin) removes that and instead uses standard C++ iostream.  I think this is a lot better because it's simpler and native and pure OO (unlike previous implementation using C-style open, file descriptors, etc.).  Perhaps there's some technical reason for not using it?  If not, then I believe the current code passes valgrind: http://jenkins.drizzle.org/job/Drizzle-param-valgrind/301/  The error is some po thing; I think that's easy to fix?  It also passes tests, http://jenkins.drizzle.org/job/drizzle-param/871/, again minus some po errors.
>
> -Daniel
>
> Le 14 juil. 2011 à 06:41, Joe Daly a écrit :
>
>> On Tue, Jul 12, 2011 at 7:33 PM, Daniel Nichter <email address hidden> wrote:
>>
>>> Thanks for working on this.  I'm not sure what to do about the valgrind
>>> warnings; perhaps you can point me in the correct direction.  For example:
>>>
>>> 464 bytes in 13 blocks are possibly lost in loss record 41 of 52
>>>   at 0x4C28973: operator new(unsigned long) (vg_replace_malloc.c:261)
>>>   by 0x70B9E98: std::string::_Rep::_S_create(unsigned long, unsigned long,
>>> std::allocator<char> const&) (in /usr/lib/libstdc++.so.6.0.14)
>>>   by 0x70BA0BD: std::string::_M_mutate(unsigned long, unsigned long,
>>> unsigned long) (in /usr/lib/libstdc++.so.6.0.14)
>>>   by 0x70BA28B: std::string::_M_replace_safe(unsigned long, unsigned long,
>>> char const*, unsigned long) (in /usr/lib/libstdc++.so.6.0.14)
>>>   by 0x70BBA8C: std::string::replace(unsigned long, unsigned long, char
>>> const*, unsigned long) (in /usr/lib/libstdc++.so.6.0.14)
>>>   by 0xC349952: boost::basic_format<char, std::char_traits<char>,
>>> std::allocator<char> >::parse(std::string const&) (basic_string.h:1493)
>>>   by 0xC3429D6: QueryLoggerFile::QueryLoggerFile() (file.cc:41)
>>>   by 0xC336478:
>>> drizzle_plugin::init_options(drizzled::module::option_context&)
>>> (module.cc:159)
>>>   by 0x4EFBA1: drizzled::plugin_load_list(drizzled::module::Registry&,
>>> drizzled::memory::Root*, std::set<std::string, std::less<std::string>,
>>> std::allocator<std::string> > const&,
>>> boost::program_options::options_description&, bool) (loader.cc:502)
>>>   by 0x4F0F53: drizzled::plugin_init(drizzled::module::Registry&,
>>> boost::program_options::options_description&) (loader.cc:291)
>>>   by 0x44ED1B:
>>> drizzled::init_remaining_variables(drizzled::module::Registry&)
>>> (drizzled.cc:1345)
>>>   by 0x4E9978: main (main.cc:273)
>>>
>>> Where or what might be the cause of that?  It seems related to
>>> boost::basic_format, but beyond that I'm not sure what needs to be fixed.
>>>
>>>
>> You might want to try to move the code to a simple example and run it with
>> valgrind, and see if you still see the problem. It looks like the only other
>> place boost::basic_format is used is in the logging_query plugin which
>> doesn't have any tests so the warning would not show up in there. A quick
>> glance of the code and it looked correct.
>>
>>
>>
>>> Thanks,
>>>
>>> Daniel
>>>
>>> Le 12 juil. 2011 à 16:21, Mark Atwood a écrit :
>>>
>>>> Review: Needs Fixing
>>>> Valgrind problems.
>>>>
>>>> Refer to
>>> http://jenkins.drizzle.org/view/Drizzle-build/job/drizzle-build-ubuntu10.04-64bit-valgrind/759/console
>>>>
>>>> "15 warnings in query_log.thresholds"
>>>>
>>>> We are trying to get the valgrind warning count down to zero.
>>>> --
>>>>
>>> https://code.launchpad.net/~daniel-nichter/drizzle/query-log-plugin/+merge/61034<https://code.launchpad.net/%7Edaniel-nichter/drizzle/query-log-plugin/+merge/61034>
>>>> You are the owner of lp:~daniel-nichter/drizzle/query-log-plugin.
>>>
>>>
>>> --
>>>
>>> https://code.launchpad.net/~daniel-nichter/drizzle/query-log-plugin/+merge/61034<https://code.launchpad.net/%7Edaniel-nichter/drizzle/query-log-plugin/+merge/61034>
>>> You are subscribed to branch lp:drizzle.
>>>
>>
>> --
>> https://code.launchpad.net/~daniel-nichter/drizzle/query-log-plugin/+merge/61034
>> You are the owner of lp:~daniel-nichter/drizzle/query-log-plugin.
>
>
> --
> https://code.launchpad.net/~daniel-nichter/drizzle/query-log-plugin/+merge/61034
> You are reviewing the proposed merge of lp:~daniel-nichter/drizzle/query-log-plugin into lp:drizzle.
>