Hello Jeremy!
Here my comments for the code that you pushed yesterday.
Your latest code revision still creates string copies instead of just
splitting up the memory mapped file:
commit dcd892e84a630c394f9f3ac4615bed09d8c6bede
Author: Jeremy Whiting <jeremy.whit...@collabora.com>
Date: Fri Aug 31 18:18:01 2012 -0600
PBAP: Fix vcard parsing regexp so all vcards are parsed.
diff --git a/src/backends/pbap/PbapSyncSource.cpp
b/src/backends/pbap/PbapSyncSource.cpp
index b854633..d39dbec 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -280,13 +280,13 @@ void PbapSession::pullAll(Content &dst)
pcrecpp::StringPiece content(addr, sb.st_size);
- string vcarddata;
+ pcrecpp::StringPiece vcarddata;
int count = 0;
- pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
+ pcrecpp::RE re("[\\r\\n]*(^BEGIN:VCARD.*?^END:VCARD)",
pcrecpp::RE_Options().set_dotall(true).set_multiline(true));
while (re.Consume(&content, &vcarddata)) {
std::string id = StringPrintf("%d", count);
- dst[id] = vcarddata;
+ dst[id] = vcarddata.as_string();
^^^^^^^^^^^^^^^^^^^^^^
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).
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).
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".
This should be added to the commit messages and as bug entry in the
issue tracker, otherwise there is a high risk that it'll get into a
release without fixing it. Heck, I probably should insist on it getting
fixed before merging into master :-/
I'd prefer to merge one patch for the PBAP backend. Feel free to squash
your and my commits into one.
--
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
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution
