Hi,
On 07/09/17 04:45, David Sommerseth wrote:
> On 23/08/17 07:30, Antonio Quartulli wrote:
>> This patch does not introduce any functional change.
>>
>> The code in route.c seems to have been written in different
>> periods by different people, without sticking to a clear
>> codestyle. For this reason the code in this file in not
>> consistent at all.
>>
>> Clean it up by:
>> - removing spaces from function invocations
>> - cutting line longer than 80 chars (where possible)
>> - moving function arguments on the same line when there is enough space
>> - adding empty line between var declarations and code
>> - adding empty line between code and final return
>> - adding empty line to make the code less sticky and easier to parse
>>
>> Signed-off-by: Antonio Quartulli <a...@unstable.cc>
>> ---
>>
>> Yes, this is a quite big patch. However, since we are planning a big
>> restructuring of the route.c file, it is better to take care of the
>> style in a separate patch (this) so that later we don't need to mixup
>> cleanups
>> and refactoring.
>>
>> Note that this patch is based on master plus the following patches:
>>
>> - ensure function declarations are compiled with their definitions
>> - fix a couple of typ0s in comments and strings
>> - route: avoid definition of unused variables in certain configurations
>> - convert *_inline attributes to bool
>> - reformatting: fix style in crypto*.{c, h}
>> - Allow learning iroutes with network made up of all 0s (only if netbits < 8)
>> - ifconfig-ipv6(-push): allow using hostnames
>>
>>
>> Applying this patch without the above, might lead to screams,
>> natural disasters and endless nightmares.
>
> I got it applying quite nicely (working my way through more patches
> now). And yes, I like that we clean up the coding style further in this
> file. But unfortunately, I'll have to say NAK in this round.
> I am not sure that what you get by applying this patch without the rest is 100% correct. But I can rebase this patch on top of master if you are willing to work on this one before the others mentioned in the patch message. > - Many places you replace spaces with tabs. I must have messed up my vim style file recently. will recheck. Thanks! > - There are several scenarios where our uncrustify config actually > improves your patch further (see the attachment). Oh ok. I never thought about re-running uncrustify. Thanks for the suggestion. > - And the contradictions like the ones below > >> -static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, >> unsigned int flags, const struct route_gateway_info *rgi, const struct >> env_set *es); >> +static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, >> + unsigned int flags, >> + const struct route_gateway_info *rgi, >> + const struct env_set *es); > > vs > >> static void >> -delete_route(struct route_ipv4 *r, >> - const struct tuntap *tt, >> - unsigned int flags, >> - const struct route_gateway_info *rgi, >> - const struct env_set *es) >> +delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int >> flags, >> + const struct route_gateway_info *rgi, const struct env_set *es) > > I think the change you do in the former one is also more readable than > squeezing everything into as few lines as possible, especially when > there's lots of arguments. Honestly I only applied the formal rule "a line should not be longer than 80 chars". This is what I grasped from our codestyle. Is this not correct? In the first case less arguments fit on the first line because of the return type. In the second case there is more space available. This is a contradiction given by the codingstyle itself. Should we use another rule to decide how function declarations and definitions should look like? I think in the rest of the code we tried to apply the line length rule too. Let me know how this should be arranged otherwise (and let's write it in the codingstyle page) :) > > Our uncrustify config doesn't touch these details of function > declarations, as tun.c and route.c is fairly extreme in variations here. > So we let that pass on the reformatting patch before the v2.4 release, > to take care of them manually, as we didn't spend much extra time > looking at more tweaks for uncrustify to make the result readable. But > I'm not sure we documented our preferences on function declarations, I > don't recall that now. I couldn't find anything about it in [1] > > Even though we are not united in the use of uncrustify after the > reformatting patches we did in December, I think it makes sense to at > least double check what uncrustify would change and consider those. The > lesser the gap is to that result, the easier it will be to have a > consistent coding style over the complete code base. Do you think it might be reasonable to document all the rules in the CodingStyle wikipage? Running uncrustify does not immediately "teach" what are the rules we want contributors to follow. > > For reference, the uncrustify command line I used was: > > $ uncrustify -c dev-tools/uncrustify.conf \ > --no-backup -l C -p debug.uncr \ > src/openvpn/route.c > Thanks! -- Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
