On 14 May 2015 at 18:17, Maxim Uvarov <[email protected]> wrote:
> > > On 14 May 2015 at 17:25, Mike Holmes <[email protected]> wrote: > >> >> >> On 14 May 2015 at 09:10, Bill Fischofer <[email protected]> >> wrote: >> >>> Well, we need to modify these new rules as these warnings are erroneous: >>> >>> *12:21:47* CHECK: Avoid CamelCase: <PRIu64>*12:21:47* #79: FILE: >>> platform/linux-generic/odp_buffer.c:79:*12:21:47* + >>> " pool %" PRIu64 "\n", >>> >>> >>> since the PRIxxx routines are hard-coded to these values. >>> >>> >> Agree, easy to fix >> >> >> >>> Also: >>> >>> >>> *12:21:58* WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet >>> addresses are __aligned(2)*12:21:58* #910: FILE: >>> platform/linux-generic/odp_packet_io.c:910:*12:21:58* + >>> memcpy(mac_addr, pktio_loop_mac, ETH_ALEN); >>> >>> >>> Shows why we can't just blindly copy Linux tools, as ether_addr_copy() is a >>> Linux internal API, not C99. >>> >>> >> Agree also easy to fix in the checkpatch rule file in the .chckpatch.conf >> possibly - so no patch needed >> >> > > Linux checkpatch is excellent here! memcpy used in odp api > odp_pktio_mac_addr() uses memcpy which is slow. And more likely we even > need separate helper for that. > > Take a look how linux copies mac addr: > > static inline void ether_addr_copy(u8 *dst, const u8 *src) > { > #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) > *(u32 *)dst = *(const u32 *)src; > *(u16 *)(dst + 4) = *(const u16 *)(src + 4); > #else > u16 *a = (u16 *)dst; > const u16 *b = (const u16 *)src; > > a[0] = b[0]; > a[1] = b[1]; > a[2] = b[2]; > #endif > } > > Link with disassembled code: http://permalink.gmane.org/gmane.linux.network/300096 > > >> >>> I'm also not overly enamored of these: >>> >>> >>> *12:21:49* CHECK: Comparison to NULL could be written "!file"*12:21:49* >>> #355: FILE: platform/linux-generic/odp_system_info.c:355:*12:21:49* + >>> if (file == NULL) { >>> >>> >>> as explicit comparisons to NULL are just easier to read and make the intent >>> very clear. >>> >>> >> That though is personal preference, I happen to agree but then we start >> the infinite discussion. That is why we follow Linux and let them have the >> discussions. We can add it to the agenda - but it really has to be a very >> snappy vote +1 or -1 for turning this off, we dont want to waste list >> bandwidth discussing merit, just decide in or out. >> >> >>> >>> I'm not sure what we're supposed to do with these: >>> >>> >>> *12:21:49* CHECK: architecture specific defines should be avoided*12:21:49* >>> #274: FILE: platform/linux-generic/odp_system_info.c:274:*12:21:49* +#if >>> defined __x86_64__ || defined __i386__ || defined __OCTEON__ || \ >>> >>> >> I do, as with my previous clean up it would be resolved by configure >> selecting a src file based on platform, See odp/platform/linux-generic/arch >> that cleans up odp_time that had the same issue. >> >> >>> >>> And these are just excessively nit-picky: >>> >>> >>> *12:21:54* CHECK: spaces preferred around that '*' (ctx:VxV)*12:21:54* #30: >>> FILE: platform/linux-generic/odp_time.c:30:*12:21:54* + return >>> (cycles*GIGA)/hz; >>> >>> >>> *12:21:55* CHECK: No space is necessary after a cast*12:21:55* #31: FILE: >>> platform/linux-generic/arch/linux/odp_time.c:31:*12:21:55* + sec = >>> (uint64_t) time.tv_sec; >>> >>> >> Personal preference again or we will start another debate. Anyone who >> wants to alter a rule should start a thread to get votes for in and out of >> that rule rather debate the merits on the list IMHO. >> >> > All of that is "personal preference". If we staid with checkpatch then > better to follow it. And from now just account that spaces. > > Maxim. > > > >> >> >>> >>> On Thu, May 14, 2015 at 7:52 AM, Mike Holmes <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On 14 May 2015 at 08:41, Bill Fischofer <[email protected]> >>>> wrote: >>>> >>>>> These rules seem excessively nit-picky. Why are we adding more >>>>> buckles on the the straightjacket? >>>>> >>>> >>>> checkpatch is not only focused on bugs, it is about consistent look and >>>> feel to save endless arguments on how much white space, can we use >>>> CamelCase, where to put parens etc. >>>> A submitter can interactively get that correct with the script rather >>>> than navigate endless different opinions on the list. >>>> >>>> We have in our charter to closely follow Linux code style, which we >>>> have now updated for the first time in over a year and a half approximatly. >>>> With the gradual maturation evident by 1.1 I think this is a good time to >>>> do it. >>>> >>>> >>>> What actual bugs did the previous checkpatch allow through? >>>>> >>>> >>>> You can look at the new results to see what is now finds, and it does >>>> see some items it classes as ERRORs, but we are fairly clean in the code >>>> base even with the old checkpatch due to coverity so there is not much >>>> there to see. >>>> >>>> >>>>> >>>>> On Thu, May 14, 2015 at 7:25 AM, Mike Holmes <[email protected]> >>>>> wrote: >>>>> >>>>>> CI has found a lot more to complain about in the current code base >>>>>> with the new checkpatch >>>>>> >>>>>> New results >>>>>> https://ci.linaro.org/job/odp-api-style-check/label=build/440/console >>>>>> >>>>>> I am not suggesting we go back and change it, as new code is added it >>>>>> will slowly clean up naturally, but the effect of the tighter rules can >>>>>> be >>>>>> seen. >>>>>> >>>>>> Previous rules here >>>>>> https://ci.linaro.org/job/odp-api-style-check/label=build/439/console >>>>>> >>>>>> On 14 May 2015 at 07:58, Maxim Uvarov <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Merged! >>>>>>> >>>>>>> Maxim. >>>>>>> >>>>>>> On 05/13/2015 13:49, Maxim Uvarov wrote: >>>>>>> >>>>>>>> Update checkpatch. Add our local fixes. Plus fix lenght limit for >>>>>>>> log functions. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Maxim. >>>>>>>> >>>>>>>> Maxim Uvarov (2): >>>>>>>> checkpatch: update to linux 4.1 rc-3 >>>>>>>> checkpatch: remove cunit warnings >>>>>>>> >>>>>>>> Taras Kondratiuk (1): >>>>>>>> checkpatch: remove line length limit for odp log functions >>>>>>>> >>>>>>>> scripts/checkpatch.pl | 2513 >>>>>>>> ++++++++++++++++++++++++++++++++++++++++++------- >>>>>>>> 1 file changed, 2183 insertions(+), 330 deletions(-) >>>>>>>> >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> lng-odp mailing list >>>>>>> [email protected] >>>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Mike Holmes >>>>>> Technical Manager - Linaro Networking Group >>>>>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM >>>>>> SoCs >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> lng-odp mailing list >>>>>> [email protected] >>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>>>> >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Mike Holmes >>>> Technical Manager - Linaro Networking Group >>>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM >>>> SoCs >>>> >>>> >>>> >>> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> >> >> >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
