Hi!
On Mon, 2010-06-07 at 22:22:00 +0200, Raphael Hertzog wrote:
> On Mon, 07 Jun 2010, Guillem Jover wrote:
> > In vim I use something like:
> >
> > ,---
> > let c_space_errors=1
> > set list
> > set listchars=tab:»-,trail:·
> > `---
> >
> > to assist me spot those.
>
> Thanks, I find this setting too much annoying. I just added this to catch
> common mistakes:
> let c_space_errors=1
> highlight EightSpaces ctermbg=lightred guibg=lightred
> match EightSpaces /\(^\|\t\)/
>
> I find this less intrusive than seeing weird chars for all tabs.
I've just got used to seeing them, and actually when I use an
editor which does not show them, I feel naked. :)
> > > +static int
> > > +filter_altdir(const struct dirent *entry)
> > > +{
> > > + if (strcmp(entry->d_name, ".") == 0 ||
> > > + strcmp(entry->d_name, "..") == 0 ||
> > > + (strlen(entry->d_name) > strlen(DPKG_TMP_EXT) &&
> > > + strcmp(entry->d_name + strlen(entry->d_name) -
> > > + strlen(DPKG_TMP_EXT), DPKG_TMP_EXT) == 0))
> >
> > There's one tab instead of spaces in the last strlen; tabs to indent,
> > spaces to align (recurring).
>
> Can we revisit this requirement? I think it's useless and hard to
> detect/fix automatically (on the contrary using spaces here is catched
> as error by the simple syntax highlighting rule mentionned above).
(Well I think that match is wrong then :)
> The time spent reviewing those is not time usefully spent IMO. And I plan
> to write a check_patch.pl script that should catch all stylistic issues we
> want to see fixed and I don't see how I could enforce this one.
Personally I think that using tabs for indentation and spaces for
aligning, is the only style which is sane and democratic! :), it has
the nicest properties of them all, it just seems to me tools might be
lacking.
It preserves the structure of the text when quoting, it does too when
changing the tab size (so it caters for different tastes). If you have
them visible, it also helps (like if it was a ruler) see where each
indentation level begins. Using tabs (instead of spaces) for indenting
is the easiest way to guarantee there's always the correct amount of
spacing (not more or less, as in a missing space).
I don't think a checker will be able to catch all stylistic problems
anyway w/o understanding C in some cases, and having taste in others :).
Some constructs might be just plain ugly, and such a tool will not be
able to spot them either. A bug report to indent would be more
worthwhile (something I should have done long time ago), then we
could just ask people to run indent over it with a set of options.
I don't actually mind entirely meanwhile having to fix those after the
fact, or when applying them myself. So I'd really like to keep the
style as it is. And just for reference (but I think I might have
mentioned this before) I consider the current mix of tabs and spaces
in the perl code base just broken, but as Frank (at the time) and
you (later on) seemed to be fine with it, it just didn't seem worth to
try to push for a change there.
> [ alternative_reset ]
> > > + }
> > > + alt->modified = false;
> >
> > The current funciton name does not seem to quite fit with the current
> > functionality, reinitialize to NULL the struct members after freeing
> > them. This is also safer in case we try to reuse or free again some
> > freed pointer.
>
> That's already the case. "while (alt->)" means the loop will
> end when the member is NULL, so they are effectively set to NULL in the
> process.
Ah true!
> > I'd rather not see such hardcoded limits, as there's systems were file
> > names are not restricted. OTOH the rest of the dpkg code base has
> > similar limits for conffile paths for example and others, and dpkg is
> > currently also limited by the tar path limit, although u-a can be used
> > to manage non-dpkg-managed paths.
>
> Ok, used a dynamic buffer that I grow on demand.
Perfect!
> > > + line = fgets(buf, sizeof(buf), ctx->fh);
> > > + if (line == NULL) {
> > > + if (errno)
> > > + ctx->bad_format(ctx, _("while reading %s: %s"),
> > > + name, strerror(errno));
> > > + ctx->bad_format(ctx, _("unexpected end of file while trying "
> > > +"to read %s"), name);
> > > + }
> > > +
> > > + len = strlen(line);
> > > + if (line[len - 1] != '\n') {
> >
> > The line could be empty so this would access out of bounds (and will
> > most probably cause a crash on fortify enabled builds).
>
> No, if the line is empty (not even a newline), fgets() returns NULL.
It will be empty if the line starts with a literal NUL character (w/ or
w/o newline).
> > > +static const char *
> > > +get_argv_string(int argc, char **argv)
> > > +{
> > > + static char string[1024];
> > > + int i;
> > > +
> > > + string[0] = '\0';
> > > + for (i = 1; i < argc; i++) {
> > > + if (strlen(string) + strlen(argv[i]) + 2 > sizeof(string))
> > > +
