On 17.9.2013 22:57, Dominik Taborsky wrote:
> On Tue, 17 Sep 2013 11:22:30 +0200, Jiri Svoboda
> <[email protected]> wrote:
...
>>> On a side note, would it be a good idea to merge libmbr's error codes
>>> into
>>> errno.h?
>> Short answer: no.
>>
>> Long answer: Obviously the errno.h approach does not scale. Every
>> function
>> has its own set of ways it can fail and so it could have its own set of
>> error codes - that would be one extreme case. The other extreme case
>> would
>> be to put all these error codes into a single header/enum, with none
>> or some
>> unification.
>
> In that case, may I ask why there's USB error codes in errno.h?
It's not only USB error codes, but also file system and networking error
codes. However, we consider it a bad thing and you should not worsen the
situation.
>> I haven't had a chance to try running your code again. I've looked at
>> briefly and I still find some deviations from the official Cstyle,
>> such as:
>> (I am only including one exaple of each type of Cstyle error):
>>
>> * Copyright (c) 2012, 2013 Dominik Taborsky
>>
>> we only put the last year of modification in, such as:
>>
>> * Copyright (c) 2013 Dominik Taborsky
>>
>> I have to admit, though, that I am not sure whether the current way is a
>> good idea, we might discuss about changing it
>
> This is even more unimportant if you yourself don't know how it should
> be done. So, am I expected to fix this to later find out the rule has
> changed?
This is probably not a big issue and you will find variations here and
there.
>> input.h:
>> extern int get_input_line(tinput_t * in, char ** str);
>> extern uint8_t get_input_uint8(tinput_t * in);
>> extern uint32_t get_input_uint32(tinput_t * in);
>> extern uint64_t get_input_uint64(tinput_t * in);
>> extern size_t get_input_size_t(tinput_t * in); func_gpt.h:
>> extern int construct_gpt_label(label_t *);
>> extern int add_gpt_part (label_t *, tinput_t *);
>> extern int delete_gpt_part (label_t *, tinput_t *);
>> extern int destroy_gpt_label(label_t *);
>> extern int new_gpt_label (label_t *);
>> extern int print_gpt_parts (label_t *);
>> extern int read_gpt_parts (label_t *, service_id_t);
>> extern int write_gpt_parts (label_t *, service_id_t);
>> extern int extra_gpt_funcs (label_t *, tinput_t *, service_id_t);
>>
>>
>> Here's a block of function declarations that use tab for aligning - in
>> one
>> case you are aligning function names, in the other you are aligning the
>> opening brackets?!. You should not align either, use a single space
>> between
>> type and name (same for lists of variable declarations) and no space
>> between
>> name and opening bracket.
>
> I am sorry about using tabs for alignment, I thought I fixed that
> everywhere.
>
> But I don't agree with you on your second point - not aligning argument
> lists. I find that very handy and helpful. Looking up the function is
> fast, but finding the opening bracket slows me down. Aligning them makes
> it easy to spot differences.
It helps you to see that functions in general tend to have different
signatures? Most text editors with syntax highlighting will highlight
the brackets for you. And if you want to group a block of functions that
logically belong together, it is much better to use vertical spacing for
this purpose.
>> func_mbr.c:133:
>> printf("\t%10u %10u %10u %7u\n", it->start_addr,
>> it->start_addr + it
>> ->length, it->length, it->type);
>>
>> This line ends at column 117 - lines should not exceed 80 characters.
>
> As I said before, I make some exceptions to this rule. I know keeping
> short lines allow you to open several source files at once and show them
> side-by-side. But this is just a printf. It doesn't do anything
> important. You don't need to care what's in there...
Please don't make deliberate exceptions to the rules, especially when
there is a nice and natural way to break the long statement. It doesn't
really matter what the function at hand does.
>> func_mbr.c:
>> #include <stdio.h>
>> #include <errno.h>
>> #include <str_error.h>
>> #include <sys/types.h>
>>
>> #include "func_mbr.h"
>> #include "input.h"
>>
>> This is a clear violation of IWIU (include what you use). You are using
>> common.h to include libmbr.h and libgpt.h, but really func_mbr.c should
>> directly include libmbr.h.
>
> So the violation is that I don't include "libmbr.h"? Which is included
> by "func_mbr.h"?
>
>>
>> Speaking of which I don't think the headers should be named libmbr.h and
>> libgpt.h - after all the library names are 'mbr' and 'gpt'. libmbr and
>> libgpt are just part of the scheme of file names where the libraries are
>> stored.
>
> The trouble is, if you name it "mbr.h", you might think it describes MBR
> label only - not the library API as well. I think "libmbr.h" makes it
> clear what it defines. Any other opinions?
The current mainline probably uses both ways (e.g. libfs.h for libfs and
minix.h for libminix), but IMO the latter prevails, therefore I would
suggest using mere mbr.h.
>> func_gpt.c:
>> printf("%3zu %10" PRIu64 " %10" PRIu64 " %10" PRIu64 "
>> %3zu %s\
>> n",
>> i-1, gpt_get_start_lba(iter), gpt_get_end_lba(iter),
>> gpt_get_end_lba(iter) - gpt_get_start_lba(iter),
>> gpt_get_part_type(iter), gpt_get_part_name(iter));
>> libgpt.c:
>> label->gpt->header->pe_array_crc32 = host2uint32_t_le(compute_crc32(
>> (uint8_t *) label->parts->part_array,
>> fillries * e_size));
>> etc.
>>
>> Continuation lines should be indented by 4 spaces.
>
> Again, as I said before, the first indentation is for readability
> purposes. I'm sorry about the second one, must have slipped through...
It should nonetheless always be exactly 4 spaces.
> Sorry about those tabs. And my tab is 4 characters. BTW, I don't see how
> "80 chars per line" and "8-char tab" can get together. Seems
> contradicting to me...
You can have around 9 levels of indentation with these parameters, which
is still many more than any sane function would use.
>> libmbr.c:256:
>> &(label->mbr->raw_data)
>>
>> the parentheses are unnecessary, it should be just &label->mbr->raw_data
>
> That's not in the CStyle guide. And I prefer my style.
Perhaps it can be added there. But I don't think this is a matter of
style. It's just that the brackets are not necessary in this case.
>>
>> /* Encoding and writing first logical partition */
>> if (l != &(label->parts->list.head)) {
>>
>> had you used list_first()/list_next() you could check for l != NULL.
>
> Explained before.
We also have some other code which does this, especially in the kernel.
I guess you can change it to be more readable as Jiri suggested or just
leave it as it is.
>> /** Remove partition
>> * Removes partition by index, indexed from zero. When removing
>> extended
>> * partition, all logical partitions get removed as well.
>> *
>>
>> It should be indented/spaced like:
>>
>> /** Remove partition
>> *
>> * Remove partition by index, indexed from zero. When removing extended
>> * partition, all logical partitions are removed aswell.
>> *
>>
>
> CStyle guide reference?
The majority of the rest of the code does that. It's usually better to
write code which follows the same, possibly implied, conventions. It's
also not clear to me whether not having the empty line there would not
result in Doxygen inappropriately joining the \brief and \description.
You may have noticed that this very long e-mail (except for the
technical discussion at its end) is primarily about nitpicking your
deviations from the cstyle or from the style of the other code. I think
the cause which provokes an e-mail like this is rooted in contributors'
tendencies to make deliberate exceptions from various rules. It is easy
to say: "I don't like this and I'll do it my way, using my preferred
style." But it won't work in a multi-developer project like HelenOS. If
the contributors merely tried to comply as much as possible with the
rules, both written and implied, we could save ourselves some time. We
are being quite pedantic about it because it is clear that if the code
gets merged with cstyle issues, somebody will have to eventually fix
them and you can guess who that somebody will be. The other alternative
is that we end-up with as many cstyles as there are contributors, that
is: a complete chaos. If you think we are a sort of cstyle nazis, I
recommend you undergo a code inspection session or two in eg. Solaris
sustaining organization where exceptions from the cstyle are not
permitted at all.
Jakub
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/listinfo/helenos-devel
