[pacman-dev] [PATCH 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
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 176 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 144 insertions(+), 32 deletions(-) diff --git a/src/pacman/package.c b/src/pacman/package.c index 6b86599..e4d6ad9 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,110 @@ #define CLBUF_SIZE 4096 +#define TITLE_COUNT 28 +#define TITLE_MAXLEN 64 +#define TITLE_MB_MAXLEN TITLE_MAXLEN * sizeof(wchar_t) + +static char titles[TITLE_COUNT][TITLE_MB_MAXLEN]; + +/* Titles references are prefixed with T_ to avoid shadowing. */ +enum { + T_ARCHITECTURE, + 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 +}; + +/** Append " *:" to every title so that the colons are aligned. + * + * We avoid allocations and clean up code by truncating titles to TITLE_MAXLEN. + */ +static void make_aligned_titles() +{ + int i; + size_t max = 0; + wchar_t wbuf[TITLE_COUNT][TITLE_MAXLEN]; + 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]; + } + } + + /* leave room for " :\0" */ + if(max > TITLE_MAXLEN - 3) { + max = TITLE_MAXLEN - 3; + } + + for(i = 0; i < TITLE_COUNT; i++) { + /* pad with " *:" */ + int j; + for(j = wlen[i]; j < max; j++) { + wbuf[i][j] = L' '; + } + wbuf[i][max] = L' '; + wbuf[i][max + 1] = L':'; + wbuf[i][max + 2] = L'\0'; + + wcstombs(titles[i], wbuf[i], TITLE_MB_MAXLEN); + } +} + /** Turn a depends list into a text list. * @param deps a list with items of type alpm_depend_t */ @@ -70,7 +175,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 +196,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 +236,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 +256,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 +290,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 +318,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 +402,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 12:04:11PM +0200, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- src/pacman/package.c | 176 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 144 insertions(+), 32 deletions(-)
diff --git a/src/pacman/package.c b/src/pacman/package.c index 6b86599..e4d6ad9 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,110 @@
#define CLBUF_SIZE 4096
+#define TITLE_COUNT 28
This is brittle. Typical convention is to make it explicit that the enum starts at 0, and then the final enum value is a sentinel called something like _T_MAX. Add a comment that it must be the last value, and then you get this counting for free.
+#define TITLE_MAXLEN 64
Surely, you examined the POT files to determine this value. Could you leave behind a comment explaining as much so that it's clear what this relates to for future readers? Also, as I note below, this is deceptive, as you really only allow a max length of 61 for the title strings.
+#define TITLE_MB_MAXLEN TITLE_MAXLEN * sizeof(wchar_t) + +static char titles[TITLE_COUNT][TITLE_MB_MAXLEN]; + +/* Titles references are prefixed with T_ to avoid shadowing. */
Nit: it'd be more clear to use TITLE_ as a prefix instead of T_.
+enum { + T_ARCHITECTURE, + 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 +}; + +/** Append " *:" to every title so that the colons are aligned.
It's not at all clear to me from this comment what " *:" means. Reading the code, I guess you've intended this to be a regular expression which matches the result of what you're doing. Not at all a useful comment.
+ * + * We avoid allocations and clean up code by truncating titles to TITLE_MAXLEN. + */ +static void make_aligned_titles() +{ + int i; + size_t max = 0; + wchar_t wbuf[TITLE_COUNT][TITLE_MAXLEN]; + 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]; + } + } + + /* leave room for " :\0" */
Instead of introducing a magical "3", write it in the code? static const size_t title_suffix_len = sizeof(" :");
+ if(max > TITLE_MAXLEN - 3) {
Your TITLE_MAXLEN isn't really the max anymore...
+ max = TITLE_MAXLEN - 3; + } + + for(i = 0; i < TITLE_COUNT; i++) { + /* pad with " *:" */
Again, you aren't padding with the literal " *:". You're padding with spaces and then appending a colon. I think the intent of this code is actually easier to grok *without* the comment.
+ int j; + for(j = wlen[i]; j < max; j++) { + wbuf[i][j] = L' '; + } + wbuf[i][max] = L' '; + wbuf[i][max + 1] = L':'; + wbuf[i][max + 2] = L'\0';
wmemcpy?
+ + wcstombs(titles[i], wbuf[i], TITLE_MB_MAXLEN);
Use sizeof(wbuf[i]) instead of TITLE_MB_MAXLEN
+ } +} + /** Turn a depends list into a text list. * @param deps a list with items of type alpm_depend_t */ @@ -70,7 +175,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 +196,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(); + }
Why not just have make_aligned_titles() return a dummy int value? Then, you can: static const int make_aligned_once = make_aligned_titles();
+ from = alpm_pkg_get_origin(pkg);
/* set variables here, do all output below */ @@ -124,7 +236,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 +256,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 +290,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 +318,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 +402,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
Thanks for all the good tips! On 15-10-17 09:11:02, Dave Reisner wrote:
Nit: it'd be more clear to use TITLE_ as a prefix instead of T_.
These constants are used to index the 'titles' variable. Having a TITLE prefix would make it look like titles[TITLE_DESCRIPTION] which I find a bit redundant. It also avoids potential future clashes with TITLE_MAXLEN and TITLE_COUNT. (I removed the other TITLE_* variables as you suggested.)
+ /* make aligned titles once only */ + static int need_alignment = 1; + if(need_alignment) { + need_alignment = 0; + make_aligned_titles(); + }
Why not just have make_aligned_titles() return a dummy int value? Then, you can:
static const int make_aligned_once = make_aligned_titles();
I don't think this would work: static variables are initialized at compile time if I'm not mistaken. -- Pierre Neidhardt
On Sat, Oct 17, 2015 at 05:44:20PM +0200, Pierre Neidhardt wrote:
Thanks for all the good tips!
On 15-10-17 09:11:02, Dave Reisner wrote:
Nit: it'd be more clear to use TITLE_ as a prefix instead of T_.
These constants are used to index the 'titles' variable. Having a TITLE prefix would make it look like
titles[TITLE_DESCRIPTION]
which I find a bit redundant. It also avoids potential future clashes with TITLE_MAXLEN and TITLE_COUNT. (I removed the other TITLE_* variables as you suggested.)
Well, but any clashes are easily resolved. They'd be compile time errors as the resulting code would look something like: enum { TITLE_FOO, TITLE_BAR, 42, TITLE_BAZ, }; Since the preprocessed token is no longer an identifier, the compiler whines.
+ /* make aligned titles once only */ + static int need_alignment = 1; + if(need_alignment) { + need_alignment = 0; + make_aligned_titles(); + }
Why not just have make_aligned_titles() return a dummy int value? Then, you can:
static const int make_aligned_once = make_aligned_titles();
I don't think this would work: static variables are initialized at compile time if I'm not mistaken.
Ah right, this only works in C++.
-- Pierre Neidhardt
On 15-10-17 11:54:40, Dave Reisner wrote:
On 15-10-17 09:11:02, Dave Reisner wrote:
Nit: it'd be more clear to use TITLE_ as a prefix instead of T_.
These constants are used to index the 'titles' variable. Having a TITLE prefix would make it look like
titles[TITLE_DESCRIPTION]
which I find a bit redundant. It also avoids potential future clashes with TITLE_MAXLEN and TITLE_COUNT. (I removed the other TITLE_* variables as you suggested.)
Well, but any clashes are easily resolved. They'd be compile time errors as the resulting code would look something like:
enum { TITLE_FOO, TITLE_BAR, 42, TITLE_BAZ, };
Since the preprocessed token is no longer an identifier, the compiler whines.
Absolutely. But I meant clashes in the nomenclature: If we'd like to add a 'Count' title to pacman -[S|Q]i, we would have to rename '#define TITLE_COUNT' to something else and adapt the code accordingly. No big deal, but I think it is good practice to keep separate nomenclatures for the different families of constants. Another solution would be to prefix the enum with TITLE_ and the configuration variables with TITLECFG_ for instance. -- Pierre Neidhardt
On 15-10-17 09:11:02, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 12:04:11PM +0200, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> +#define TITLE_MAXLEN 64
Surely, you examined the POT files to determine this value. Could you leave behind a comment explaining as much so that it's clear what this relates to for future readers?
Actually I did! :) Longest title string is less than 26 characters long. But you are right, I will add a comment in the next patch. This raises a good question however: what would be a good maximum value for titles? (Titles longer than TITLE_MAXLEN get truncated.) -- Pierre Neidhardt Mind your own business, then you don't mind mine.
On Sat, Oct 17, 2015 at 06:49:14PM +0200, Pierre Neidhardt wrote:
On 15-10-17 09:11:02, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 12:04:11PM +0200, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> +#define TITLE_MAXLEN 64
Surely, you examined the POT files to determine this value. Could you leave behind a comment explaining as much so that it's clear what this relates to for future readers?
Actually I did! :) Longest title string is less than 26 characters long. But you are right, I will add a comment in the next patch.
This raises a good question however: what would be a good maximum value for titles? (Titles longer than TITLE_MAXLEN get truncated.)
Well, it's a bit of a misfeature, IMO. Have you run a benchmark to determine that heap allocating the right amount is substantially slower or more complicated than keeping it on the stack? After all, this code is only run once, so I'm a little skeptical that we gain anything in exchange to bearing the maintenance cost of this magic value.
-- Pierre Neidhardt
Mind your own business, then you don't mind mine.
On 15-10-17 12:56:57, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 06:49:14PM +0200, Pierre Neidhardt wrote:
On 15-10-17 09:11:02, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 12:04:11PM +0200, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> +#define TITLE_MAXLEN 64
Surely, you examined the POT files to determine this value. Could you leave behind a comment explaining as much so that it's clear what this relates to for future readers?
Actually I did! :) Longest title string is less than 26 characters long. But you are right, I will add a comment in the next patch.
This raises a good question however: what would be a good maximum value for titles? (Titles longer than TITLE_MAXLEN get truncated.)
Well, it's a bit of a misfeature, IMO. Have you run a benchmark to determine that heap allocating the right amount is substantially slower or more complicated than keeping it on the stack? After all, this code is only run once, so I'm a little skeptical that we gain anything in exchange to bearing the maintenance cost of this magic value.
Agreed on performance, it's complitely negligible (2*TITLE_COUNT allocs). But the matter is more to cleanup here. Since `dump_pkg_full` can be called several times, we cannot free `titles` from here. It has to be done from one of the callers. Which means that 'titles' should be stored somewhere else, and thus passed to `make_aligned_titles` down the callstack. If we do not want to make it fully global, that being said. Solutions: * Stay static. * Go global. * Pass `tables` down the callstack. Staying static is simple and efficient IMO. TITLE_MAXLEN can be set high enough that it will never get rechead. -- Pierre Neidhardt The major advances in civilization are processes that all but wreck the societies in which they occur. -- A.N. Whitehead
On Sat, Oct 17, 2015 at 07:18:25PM +0200, Pierre Neidhardt wrote:
On 15-10-17 12:56:57, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 06:49:14PM +0200, Pierre Neidhardt wrote:
On 15-10-17 09:11:02, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 12:04:11PM +0200, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> +#define TITLE_MAXLEN 64
Surely, you examined the POT files to determine this value. Could you leave behind a comment explaining as much so that it's clear what this relates to for future readers?
Actually I did! :) Longest title string is less than 26 characters long. But you are right, I will add a comment in the next patch.
This raises a good question however: what would be a good maximum value for titles? (Titles longer than TITLE_MAXLEN get truncated.)
Well, it's a bit of a misfeature, IMO. Have you run a benchmark to determine that heap allocating the right amount is substantially slower or more complicated than keeping it on the stack? After all, this code is only run once, so I'm a little skeptical that we gain anything in exchange to bearing the maintenance cost of this magic value.
Agreed on performance, it's complitely negligible (2*TITLE_COUNT allocs).
But the matter is more to cleanup here. Since `dump_pkg_full` can be called
Also not terribly concerning. It's a fixed size leak which the operating system cleans up for us on process exit. This isn't library code, but I realize this also probably violates pacman "style" and touches a lot of people's OCDs in funny ways.
several times, we cannot free `titles` from here. It has to be done from one of the callers. Which means that 'titles' should be stored somewhere else, and thus passed to `make_aligned_titles` down the callstack. If we do not want to make it fully global, that being said.
Solutions:
* Stay static. * Go global. * Pass `tables` down the callstack.
This third option has the distinct advantage that you can now reasonably *test* your code.
Staying static is simple and efficient IMO. TITLE_MAXLEN can be set high enough that it will never get rechead.
-- Pierre Neidhardt
The major advances in civilization are processes that all but wreck the societies in which they occur. -- A.N. Whitehead
On 15-10-17 13:28:58, Dave Reisner wrote:
On Sat, Oct 17, 2015 at 07:18:25PM +0200, Pierre Neidhardt wrote:
Solutions:
* Stay static. * Go global. * Pass `tables` down the callstack.
(I meant `titles` by the way.)
This third option has the distinct advantage that you can now reasonably *test* your code.
Do you mean with pactest? Can you explain how? -- Pierre Neidhardt
participants (2)
-
Dave Reisner
-
Pierre Neidhardt