Kevin Wolf <kw...@redhat.com> writes: > Am 29.09.2014 um 15:05 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: >> >> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, >> >> BlockInterfaceType block_default_type) >> >> } >> >> >> >> /* Set legacy DriveInfo fields */ >> >> - dinfo = blk_legacy_dinfo(blk); >> >> + dinfo = g_malloc0(sizeof(*dinfo)); >> >> dinfo->enable_auto_del = true; >> >> dinfo->opts = all_opts; >> >> - >> >> dinfo->cyls = cyls; >> >> dinfo->heads = heads; >> >> dinfo->secs = secs; >> >> dinfo->trans = translation; >> >> - >> >> dinfo->type = type; >> >> dinfo->bus = bus_id; >> >> dinfo->unit = unit_id; >> >> dinfo->devaddr = devaddr; >> >> - >> >> dinfo->serial = g_strdup(serial); >> >> + blk_set_legacy_dinfo(blk, dinfo); >> > >> > You don't like the grouping? >> >> No, because the comment appears as if it applied only to the first >> group. >> >> This is how this spot looks at the end of the series: >> >> /* Set legacy DriveInfo fields */ >> dinfo = g_malloc0(sizeof(*dinfo)); >> dinfo->opts = all_opts; >> dinfo->cyls = cyls; >> dinfo->heads = heads; >> dinfo->secs = secs; >> dinfo->trans = translation; >> dinfo->type = type; >> dinfo->bus = bus_id; >> dinfo->unit = unit_id; >> dinfo->devaddr = devaddr; >> dinfo->serial = g_strdup(serial); >> blk_set_legacy_dinfo(blk, dinfo); >> >> Could do this instead: >> >> dinfo = g_malloc0(sizeof(*dinfo)); >> dinfo->opts = all_opts; >> >> dinfo->cyls = cyls; >> dinfo->heads = heads; >> dinfo->secs = secs; >> dinfo->trans = translation; >> >> dinfo->type = type; >> dinfo->bus = bus_id; >> dinfo->unit = unit_id; >> dinfo->devaddr = devaddr; >> dinfo->serial = g_strdup(serial); >> >> blk_set_legacy_dinfo(blk, dinfo); >> >> The comment is next to useless anyway. Got a preference? > > The reason why it's there is that I like to use comments to give > "headlines" to the blocks of code. In drive_new(), I can only read the > comments without looking at the code and understand what functionality > is implemented at a high level. > > So for me the "headline" is valid until the next one appears (except > that this is the last one) and it's good as it is today. Your taste > differs, as it seems. > > If I have to choose between your two alternatives, I'll reluctantly vote > for removing the empty lines, because making it part of the "Actual > block device init" block by removing the comment makes even less sense.
Okay, you care for the headline comment and the grouping more than I dislike them, so I'll keep them both.