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