[pacman-dev] [PATCH v2 1/3] package.c: Remove obsolete param from doc
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pacman/package.c b/src/pacman/package.c index 2fd0a9c..0e4977f 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -79,7 +79,6 @@ static void optdeplist_display(alpm_pkg_t *pkg, unsigned short cols) * Extra information entails 'required by' info for sync packages and backup * files info for local packages. * @param pkg package to display information for - * @param from the type of package we are dealing with * @param extra should we show extra information */ void dump_pkg_full(alpm_pkg_t *pkg, int extra) -- 2.6.1
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/package.c b/src/pacman/package.c index 0e4977f..6b86599 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -173,7 +173,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Compressed Size:"), config->colstr.nocolor, size, label); } else { - // autodetect size for "Installed Size" + /* autodetect size for "Installed Size" */ label = "\0"; } -- 2.6.1
On 18/10/15 03:02, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/package.c b/src/pacman/package.c index 0e4977f..6b86599 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -173,7 +173,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Compressed Size:"), config->colstr.nocolor, size, label); } else { - // autodetect size for "Installed Size" + /* autodetect size for "Installed Size" */ label = "\0"; }
OK - no idea why a v2 of this was sent...
On 15-10-18 11:24:15, Allan McRae wrote:
OK - no idea why a v2 of this was sent...
Oopsy! Overzealous frenetic terminal bashing... Only [PATCH v2 3/3] has changes indeed. -- Pierre Neidhardt Children begin by loving their parents. After a time they judge them. Rarely, if ever, do they forgive them. - Oscar Wilde
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 167 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 135 insertions(+), 32 deletions(-) diff --git a/src/pacman/package.c b/src/pacman/package.c index 6b86599..8f6f53b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -25,6 +25,7 @@ #include <limits.h> #include <errno.h> #include <time.h> +#include <wchar.h> #include <alpm.h> #include <alpm_list.h> @@ -36,6 +37,101 @@ #define CLBUF_SIZE 4096 +/* we truncate titles longer than this arbitrary value */ +#define TITLE_MAXLEN 50 + +/* A title is referenced by titles[T_XXX]. The T_ prefix avoids shadowing. */ +enum { + T_ARCHITECTURE = 0, + T_BACKUP_FILES, + T_BUILD_DATE, + T_COMPRESSED_SIZE, + T_CONFLICTS_WITH, + T_DEPENDS_ON, + T_DESCRIPTION, + T_DOWNLOAD_SIZE, + T_GROUPS, + T_INSTALL_DATE, + T_INSTALL_REASON, + T_INSTALL_SCRIPT, + T_INSTALLED_SIZE, + T_LICENSES, + T_MD5_SUM, + T_NAME, + T_OPTIONAL_DEPS, + T_OPTIONAL_FOR, + T_PACKAGER, + T_PROVIDES, + T_REPLACES, + T_REPOSITORY, + T_REQUIRED_BY, + T_SHA_256_SUM, + T_SIGNATURES, + T_URL, + T_VALIDATED_BY, + T_VERSION, + TITLE_COUNT +}; + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)]; + +/** Truncate all titles longer than TITLE_MAX and pad the rest with spaces so + * that they align with the longest title. " :" is appended to all titles. + * + * Truncation allows us to avoid allocations and clean up code. + */ +static void make_aligned_titles() +{ + int i; + size_t max = 0; + static const wchar_t *title_suffix = L" :"; + static const size_t title_suffix_len = sizeof(title_suffix); + wchar_t wbuf[TITLE_COUNT][TITLE_MAXLEN + title_suffix_len]; + size_t wlen[TITLE_COUNT]; + char *buf[TITLE_COUNT]; + buf[T_ARCHITECTURE] = _("Architecture"); + buf[T_BACKUP_FILES] = _("Backup Files"); + buf[T_BUILD_DATE] = _("Build Date"); + buf[T_COMPRESSED_SIZE] = _("Compressed Size"); + buf[T_CONFLICTS_WITH] = _("Conflicts With"); + buf[T_DEPENDS_ON] = _("Depends On"); + buf[T_DESCRIPTION] = _("Description"); + buf[T_DOWNLOAD_SIZE] = _("Download Size"); + buf[T_GROUPS] = _("Groups"); + buf[T_INSTALL_DATE] = _("Install Date"); + buf[T_INSTALL_REASON] = _("Install Reason"); + buf[T_INSTALL_SCRIPT] = _("Install Script"); + buf[T_INSTALLED_SIZE] = _("Installed Size"); + buf[T_LICENSES] = _("Licenses"); + buf[T_MD5_SUM] = _("MD5 Sum"); + buf[T_NAME] = _("Name"); + buf[T_OPTIONAL_DEPS] = _("Optional Deps"); + buf[T_OPTIONAL_FOR] = _("Optional For"); + buf[T_PACKAGER] = _("Packager"); + buf[T_PROVIDES] = _("Provides"); + buf[T_REPLACES] = _("Replaces"); + buf[T_REPOSITORY] = _("Repository"); + buf[T_REQUIRED_BY] = _("Required By"); + buf[T_SHA_256_SUM] = _("SHA-256 Sum"); + buf[T_SIGNATURES] = _("Signatures"); + buf[T_URL] = _("URL"); + buf[T_VALIDATED_BY] = _("Validated By"); + buf[T_VERSION] = _("Version"); + + for(i = 0; i < TITLE_COUNT; i++) { + wlen[i] = mbstowcs(wbuf[i], buf[i], strlen(buf[i]) + 1); + if(wlen[i] > max) { + max = wlen[i]; + } + } + + for(i = 0; i < TITLE_COUNT; i++) { + wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]); + wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); + } +} + /** Turn a depends list into a text list. * @param deps a list with items of type alpm_depend_t */ @@ -70,7 +166,7 @@ static void optdeplist_display(alpm_pkg_t *pkg, unsigned short cols) } text = alpm_list_add(text, depstring); } - list_display_linebreak(_("Optional Deps :"), text, cols); + list_display_linebreak(titles[T_OPTIONAL_DEPS], text, cols); FREELIST(text); } @@ -91,6 +187,13 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) const char *label, *reason; alpm_list_t *validation = NULL, *requiredby = NULL, *optionalfor = NULL; + /* make aligned titles once only */ + static int need_alignment = 1; + if(need_alignment) { + need_alignment = 0; + make_aligned_titles(); + } + from = alpm_pkg_get_origin(pkg); /* set variables here, do all output below */ @@ -124,7 +227,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) validation = alpm_list_add(validation, _("MD5 Sum")); } if(v & ALPM_PKG_VALIDATION_SHA256SUM) { - validation = alpm_list_add(validation, _("SHA256 Sum")); + validation = alpm_list_add(validation, _("SHA-256 Sum")); } if(v & ALPM_PKG_VALIDATION_SIGNATURE) { validation = alpm_list_add(validation, _("Signature")); @@ -144,33 +247,33 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) /* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { - string_display(_("Repository :"), + string_display(titles[T_REPOSITORY], alpm_db_get_name(alpm_pkg_get_db(pkg)), cols); } - string_display(_("Name :"), alpm_pkg_get_name(pkg), cols); - string_display(_("Version :"), alpm_pkg_get_version(pkg), cols); - string_display(_("Description :"), alpm_pkg_get_desc(pkg), cols); - string_display(_("Architecture :"), alpm_pkg_get_arch(pkg), cols); - string_display(_("URL :"), alpm_pkg_get_url(pkg), cols); - list_display(_("Licenses :"), alpm_pkg_get_licenses(pkg), cols); - list_display(_("Groups :"), alpm_pkg_get_groups(pkg), cols); - deplist_display(_("Provides :"), alpm_pkg_get_provides(pkg), cols); - deplist_display(_("Depends On :"), alpm_pkg_get_depends(pkg), cols); + string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols); + string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols); + string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols); + string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols); + string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols); + list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols); + list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols); + deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols); + deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols); optdeplist_display(pkg, cols); if(extra || from == ALPM_PKG_FROM_LOCALDB) { - list_display(_("Required By :"), requiredby, cols); - list_display(_("Optional For :"), optionalfor, cols); + list_display(titles[T_REQUIRED_BY], requiredby, cols); + list_display(titles[T_OPTIONAL_FOR], optionalfor, cols); } - deplist_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg), cols); - deplist_display(_("Replaces :"), alpm_pkg_get_replaces(pkg), cols); + deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols); + deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols); size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) { - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Download Size :"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_DOWNLOAD_SIZE], config->colstr.nocolor, size, label); } else if(from == ALPM_PKG_FROM_FILE) { - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Compressed Size:"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_COMPRESSED_SIZE], config->colstr.nocolor, size, label); } else { /* autodetect size for "Installed Size" */ @@ -178,17 +281,17 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) } size = humanize_size(alpm_pkg_get_isize(pkg), label[0], 2, &label); - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Installed Size :"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label); - string_display(_("Packager :"), alpm_pkg_get_packager(pkg), cols); - string_display(_("Build Date :"), bdatestr, cols); + string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols); + string_display(titles[T_BUILD_DATE], bdatestr, cols); if(from == ALPM_PKG_FROM_LOCALDB) { - string_display(_("Install Date :"), idatestr, cols); - string_display(_("Install Reason :"), reason, cols); + string_display(titles[T_INSTALL_DATE], idatestr, cols); + string_display(titles[T_INSTALL_REASON], reason, cols); } if(from == ALPM_PKG_FROM_FILE || from == ALPM_PKG_FROM_LOCALDB) { - string_display(_("Install Script :"), + string_display(titles[T_INSTALL_SCRIPT], alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols); } @@ -206,27 +309,27 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); } - string_display(_("MD5 Sum :"), alpm_pkg_get_md5sum(pkg), cols); - string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); - list_display(_("Signatures :"), keys, cols); + string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols); + string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols); + list_display(titles[T_SIGNATURES], keys, cols); if(base64_sig) { FREELIST(keys); } } else { - list_display(_("Validated By :"), validation, cols); + list_display(titles[T_VALIDATED_BY], validation, cols); } if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) { - string_display(_("Signatures :"), _("None"), cols); + string_display(titles[T_SIGNATURES], _("None"), cols); } else if(err) { - string_display(_("Signatures :"), + string_display(titles[T_SIGNATURES], alpm_strerror(alpm_errno(config->handle)), cols); } else { - signature_display(_("Signatures :"), &siglist, cols); + signature_display(titles[T_SIGNATURES], &siglist, cols); } alpm_siglist_cleanup(&siglist); } @@ -290,7 +393,7 @@ void dump_pkg_backups(alpm_pkg_t *pkg) { alpm_list_t *i; const char *root = alpm_option_get_root(config->handle); - printf("%s%s%s", config->colstr.title, _("Backup Files:\n"), + printf("%s%s\n%s", config->colstr.title, titles[T_BACKUP_FILES], config->colstr.nocolor); if(alpm_pkg_get_backup(pkg)) { /* package has backup files, so print them */ -- 2.6.1
On Sat, Oct 17, 2015 at 07:02:57PM +0200, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 167 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 135 insertions(+), 32 deletions(-)
diff --git a/src/pacman/package.c b/src/pacman/package.c index 6b86599..8f6f53b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -25,6 +25,7 @@ #include <limits.h> #include <errno.h> #include <time.h> +#include <wchar.h>
#include <alpm.h> #include <alpm_list.h> @@ -36,6 +37,101 @@
#define CLBUF_SIZE 4096
+/* we truncate titles longer than this arbitrary value */
Well no, it isn't arbitrary, you said so yourself. Why not state this? Something like: "As of such and such a date, the longest relevant string from the PO files was measured to be X characters long. We set this to 50 to allow for some potential growth."
+#define TITLE_MAXLEN 50 + +/* A title is referenced by titles[T_XXX]. The T_ prefix avoids shadowing. */
Not a useful comment. Nowhere in this file do you explain what a "title" is. The fact that you use a prefix is quite obviously done to make for unique enum identifiers.
+enum { + T_ARCHITECTURE = 0, + T_BACKUP_FILES, + T_BUILD_DATE, + T_COMPRESSED_SIZE, + T_CONFLICTS_WITH, + T_DEPENDS_ON, + T_DESCRIPTION, + T_DOWNLOAD_SIZE, + T_GROUPS, + T_INSTALL_DATE, + T_INSTALL_REASON, + T_INSTALL_SCRIPT, + T_INSTALLED_SIZE, + T_LICENSES, + T_MD5_SUM, + T_NAME, + T_OPTIONAL_DEPS, + T_OPTIONAL_FOR, + T_PACKAGER, + T_PROVIDES, + T_REPLACES, + T_REPOSITORY, + T_REQUIRED_BY, + T_SHA_256_SUM, + T_SIGNATURES, + T_URL, + T_VALIDATED_BY, + T_VERSION, + TITLE_COUNT
Please comment that this is a sentinel value and needs to be last... I'd also love to claim that this needs to share the same prefix as the enums it belongs to (and have an underscore prefix), but there's not really any prevailing coding style in ALPM that I can lean on to "enforce" that.
+}; + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)]; + +/** Truncate all titles longer than TITLE_MAX and pad the rest with spaces so + * that they align with the longest title. " :" is appended to all titles. + * + * Truncation allows us to avoid allocations and clean up code.
This isn't a useful comment. It's stack allocation which avoids the cleanup and allocation, not truncation. Being forced to truncate is a side effect of using stack allocating. Perhaps something like: "Build an array of localized strings used to create aligned titles in verbose package information. Storage for strings is stack allocated and naively truncated to TITLE_MAXLEN characters."
+ */ +static void make_aligned_titles() +{ + int i; + size_t max = 0; + static const wchar_t *title_suffix = L" :"; + static const size_t title_suffix_len = sizeof(title_suffix); + wchar_t wbuf[TITLE_COUNT][TITLE_MAXLEN + title_suffix_len]; + size_t wlen[TITLE_COUNT]; + char *buf[TITLE_COUNT]; + buf[T_ARCHITECTURE] = _("Architecture"); + buf[T_BACKUP_FILES] = _("Backup Files"); + buf[T_BUILD_DATE] = _("Build Date"); + buf[T_COMPRESSED_SIZE] = _("Compressed Size"); + buf[T_CONFLICTS_WITH] = _("Conflicts With"); + buf[T_DEPENDS_ON] = _("Depends On"); + buf[T_DESCRIPTION] = _("Description"); + buf[T_DOWNLOAD_SIZE] = _("Download Size"); + buf[T_GROUPS] = _("Groups"); + buf[T_INSTALL_DATE] = _("Install Date"); + buf[T_INSTALL_REASON] = _("Install Reason"); + buf[T_INSTALL_SCRIPT] = _("Install Script"); + buf[T_INSTALLED_SIZE] = _("Installed Size"); + buf[T_LICENSES] = _("Licenses"); + buf[T_MD5_SUM] = _("MD5 Sum"); + buf[T_NAME] = _("Name"); + buf[T_OPTIONAL_DEPS] = _("Optional Deps"); + buf[T_OPTIONAL_FOR] = _("Optional For"); + buf[T_PACKAGER] = _("Packager"); + buf[T_PROVIDES] = _("Provides"); + buf[T_REPLACES] = _("Replaces"); + buf[T_REPOSITORY] = _("Repository"); + buf[T_REQUIRED_BY] = _("Required By"); + buf[T_SHA_256_SUM] = _("SHA-256 Sum"); + buf[T_SIGNATURES] = _("Signatures"); + buf[T_URL] = _("URL"); + buf[T_VALIDATED_BY] = _("Validated By"); + buf[T_VERSION] = _("Version"); + + for(i = 0; i < TITLE_COUNT; i++) { + wlen[i] = mbstowcs(wbuf[i], buf[i], strlen(buf[i]) + 1); + if(wlen[i] > max) { + max = wlen[i]; + } + } + + for(i = 0; i < TITLE_COUNT; i++) { + wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]); + wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); + } +} + /** Turn a depends list into a text list. * @param deps a list with items of type alpm_depend_t */ @@ -70,7 +166,7 @@ static void optdeplist_display(alpm_pkg_t *pkg, unsigned short cols) } text = alpm_list_add(text, depstring); } - list_display_linebreak(_("Optional Deps :"), text, cols); + list_display_linebreak(titles[T_OPTIONAL_DEPS], text, cols); FREELIST(text); }
@@ -91,6 +187,13 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) const char *label, *reason; alpm_list_t *validation = NULL, *requiredby = NULL, *optionalfor = NULL;
+ /* make aligned titles once only */ + static int need_alignment = 1; + if(need_alignment) { + need_alignment = 0; + make_aligned_titles(); + } + from = alpm_pkg_get_origin(pkg);
/* set variables here, do all output below */ @@ -124,7 +227,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) validation = alpm_list_add(validation, _("MD5 Sum")); } if(v & ALPM_PKG_VALIDATION_SHA256SUM) { - validation = alpm_list_add(validation, _("SHA256 Sum")); + validation = alpm_list_add(validation, _("SHA-256 Sum")); } if(v & ALPM_PKG_VALIDATION_SIGNATURE) { validation = alpm_list_add(validation, _("Signature")); @@ -144,33 +247,33 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra)
/* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { - string_display(_("Repository :"), + string_display(titles[T_REPOSITORY], alpm_db_get_name(alpm_pkg_get_db(pkg)), cols); } - string_display(_("Name :"), alpm_pkg_get_name(pkg), cols); - string_display(_("Version :"), alpm_pkg_get_version(pkg), cols); - string_display(_("Description :"), alpm_pkg_get_desc(pkg), cols); - string_display(_("Architecture :"), alpm_pkg_get_arch(pkg), cols); - string_display(_("URL :"), alpm_pkg_get_url(pkg), cols); - list_display(_("Licenses :"), alpm_pkg_get_licenses(pkg), cols); - list_display(_("Groups :"), alpm_pkg_get_groups(pkg), cols); - deplist_display(_("Provides :"), alpm_pkg_get_provides(pkg), cols); - deplist_display(_("Depends On :"), alpm_pkg_get_depends(pkg), cols); + string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols); + string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols); + string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols); + string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols); + string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols); + list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols); + list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols); + deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols); + deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols); optdeplist_display(pkg, cols);
if(extra || from == ALPM_PKG_FROM_LOCALDB) { - list_display(_("Required By :"), requiredby, cols); - list_display(_("Optional For :"), optionalfor, cols); + list_display(titles[T_REQUIRED_BY], requiredby, cols); + list_display(titles[T_OPTIONAL_FOR], optionalfor, cols); } - deplist_display(_("Conflicts With :"), alpm_pkg_get_conflicts(pkg), cols); - deplist_display(_("Replaces :"), alpm_pkg_get_replaces(pkg), cols); + deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols); + deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols);
size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) { - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Download Size :"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_DOWNLOAD_SIZE], config->colstr.nocolor, size, label); } else if(from == ALPM_PKG_FROM_FILE) { - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Compressed Size:"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_COMPRESSED_SIZE], config->colstr.nocolor, size, label); } else { /* autodetect size for "Installed Size" */ @@ -178,17 +281,17 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
size = humanize_size(alpm_pkg_get_isize(pkg), label[0], 2, &label); - printf("%s%s%s %6.2f %s\n", config->colstr.title, _("Installed Size :"), + printf("%s%s%s %6.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label);
- string_display(_("Packager :"), alpm_pkg_get_packager(pkg), cols); - string_display(_("Build Date :"), bdatestr, cols); + string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols); + string_display(titles[T_BUILD_DATE], bdatestr, cols); if(from == ALPM_PKG_FROM_LOCALDB) { - string_display(_("Install Date :"), idatestr, cols); - string_display(_("Install Reason :"), reason, cols); + string_display(titles[T_INSTALL_DATE], idatestr, cols); + string_display(titles[T_INSTALL_REASON], reason, cols); } if(from == ALPM_PKG_FROM_FILE || from == ALPM_PKG_FROM_LOCALDB) { - string_display(_("Install Script :"), + string_display(titles[T_INSTALL_SCRIPT], alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols); }
@@ -206,27 +309,27 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); }
- string_display(_("MD5 Sum :"), alpm_pkg_get_md5sum(pkg), cols); - string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); - list_display(_("Signatures :"), keys, cols); + string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols); + string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols); + list_display(titles[T_SIGNATURES], keys, cols);
if(base64_sig) { FREELIST(keys); } } else { - list_display(_("Validated By :"), validation, cols); + list_display(titles[T_VALIDATED_BY], validation, cols); }
if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) { - string_display(_("Signatures :"), _("None"), cols); + string_display(titles[T_SIGNATURES], _("None"), cols); } else if(err) { - string_display(_("Signatures :"), + string_display(titles[T_SIGNATURES], alpm_strerror(alpm_errno(config->handle)), cols); } else { - signature_display(_("Signatures :"), &siglist, cols); + signature_display(titles[T_SIGNATURES], &siglist, cols); } alpm_siglist_cleanup(&siglist); } @@ -290,7 +393,7 @@ void dump_pkg_backups(alpm_pkg_t *pkg) { alpm_list_t *i; const char *root = alpm_option_get_root(config->handle); - printf("%s%s%s", config->colstr.title, _("Backup Files:\n"), + printf("%s%s\n%s", config->colstr.title, titles[T_BACKUP_FILES], config->colstr.nocolor); if(alpm_pkg_get_backup(pkg)) { /* package has backup files, so print them */ -- 2.6.1
On 15-10-19 17:22:55, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 07:02:57PM +0200, Pierre Neidhardt wrote:
+/* we truncate titles longer than this arbitrary value */
Well no, it isn't arbitrary, you said so yourself. Why not state this? Something like:
"As of such and such a date, the longest relevant string from the PO files was measured to be X characters long. We set this to 50 to allow for some potential growth."
+#define TITLE_MAXLEN 50 + +/* A title is referenced by titles[T_XXX]. The T_ prefix avoids shadowing. */
Not a useful comment. Nowhere in this file do you explain what a "title" is.
The term "title" is used (not only by me) here and there throughout the code. (See 'util.c'.) I thought this was a given, but it does not cost much to explain here what it is. What is more useful though is to explain what an "aligned title" is.
The fact that you use a prefix is quite obviously done to make for unique enum identifiers.
+enum { + T_ARCHITECTURE = 0, + T_BACKUP_FILES, + T_BUILD_DATE, + T_COMPRESSED_SIZE, + T_CONFLICTS_WITH, + T_DEPENDS_ON, + T_DESCRIPTION, + T_DOWNLOAD_SIZE, + T_GROUPS, + T_INSTALL_DATE, + T_INSTALL_REASON, + T_INSTALL_SCRIPT, + T_INSTALLED_SIZE, + T_LICENSES, + T_MD5_SUM, + T_NAME, + T_OPTIONAL_DEPS, + T_OPTIONAL_FOR, + T_PACKAGER, + T_PROVIDES, + T_REPLACES, + T_REPOSITORY, + T_REQUIRED_BY, + T_SHA_256_SUM, + T_SIGNATURES, + T_URL, + T_VALIDATED_BY, + T_VERSION, + TITLE_COUNT
Please comment that this is a sentinel value and needs to be last...
OK.
I'd also love to claim that this needs to share the same prefix as the enums it belongs to (and have an underscore prefix), but there's not really any prevailing coding style in ALPM that I can lean on to "enforce" that.
Agreed, but I prefer keeping underscore prefixes for library code. This is not ALPM but the pacman frontend if this is what you meant.
+}; + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)]; + +/** Truncate all titles longer than TITLE_MAX and pad the rest with spaces so + * that they align with the longest title. " :" is appended to all titles. + * + * Truncation allows us to avoid allocations and clean up code.
This isn't a useful comment. It's stack allocation which avoids the cleanup and allocation, not truncation. Being forced to truncate is a side effect of using stack allocating.
Agreed.
Perhaps something like:
"Build an array of localized strings used to create aligned titles in verbose package information. Storage for strings is stack allocated and naively truncated to TITLE_MAXLEN characters."
Your version is a bit misleading: we could think that the alignment happens outside this function. It misses an explanation for what "aligning" titles precisely means. I'd say:
Build the `titles` array of localized titles and pad them with spaces so that they align with the longest title. Storage for strings is stack allocated and naively truncated to TITLE_MAXLEN characters."
-- Pierre Neidhardt It is better to have loved and lost than just to have lost.
On Tue, Oct 20, 2015 at 10:29:43AM +0200, Pierre Neidhardt wrote:
On 15-10-19 17:22:55, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 07:02:57PM +0200, Pierre Neidhardt wrote:
+/* we truncate titles longer than this arbitrary value */
Well no, it isn't arbitrary, you said so yourself. Why not state this? Something like:
"As of such and such a date, the longest relevant string from the PO files was measured to be X characters long. We set this to 50 to allow for some potential growth."
+#define TITLE_MAXLEN 50 + +/* A title is referenced by titles[T_XXX]. The T_ prefix avoids shadowing. */
Not a useful comment. Nowhere in this file do you explain what a "title" is.
The term "title" is used (not only by me) here and there throughout the code. (See 'util.c'.) I thought this was a given, but it does not cost much to explain here what it is. What is more useful though is to explain what an "aligned title" is.
Ack.
The fact that you use a prefix is quite obviously done to make for unique enum identifiers.
+enum { + T_ARCHITECTURE = 0, + T_BACKUP_FILES, + T_BUILD_DATE, + T_COMPRESSED_SIZE, + T_CONFLICTS_WITH, + T_DEPENDS_ON, + T_DESCRIPTION, + T_DOWNLOAD_SIZE, + T_GROUPS, + T_INSTALL_DATE, + T_INSTALL_REASON, + T_INSTALL_SCRIPT, + T_INSTALLED_SIZE, + T_LICENSES, + T_MD5_SUM, + T_NAME, + T_OPTIONAL_DEPS, + T_OPTIONAL_FOR, + T_PACKAGER, + T_PROVIDES, + T_REPLACES, + T_REPOSITORY, + T_REQUIRED_BY, + T_SHA_256_SUM, + T_SIGNATURES, + T_URL, + T_VALIDATED_BY, + T_VERSION, + TITLE_COUNT
Please comment that this is a sentinel value and needs to be last...
OK.
I'd also love to claim that this needs to share the same prefix as the enums it belongs to (and have an underscore prefix), but there's not really any prevailing coding style in ALPM that I can lean on to "enforce" that.
Agreed, but I prefer keeping underscore prefixes for library code. This is not ALPM but the pacman frontend if this is what you meant.
I've no idea why you would suddenly change coding style for internal library code versus frontend code.
+}; + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)];
I do wonder if it isn't best to also only use _T_MAX here and elsewhere in the code where you traverse the arrays use: sizeof(titles)/sizeof(titles[0]) And perhaps introduces an ARRAYSIZE or ELEMENTSOF macro to do this for us (we'd make use it elsewhere in the lib and frontend). This is safer and clearer to the reader that you're traversing the precise bounds of the array, not some max index that you "hope" lines up with the bound.
+ +/** Truncate all titles longer than TITLE_MAX and pad the rest with spaces so + * that they align with the longest title. " :" is appended to all titles. + * + * Truncation allows us to avoid allocations and clean up code.
This isn't a useful comment. It's stack allocation which avoids the cleanup and allocation, not truncation. Being forced to truncate is a side effect of using stack allocating.
Agreed.
Perhaps something like:
"Build an array of localized strings used to create aligned titles in verbose package information. Storage for strings is stack allocated and naively truncated to TITLE_MAXLEN characters."
Your version is a bit misleading: we could think that the alignment happens outside this function. It misses an explanation for what "aligning" titles precisely means. I'd say:
Build the `titles` array of localized titles and pad them with spaces so that they align with the longest title. Storage for strings is stack allocated and naively truncated to TITLE_MAXLEN characters."
Sounds good.
-- Pierre Neidhardt
It is better to have loved and lost than just to have lost.
On 15-10-20 06:58:58, Dave Reisner wrote:
On Tue, Oct 20, 2015 at 10:29:43AM +0200, Pierre Neidhardt wrote:
On 15-10-19 17:22:55, Dave Reisner wrote:
I'd also love to claim that this needs to share the same prefix as the enums it belongs to (and have an underscore prefix), but there's not really any prevailing coding style in ALPM that I can lean on to "enforce" that.
Agreed, but I prefer keeping underscore prefixes for library code. This is not ALPM but the pacman frontend if this is what you meant.
I've no idea why you would suddenly change coding style for internal library code versus frontend code.
I believe there is a misunderstanding here. To me, underscore prefixed variables are used in libraries as a mean to tell the calling programs "don't use me, I'm internal". My patch only changes 'package.c' which is part of the frontend code. Following the aforementioned coding style, I would rather not prefix any variable with an underscore. That is really a nit though, if you really like it better, I don't mind changing it.
+}; + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)];
I do wonder if it isn't best to also only use _T_MAX here and elsewhere in the code where you traverse the arrays use:
sizeof(titles)/sizeof(titles[0])
And perhaps introduces an ARRAYSIZE or ELEMENTSOF macro to do this for us (we'd make use it elsewhere in the lib and frontend). This is safer and clearer to the reader that you're traversing the precise bounds of the array, not some max index that you "hope" lines up with the bound.
Agreed, plus this is idiomatic enough in C. But! From HACKING:
6. The sizeof() operator should accept a type, not a value. (TODO: in certain cases, it may be better- should this be a set guideline? Read "The Practice of Programming")
I must say I agree with the TODO. Shall we update this? From your first review, you asked for 2 changes that go againt paragraph 6: static const size_t title_suffix_len = sizeof(title_suffix); and wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); Besides, I have seen a few other places in the current code where §6 is being transgressed. Should we agree on this, shall we place the ARRAYSIZE macro in util-common.h? Other typical names include LENGTH, SIZE, COUNT_OF, ELEMENTSOF. I like ARRAYSIZE better as it makes clear it must be an array. -- Pierre Neidhardt An optimist is a guy that has never had much experience. -- Don Marquis
On Tue, Oct 20, 2015 at 02:11:41PM +0200, Pierre Neidhardt wrote:
On 15-10-20 06:58:58, Dave Reisner wrote:
On Tue, Oct 20, 2015 at 10:29:43AM +0200, Pierre Neidhardt wrote:
On 15-10-19 17:22:55, Dave Reisner wrote:
I'd also love to claim that this needs to share the same prefix as the enums it belongs to (and have an underscore prefix), but there's not really any prevailing coding style in ALPM that I can lean on to "enforce" that.
Agreed, but I prefer keeping underscore prefixes for library code. This is not ALPM but the pacman frontend if this is what you meant.
I've no idea why you would suddenly change coding style for internal library code versus frontend code.
I believe there is a misunderstanding here. To me, underscore prefixed variables are used in libraries as a mean to tell the calling programs "don't use me, I'm internal".
That sounds like bad API design. If you didn't want it exposed and used, you shouldn't have exposed it. The underscore prefix here is just a case of "one of these things is not like the other". It belongs, because it's related to the rest of the enum, but it isn't one of the identifiers used as part of the enum to index a title string.
My patch only changes 'package.c' which is part of the frontend code. Following the aforementioned coding style, I would rather not prefix any variable with an underscore.
That is really a nit though, if you really like it better, I don't mind changing it.
+}; + +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)];
I do wonder if it isn't best to also only use _T_MAX here and elsewhere in the code where you traverse the arrays use:
sizeof(titles)/sizeof(titles[0])
And perhaps introduces an ARRAYSIZE or ELEMENTSOF macro to do this for us (we'd make use it elsewhere in the lib and frontend). This is safer and clearer to the reader that you're traversing the precise bounds of the array, not some max index that you "hope" lines up with the bound.
Agreed, plus this is idiomatic enough in C. But! From HACKING:
6. The sizeof() operator should accept a type, not a value. (TODO: in certain cases, it may be better- should this be a set guideline? Read "The Practice of Programming")
I must say I agree with the TODO. Shall we update this? From your first review, you asked for 2 changes that go againt paragraph 6:
Seems fine to me. To be explicit, the referenced literature says "Use the language to calculate the size of an object" and gives an example macro of: #define NELEMS(array) (sizeof(array) / sizeof(array[0])) This is pragmatic and, more importantly, safe. I'm not sure why you'd want to prefer a type over a value. The former seems to me to be more error prone than using the value.
static const size_t title_suffix_len = sizeof(title_suffix);
and
wcstombs(titles[i], wbuf[i], sizeof(wbuf[i]));
Besides, I have seen a few other places in the current code where §6 is being transgressed.
Sounds likely...
Should we agree on this, shall we place the ARRAYSIZE macro in util-common.h? Other typical names include LENGTH, SIZE, COUNT_OF, ELEMENTSOF. I like ARRAYSIZE better as it makes clear it must be an array.
Please do. We can do cleanup in other places in another patch, but it'll be nice to have it here.
-- Pierre Neidhardt
An optimist is a guy that has never had much experience. -- Don Marquis
On 18/10/15 03:02, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 1 - 1 file changed, 1 deletion(-)
Ack.
participants (3)
-
Allan McRae
-
Dave Reisner
-
Pierre Neidhardt