Jason King writes: > Looking for reviewers for http://cr.opensolaris.org/~jbk/ls -- This > adds a number of GNU compatibility options (including color) to > Solaris ls.
ls.c 253,255,336,338,2954: nit: suggest using ANSI '(void)' here. 277,304,305,333,340-351: nit: initialization to zero isn't needed. 330: it'd be nice to avoid having to check for NULL (and just set the value of time_fmt_new to be equal to time_fmt_old in those cases), but it looks like that might be hard. 336-338: these seem misplaced; do they belong near line 255? 374: why declare this table inside main()? Suggest moving out. 484,582: is duplicating this string necessary? I thought that optarg pointed to modifiable storage. 488-513: this doesn't look like the set supported by GNU ls (kB, K, MB, M, GB, G, ...), nor does it look like the -h/-H used by df and du. (We probably should have caught this in ARC review, but I think the assumption of those reading the case was that it would conform to GNU's behavior for the options that were implemented.) 518: should probably protect against negative and zero here. 542,2552: stray XXX comments need to be removed before integration. (Design issues need to be resolved.) 560,568-569: should these new formats also have #defines like the others? 584: these 'continue' statements look more like 'exit(1)' to me. 590,598: why does this need to be allocated, and if it does need to be allocated, why is calloc required? 593: comment is confusing; what's actually being replaced is the first character in the array, which happens to be '\0'. 608: I doubt that this code works; nothing ever copies 'tmp' (or optarg) into 'told'. Thus, the 'told' string is always exact " " (two spaces), set by lines 594 and 595. 612: does getting here represent an error in "time-style"? 627-629: This was cited as an existing Solaris ls flag in the ARC case. Why is it being added? (I'm guessing that was just a minor mistake in the ARC materials ...) 782: range checks? (Perhaps lines 922-924 should just be taken outside of the 'if' block starting at line 912, so that num_cols is always repaired if in error, no matter whether it comes from the user or from the system.) 880: 'U' wasn't included here. 1217: this will drop core if block_size isn't validated (at least non-zero) when set by users. 1219: not your code, but this test does nothing. Just using "%7lld" all the time would work fine. 1460: C Coding Standards: '==' operator goes at end of previous line. 1717,1732: what are these changes about? They don't seem related to this project. (Do they fix a bug ... ?) 2495: I think this should be '!eflg && Eflg' to capture the original logic. 2496: can 'fstr' potentially be NULL here due to the convention used for time_fmt_new? 2553: this looks like a functional change to me. Why would we print these things at all if Eflg isn't set? The old code didn't do that. 2537,2577: nit: save is unconditional, so restore can be as well; 'if' not needed here, and code is safer without. 2583: could use a comment or two (at least a block comment). 2584,2589,2683,2767,2793,2795,2934, et al: C Coding Standard: consider using better variable names. 2774: not sure what this comment means. If not now, when? 2776: this skips over (does not visit) lsc_colors[0]. Is that intentional? 2788: nit: missing blank line between variable declarations and executable code. 2788: when does tparm return (char *)-1? 2833: nit: could be const. 2861-2684: unnecessary; check at line 2867 will catch this. 2875: why strdup? 2904-2919: this doesn't appear to work. The value computed is overwritten at line 2932. Did you mean to use 'attr' in each of these assignments? 2899: consider using strtol(p, NULL, 10) here to avoid accidental octal interpretation. 2918: this value doesn't seem to be used in the 'default_colorstr' string. It's a good thing, because atoi would likely have trouble with "08" (which is an illegal octal value). 2980: why? (Concern about modifying LS_COLORS in the environment ... ?) -- James Carlson, Solaris Networking <james.d.carl...@sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677 _______________________________________________ opensolaris-code mailing list opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
