GNOME Bugzilla – Bug 650255
GcrColons -> GcrRecord
Last modified: 2019-02-22 11:59:09 UTC
In order to work towards parsing photos and attributes, from the gpg output we need more features in our GcrColons support. In addition we need to be able to parse a space delimited output line easily. GcrColons just needs a few changes for this, so I've renamed it to GcrRecord. * Completed tests * Add boxed type so we can use GcrRecord in signals * Allow parsing of space delimited records * Parsing of unsigned integers in records
Created attachment 187867 [details] [review] Patch which adds more GcrRecord functionality The commit which renames GcrColons to GcrRecord is not included in the above patch because it's hard to review. But all commits are available here: http://cgit.collabora.co.uk/git/user/stefw/gnome-keyring.git/log/?h=gnupg-record
Review of attachment 187867 [details] [review]: Looks good to me apart from the few minor comments below. ::: gcr/gcr-record.c @@ +54,3 @@ + + return type; +} I think you could use G_DEFINE_BOXED_TYPE() here instead, to save some LoC. @@ +215,3 @@ + return FALSE; + + result = strtol (raw, &end, 10); Is the output from gpg locale-independent? If not, you might want to use g_ascii_strtoll() instead. @@ +232,3 @@ + + + Excess whitespace. ::: gcr/tests/test-record.c @@ +248,3 @@ + g_assert_cmpstr (_gcr_record_get_raw (copy, i), ==, + _gcr_record_get_raw (test->record, i)); + } Not that it especially matters, but copy should probably be freed here. Also, you might want to check the boundary condition that (_gcr_record_get_raw (copy, count) == NULL). @@ +263,3 @@ + for (i = 0; i < count; i++) { + g_assert_cmpstr (_gcr_record_get_raw (copy, i), ==, + _gcr_record_get_raw (test->record, i)); Might want to check that boundary condition here too.
Thanks for your quick review. Made those changes. (In reply to comment #2) > @@ +215,3 @@ > + return FALSE; > + > + result = strtol (raw, &end, 10); > > Is the output from gpg locale-independent? If not, you might want to use > g_ascii_strtoll() instead. We should make it locale independent (when running to list keys, at least). Will take that into account in the next bug. But switched to g_ascii_strtoll() here. Merged with master.