Re: [systemd-devel] [PATCH 2/2] test: test for ellipsize (manual)
On Sun, Oct 13, 2013 at 4:46 PM, Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl wrote: On Sun, Oct 06, 2013 at 05:20:58AM +0200, Zbigniew Jędrzejewski-Szmek wrote: Hi, sorry for the long delay. I was expecting this review to take some time, and I was right :) I started by running test-ellipsize under valgrind, and found that there unitialized memory was written. There was a bug in ellipsize_mem: when the string was short enough to fit whole, the middle part was used twice. E.g. abcdef was ellipsized as abcd...cdef. But space was allocated without this overlap, so valgrind was unhappy. Also, there was some overlap with existing functions, so I managed to remove a large part of the patch. I moved the new code to a separate file, to avoid mixing code written in two copletely different styles in the same file. I hope it'll make it easier to pull changes from glib in the future. I think that ellipsize_ascii should be removed, small saving in allocated memory probably isn't worth the extra scan which is done to check if the string is pure ASCII. Also, it's not nice that ellipsization is different for pure ASCII and ASCII strings. I think we should use the same character(s) in both cases. But I left it as is for now. I also optimized the function a bit, so that memory is not zeroed after allocation, and unicode verification is performed while counting the characters. I found that the COW FACE sequence is not ellipsized properly, because COW FACEs are fullwidth. I've added this whole block to the fullwidth table in a separate patch. All in all, those are pretty significant changes. Therefore, instead of pushing, I've posted the changes at http://in.waw.pl/git/systemd/ Could you have a look at the updated patches, and maybe test them a bit? Pushed now. Thanks for your work on this and sorry for not getting around to reviewing your work. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] test: test for ellipsize (manual)
On Sun, Oct 06, 2013 at 05:20:58AM +0200, Zbigniew Jędrzejewski-Szmek wrote: Hi, sorry for the long delay. I was expecting this review to take some time, and I was right :) I started by running test-ellipsize under valgrind, and found that there unitialized memory was written. There was a bug in ellipsize_mem: when the string was short enough to fit whole, the middle part was used twice. E.g. abcdef was ellipsized as abcd...cdef. But space was allocated without this overlap, so valgrind was unhappy. Also, there was some overlap with existing functions, so I managed to remove a large part of the patch. I moved the new code to a separate file, to avoid mixing code written in two copletely different styles in the same file. I hope it'll make it easier to pull changes from glib in the future. I think that ellipsize_ascii should be removed, small saving in allocated memory probably isn't worth the extra scan which is done to check if the string is pure ASCII. Also, it's not nice that ellipsization is different for pure ASCII and ASCII strings. I think we should use the same character(s) in both cases. But I left it as is for now. I also optimized the function a bit, so that memory is not zeroed after allocation, and unicode verification is performed while counting the characters. I found that the COW FACE sequence is not ellipsized properly, because COW FACEs are fullwidth. I've added this whole block to the fullwidth table in a separate patch. All in all, those are pretty significant changes. Therefore, instead of pushing, I've posted the changes at http://in.waw.pl/git/systemd/ Could you have a look at the updated patches, and maybe test them a bit? Pushed now. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] test: test for ellipsize (manual)
Hi, sorry for the long delay. I was expecting this review to take some time, and I was right :) I started by running test-ellipsize under valgrind, and found that there unitialized memory was written. There was a bug in ellipsize_mem: when the string was short enough to fit whole, the middle part was used twice. E.g. abcdef was ellipsized as abcd...cdef. But space was allocated without this overlap, so valgrind was unhappy. Also, there was some overlap with existing functions, so I managed to remove a large part of the patch. I moved the new code to a separate file, to avoid mixing code written in two copletely different styles in the same file. I hope it'll make it easier to pull changes from glib in the future. I think that ellipsize_ascii should be removed, small saving in allocated memory probably isn't worth the extra scan which is done to check if the string is pure ASCII. Also, it's not nice that ellipsization is different for pure ASCII and ASCII strings. I think we should use the same character(s) in both cases. But I left it as is for now. I also optimized the function a bit, so that memory is not zeroed after allocation, and unicode verification is performed while counting the characters. I found that the COW FACE sequence is not ellipsized properly, because COW FACEs are fullwidth. I've added this whole block to the fullwidth table in a separate patch. All in all, those are pretty significant changes. Therefore, instead of pushing, I've posted the changes at http://in.waw.pl/git/systemd/ Could you have a look at the updated patches, and maybe test them a bit? Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 2/2] test: test for ellipsize (manual)
---
Makefile.am | 7 +++
src/test/test-ellipsize.c | 42 ++
2 files changed, 49 insertions(+)
create mode 100644 src/test/test-ellipsize.c
diff --git a/Makefile.am b/Makefile.am
index 89a5c86..802f6eb 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1114,6 +1114,7 @@ manual_tests += \
test-cgroup \
test-install \
test-watchdog \
+ test-ellipsize \
test-log
tests += \
@@ -1319,6 +1320,12 @@ test_log_SOURCES = \
test_log_LDADD = \
libsystemd-core.la
+test_ellipsize_SOURCES = \
+ src/test/test-ellipsize.c
+
+test_ellipsize_LDADD = \
+ libsystemd-core.la
+
test_date_SOURCES = \
src/test/test-date.c
diff --git a/src/test/test-ellipsize.c b/src/test/test-ellipsize.c
new file mode 100644
index 000..fab5e4b
--- /dev/null
+++ b/src/test/test-ellipsize.c
@@ -0,0 +1,42 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright 2013 Shawn Landden
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see http://www.gnu.org/licenses/.
+***/
+
+#include stdio.h
+
+#include util.h
+#include utf8.h
+
+static void test_one(const char *p) {
+_cleanup_free_ char *t = NULL;
+t = ellipsize(p, 80, 70);
+puts(t);
+}
+
+int main(int argc, char *argv[]) {
+
test_one(??);
+
test_one(-?);
+
test_one(-??);
+test_one(s??st??md s??st??md s??st??md s??st??md s??st??md s??st??md
s??st??md s??st??md s??st??md s??st??md s??st??md s??st??md s??st??md);
+
test_one();
+test_one(Lorem ipsum dolor sit amet, consectetur adipisicing elit,
sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit
esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat
non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.);
+
+return 0;
+}
--
1.8.4.rc3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
