On Fri, 2012-09-21 at 22:06 +0200, Patrick Ohly wrote:
> I still think it would be better to rearrange the class so that dst maps
> from id to a StringPiece, with the actual memory owned by the class
> (either in the memory mapped area or in a std::string buffer).
Your commit which implements that (0f827319) is incomplete: ownership of
the memory referenced by the StringPiece instances is unclear. Because
umapping that memory is missing, it happens to work. I doubt that the
code works for old obex, because the StringPiece is initialized with a
reference to a std::string which then gets deleted.
void PbapSyncSource::listAllItems(RevisionMap_t &revisions)
{
- typedef std::pair<std::string, std::string> Entry;
+ typedef std::pair<std::string, pcrecpp::StringPiece> Entry;
BOOST_FOREACH(const Entry &entry, m_content) {
revisions[entry.first] = "0";
}
A better solution would be to use Content::value_type in BOOST_FOREACH
instead of defining Entry. Not sure why that was done in the first
place.
> The parsing code should be identical for both code paths.
>
> Unmapping the memory mapped file is missing.
>
> The comment for the commit should describe the commit in enough detail
> that a reader of the commit log knows how this new functionality works
> (key points: dynamic runtime check, tries new API first), and what the
> limitations/requirements are (needs D-Bus daemon >= xxx - forgot the
> exact number).
This all still missing.
> It only works as long as obexd only sends strings as property values.
> This is not guaranteed. As I said earlier, "Instead we need a
> boost::variant<std::string, GDBusCXX::DBusObject_t,
> int, bool, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
> uint64_t> as value for the map".
I'll have a look at that tomorrow.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution
