On 14 May 2015 at 09:10, Bill Fischofer <bill.fischo...@linaro.org> 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
>
> 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.
>
> On Thu, May 14, 2015 at 7:52 AM, Mike Holmes <mike.hol...@linaro.org>
> wrote:
>
>>
>>
>> On 14 May 2015 at 08:41, Bill Fischofer <bill.fischo...@linaro.org>
>> 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 <mike.hol...@linaro.org>
>>> 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 <maxim.uva...@linaro.org> 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
>>>>> lng-odp@lists.linaro.org
>>>>> 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
>>>> lng-odp@lists.linaro.org
>>>> 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
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
