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
}
>
>> 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
