GSoC - Code Formatting and Style Checking for RTEMS score

Tue Jun 15 21:50:42 UTC 2021

On Tue, Jun 15, 2021 at 3:40 PM Ida Delphine <idadelm at gmail.com> wrote:

> Thanks for the feedback. Will work on it.
>
> On Tue, 15 Jun 2021, 3:44 pm Gedare Bloom, <gedare at rtems.org> wrote:
>
>>
>>
>> On Tue, Jun 15, 2021 at 8:15 AM Kuan Hsun Chen <
>> kuan-hsun.chen at tu-dortmund.de> wrote:
>>
>>> Hello Ida,
>>>
>>> It looks nice! Is it possible to highlight the mismatching part in color
>>> or suggest the concrete revision? (So that the developer might learn the
>>> mistakes).
>>>
>>>
>> I would like the script to be a warning rather than an error. By that, I
>> mean if there is a style mismatch, it should print out there is a mismatch.
>> Maybe, we should have multiple modes, where the default is to cause an
>> error and abort the commit, but provide an override somehow. For example,
>> whitespace warnings work like that (in git-am/git-apply). I believe there
>> are git-config options that can be set to control the behavior.
>>
>> I also think there should be two modes of output, a simple one that says
>> "there were X style errors" and a more detailed one that shows the specific
>> errors. We can decide what should be the default behavior later, but these
>> would both be useful. For example, when working on a third-party source
>> code, I don't want to be inundated with style errors on every line of code,
>> and I need to be able to make the commit work to put in a fix.
>>
>
For third party code, is there a way to capture which directories and files
are not subject to the RTEMS Style? Then the script could just avoid doing
the check.

Identifying third party code seems to be an issue which comes up
periodically. It might be helpful to have that list. It also would help
when knowing what to ignore with Coverity.

Could take that further and have a set which generate errors, a set which
do not get style checks, and everything else is a warning. Maybe.

Thinking out loud, the bulk of the test suite could be made style tool
conformant since it should have very little third party code. Then it would
be an error to try to commit without adhering to the style.

--joel

>
>>
>>
>>> Best,
>>> Kuan-Hsun
>>>
>>>
>>> On Tue, Jun 15, 2021 at 3:45 PM Ida Delphine <idadelm at gmail.com> wrote:
>>>
>>>> Hello everyone,
>>>>
>>>> For this project, I am supposed to write a git pre-commit hook in which
>>>> runs over a patch checking if the style matches that of RTEMS and aborts in
>>>> case it doesn't. From the screenshot below you can see what happens in case
>>>> the style doesn't match in a patch. The commit doesn't happen and it
>>>> outputs the diff first showing what it looked like when committed and how
>>>> the patch's format is supposed to look like. It does not directly format
>>>> the patch. I am yet to do more tests to be sure there are no bugs but will
>>>> like to get some feedback if there is a better way it should look like.
>>>>
>>>> [image: hooks-output.png]
>>>>
>>>>
>>>> On Fri, Jun 4, 2021 at 10:50 PM Ida Delphine <idadelm at gmail.com> wrote:
>>>>
>>>>> Okay. I will do that.
>>>>>
>>>>> On Fri, 4 Jun 2021, 10:48 pm Gedare Bloom, <gedare at rtems.org> wrote:
>>>>>
>>>>>> On Fri, Jun 4, 2021 at 2:57 PM Ida Delphine <idadelm at gmail.com>
>>>>>> wrote:
>>>>>> >
>>>>>> > Okay, I will take a look.
>>>>>> >
>>>>>> > Regarding me asking a question in the appropriate clang-format
>>>>>> mailing list should it be just regarding the parentheses and braces being
>>>>>> aligned?
>>>>>> >
>>>>>> That would be the right question to ask, if you can't find a way to
>>>>>> align the closing parenthesis.
>>>>>>
>>>>>> You might also follow-up that old thread related to alignment of the
>>>>>> pointers.
>>>>>>
>>>>>> > On Fri, Jun 4, 2021 at 8:41 PM Joel Sherrill <joel at rtems.org>
>>>>>> wrote:
>>>>>> >>
>>>>>> >>
>>>>>> >>
>>>>>> >> On Fri, Jun 4, 2021 at 12:39 PM Gedare Bloom <gedare at rtems.org>
>>>>>> wrote:
>>>>>> >>>
>>>>>> >>> On Fri, Jun 4, 2021 at 8:47 AM Joel Sherrill <joel at rtems.org>
>>>>>> wrote:
>>>>>> >>> >
>>>>>> >>> >
>>>>>> >>> >
>>>>>> >>> > On Fri, Jun 4, 2021 at 12:24 AM Ida Delphine <idadelm at gmail.com>
>>>>>> wrote:
>>>>>> >>> >>
>>>>>> >>> >> Hello everyone,
>>>>>> >>> >>
>>>>>> >>> >> I applied the configuration Sebastian used and ran
>>>>>> clang-format on cpukit/score/src/threadqenque.c and so far these are the
>>>>>> differences I could notice...
>>>>>> >>> >> Below are some example areas in the code you can spot the
>>>>>> differences:
>>>>>> >>> >>
>>>>>> >>> >> In line 68, the ")" at the end of the parameter list needs to
>>>>>> be in a new row, but this doesn't seem to be supported in clang-format.
>>>>>> >>> >
>>>>>> >>> > If I understand correctly, clang-format does not like:
>>>>>> >>> >
>>>>>> >>> >
>>>>>> https://git.rtems.org/rtems/tree/cpukit/score/src/threadqenqueue.c
>>>>>> >>> >
>>>>>> >>> > which has the first parameter on its one line but wants the
>>>>>> first parameter
>>>>>> >>> > after the open parenthesis?
>>>>>> >>> >
>>>>>> >>> > The RTEMS style would seem to correspond to
>>>>>> AlignAfterOpenBracket being
>>>>>> >>> > set to AlwaysBreak
>>>>>> >>> >
>>>>>> >>> >>
>>>>>> >>> >> In line 142, if the function call is split into multiple rows,
>>>>>> the ");" should always be in a new row.
>>>>>> >>> >
>>>>>> >>> > Having the closing parenthesis on its own line may end up being
>>>>>> something
>>>>>> >>> > we have to change the RTEMS style on. I do not see an option in
>>>>>> their
>>>>>> >>> > documentation to do this. Unfortunate, since I like the
>>>>>> symmetry between
>>>>>> >>> > braces and parentheses.
>>>>>> >>> >
>>>>>> >>> >  Could you file an issue with them and/or ask a question the
>>>>>> appropriate
>>>>>> >>> > mailing list? Please cc Gedara and me. Give them an example.
>>>>>> Maybe
>>>>>> >>> > we are missing something.
>>>>>> >>> >>
>>>>>> >>> >> In line 201-202, we can see that the "*" of the pointers are
>>>>>> not aligned to the right.
>>>>>> >>> >
>>>>>> >>> >
>>>>>> >>> > This seems to be the issue Gedare mentioned which might have a
>>>>>> patch.
>>>>>> >>> > Follow up on that.
>>>>>> >>> >
>>>>>> >>> > But I think we had previously discussed this as a point we may
>>>>>> have to
>>>>>> >>> > concede and change RTEMS style on.
>>>>>> >>> >>
>>>>>> >>> >> You can check out the formatted file here -
>>>>>> https://pastebin.com/nDBrSSCP
>>>>>> >>> >
>>>>>> >>> >
>>>>>> >>> > Is it just the website or are blank line differences? It may
>>>>>> just be an
>>>>>> >>> > illusion. I think the spacing between the numbered lines is
>>>>>> greater
>>>>>> >>> > than in the git view. Just odd.
>>>>>> >>> >
>>>>>> >>> That's just the pastebin having more vertical padding between
>>>>>> consecutive lines.
>>>>>> >>
>>>>>> >>
>>>>>> >> That's what I thought but it did make the code look funny.
>>>>>> >>
>>>>>> >> Ida/Gedare.. does this mean there are only 3 style mismatch
>>>>>> issues? Or only
>>>>>> >> three in this file?
>>>>>> >>
>>>>>> >> Probably should try a few more files and see if there are other
>>>>>> discrepancies.
>>>>>> >>
>>>>>> >> And obviously work on the integration/automation of using the
>>>>>> tools. :)
>>>>>> >>
>>>>>> >> --joel
>>>>>> >>
>>>>>> >>>
>>>>>> >>>
>>>>>> >>> > --joel
>>>>>> >>> >>
>>>>>> >>> >>
>>>>>> >>> >>
>>>>>> >>> >> On Tue, Jun 1, 2021 at 5:36 PM Gedare Bloom <gedare at rtems.org>
>>>>>> wrote:
>>>>>> >>> >>>
>>>>>> >>> >>> On Mon, May 31, 2021 at 2:59 PM Ida Delphine <
>>>>>> idadelm at gmail.com> wrote:
>>>>>> >>> >>> >
>>>>>> >>> >>> > Hi Gedare,
>>>>>> >>> >>> >
>>>>>> >>> >>> > With regards to your comment on discord on me looking for a
>>>>>> tool that works on both patches and source files, it turns out clang-format
>>>>>> has that functionality already. Here's what I found -
>>>>>> https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
>>>>>> >>> >>> >
>>>>>> >>> >>> > Does it match what you have in mind?
>>>>>> >>> >>> >
>>>>>> >>> >>> Yes. I think we would want to not use the `-i` option but
>>>>>> instead pass
>>>>>> >>> >>> through and check the changes. I don't think we should
>>>>>> rewrite the
>>>>>> >>> >>> patches themselves, but instead we want to use a tool that
>>>>>> can be used
>>>>>> >>> >>> to check and approve the style of submitted patches. You
>>>>>> might need to
>>>>>> >>> >>> write a modified version of the clang-format-diff.py to use
>>>>>> as a
>>>>>> >>> >>> "checker" with ability to provide exceptions to the rules.
>>>>>> >>> >>>
>>>>>> >>> >>> Gedare
>>>>>> >>> >>>
>>>>>> >>> >>> > On Thu, May 13, 2021 at 3:49 PM Gedare Bloom <
>>>>>> gedare at rtems.org> wrote:
>>>>>> >>> >>> >>
>>>>>> >>> >>> >> On Wed, May 12, 2021 at 2:18 PM Ida Delphine <
>>>>>> idadelm at gmail.com> wrote:
>>>>>> >>> >>> >> >
>>>>>> >>> >>> >> > Hello everyone,
>>>>>> >>> >>> >> > Still waiting for some feedback :)
>>>>>> >>> >>> >> >
>>>>>> >>> >>> >> > Cheers,
>>>>>> >>> >>> >> > Ida.
>>>>>> >>> >>> >> >
>>>>>> >>> >>> >> > On Mon, 10 May 2021, 5:59 am Ida Delphine, <
>>>>>> idadelm at gmail.com> wrote:
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> Hello everyone,
>>>>>> >>> >>> >> >> Went through some previous emails and it turns out
>>>>>> Sebastian already came up with a configuration for clang format which works
>>>>>> well for RTEMS except for the fact that some configurations haven't been
>>>>>> implemented into clang-format yet. Using
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> AlignConsecutiveDeclarations: false
>>>>>> >>> >>> >> >> PointerAlignment: Right
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> Doesn't seem to work.
>>>>>> >>> >>> >> >> For example in the cpukit/score/src/threadq.c file,
>>>>>> something like
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> RTEMS_STATIC_ASSERT(
>>>>>> >>> >>> >> >> offsetof( Thread_queue_Syslock_queue, Queue.name )
>>>>>> >>> >>> >> >> == offsetof( struct _Thread_queue_Queue, _name ),
>>>>>> >>> >>> >> >> THREAD_QUEUE_SYSLOCK_QUEUE_NAME
>>>>>> >>> >>> >> >> );
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> RTEMS_STATIC_ASSERT(
>>>>>> >>> >>> >> >> sizeof( Thread_queue_Syslock_queue )
>>>>>> >>> >>> >> >> == sizeof( struct _Thread_queue_Queue ),
>>>>>> >>> >>> >> >> THREAD_QUEUE_SYSLOCK_QUEUE_SIZE
>>>>>> >>> >>> >> >> );
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> #if defined(RTEMS_SMP)
>>>>>> >>> >>> >> >> void _Thread_queue_Do_acquire_critical(
>>>>>> >>> >>> >> >> Thread_queue_Control *the_thread_queue,
>>>>>> >>> >>> >> >> ISR_lock_Context *lock_context
>>>>>> >>> >>> >> >> )
>>>>>> >>> >>> >> >> {
>>>>>> >>> >>> >> >> _Thread_queue_Queue_acquire_critical(
>>>>>> >>> >>> >> >> &the_thread_queue->Queue,
>>>>>> >>> >>> >> >> &the_thread_queue->Lock_stats,
>>>>>> >>> >>> >> >> lock_context
>>>>>> >>> >>> >> >> );
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> becomes this after using the given configuration
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> RTEMS_STATIC_ASSERT(sizeof(Thread_queue_Syslock_queue)
>>>>>> ==
>>>>>> >>> >>> >> >> sizeof(struct _Thread_queue_Queue),
>>>>>> >>> >>> >> >> THREAD_QUEUE_SYSLOCK_QUEUE_SIZE);
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> #if defined(RTEMS_SMP)
>>>>>> >>> >>> >> >> void
>>>>>> _Thread_queue_Do_acquire_critical(Thread_queue_Control *the_thread_queue,
>>>>>> >>> >>> >> >> ISR_lock_Context *lock_context) {
>>>>>> >>> >>> >> >> _Thread_queue_Queue_acquire_critical(
>>>>>> >>> >>> >> >> &the_thread_queue->Queue,
>>>>>> &the_thread_queue->Lock_stats, lock_context);
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> >> Everything seems manageable except for this alignment
>>>>>> issue...
>>>>>> >>> >>> >> >> This also throws more light on the changes using
>>>>>> clang-format (
>>>>>> https://lists.rtems.org/pipermail/devel/2018-December/024145.html)
>>>>>> >>> >>> >> >>
>>>>>> >>> >>> >> I think we're willing to concede the pointer alignment.
>>>>>> However, it
>>>>>> >>> >>> >> would be worth spending some time to see if
>>>>>> >>> >>> >> https://reviews.llvm.org/D27651 can be made to work. The
>>>>>> current state
>>>>>> >>> >>> >> of the code would need to be compared to the patch on that
>>>>>> review
>>>>>> >>> >>> >> board.
>>>>>> >>> >>> >>
>>>>>> >>> >>> >> Beyond that, documenting the clang-format options to use
>>>>>> is next, and
>>>>>> >>> >>> >> then identifying a plan how to invoke clang-format during
>>>>>> a git
>>>>>> >>> >>> >> workflow is needed.
>>>>>> >>> >>> >>
>>>>>> >>> >>> >> >> On Thu, May 6, 2021 at 8:05 PM Joel Sherrill <
>>>>>> joel at rtems.org> wrote:
>>>>>> >>> >>> >> >>>
>>>>>> >>> >>> >> >>>
>>>>>> >>> >>> >> >>>
>>>>>> >>> >>> >> >>> On Thu, May 6, 2021 at 12:47 PM Christian Mauderer <
>>>>>> oss at c-mauderer.de> wrote:
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> Hello Ida and Gedare,
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> On 06/05/2021 06:26, Gedare Bloom wrote:
>>>>>> >>> >>> >> >>>> > hi Ida,
>>>>>> >>> >>> >> >>>> >
>>>>>> >>> >>> >> >>>> > On Wed, May 5, 2021 at 3:21 PM Ida Delphine <
>>>>>> idadelm at gmail.com> wrote:
>>>>>> >>> >>> >> >>>> >>
>>>>>> >>> >>> >> >>>> >> Hello everyone,
>>>>>> >>> >>> >> >>>> >>
>>>>>> >>> >>> >> >>>> >> Regarding this project (
>>>>>> https://devel.rtems.org/ticket/3860) I went with clang-format as we
>>>>>> all agreed. I have tested it on some "score" files and it made some changes
>>>>>> which I don't think are very much in line with the RTEMS coding style.
>>>>>> However, it wasn't really clear if we will chage the RTEMS coding style or
>>>>>> try to make changes to clang-format to fit the style.
>>>>>> >>> >>> >> >>>> >> Please will love to know the best option.
>>>>>> >>> >>> >> >>>> >>
>>>>>> >>> >>> >> >>>> > We will likely need to consider our choices
>>>>>> carefully. If we can find
>>>>>> >>> >>> >> >>>> > a suitably close style that is already
>>>>>> well-supported by clang, and
>>>>>> >>> >>> >> >>>> > get consensus from the maintainers on a change,
>>>>>> then that might be the
>>>>>> >>> >>> >> >>>> > best route forward.
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> +1
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> > I think the first thing to do is take the examples
>>>>>> >>> >>> >> >>>> > that have been shown by Sebastian that are "close"
>>>>>> but not quite
>>>>>> >>> >>> >> >>>> > perfect, and identify the cases where they differ
>>>>>> with RTEMS style in
>>>>>> >>> >>> >> >>>> > order to present for discussion here. If consensus
>>>>>> can't be reached to
>>>>>> >>> >>> >> >>>> > change the style, then we would need to have a plan
>>>>>> for how to improve
>>>>>> >>> >>> >> >>>> > the existing tools for what we have.
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> I also found the following tool quite useful to play
>>>>>> with the clang
>>>>>> >>> >>> >> >>>> style config:
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> https://zed0.co.uk/clang-format-configurator/
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> Maybe it can help a bit to find out what certain
>>>>>> options mean.
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> >
>>>>>> >>> >>> >> >>>> > However, I think there is interest in doing less
>>>>>> work on the tool
>>>>>> >>> >>> >> >>>> > side, and more work on how to integrate it into our
>>>>>> workflows better.
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> +1
>>>>>> >>> >>> >> >>>
>>>>>> >>> >>> >> >>>
>>>>>> >>> >>> >> >>> I agree with all of this from the student perspective.
>>>>>> But we will have
>>>>>> >>> >>> >> >>> to come to some agreement on a machine producible
>>>>>> format to
>>>>>> >>> >>> >> >>> be able to use the integration. A report on what
>>>>>> doesn't match would
>>>>>> >>> >>> >> >>> give us something to chew on while Ida works the
>>>>>> integration.
>>>>>> >>> >>> >> >>>
>>>>>> >>> >>> >> >>> --joel
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>>
>>>>>> >>> >>> >> >>>> >
>>>>>> >>> >>> >> >>>> >> Cheers,
>>>>>> >>> >>> >> >>>> >> Ida.
>>>>>> >>> >>> >> >>>> >> _______________________________________________
>>>>>> >>> >>> >> >>>> >> devel mailing list
>>>>>> >>> >>> >> >>>> >> devel at rtems.org
>>>>>> >>> >>> >> >>>> >> http://lists.rtems.org/mailman/listinfo/devel
>>>>>> >>> >>> >> >>>> > _______________________________________________
>>>>>> >>> >>> >> >>>> > devel mailing list
>>>>>> >>> >>> >> >>>> > devel at rtems.org
>>>>>> >>> >>> >> >>>> > http://lists.rtems.org/mailman/listinfo/devel
>>>>>> >>> >>> >> >>>> >
>>>>>> >>> >>> >> >>>> _______________________________________________
>>>>>> >>> >>> >> >>>> devel mailing list
>>>>>> >>> >>> >> >>>> devel at rtems.org
>>>>>> >>> >>> >> >>>> http://lists.rtems.org/mailman/listinfo/devel
>>>>>>
>>>>> _______________________________________________
>>>> devel mailing list
>>>> devel at rtems.org
>>>> http://lists.rtems.org/mailman/listinfo/devel
>>>
>>>
>>>
>>> --
>>> Dr.-Ing.
>>> Kuan-Hsun Chen
>>> Postdoctoral Researcher
>>>
>>> *Technische Universität Dortmund *
>>> Department of Computer Science, Chair 12
>>> Design Automation of Embedded Systems
>>> Otto-Hahn Strasse 16, Room E19
>>> 44227 Dortmund
>>>
>>> Tel.: +49 231-755 61 11
>>> Fax: +49 231-755 61 16
>>> kuan-hsun.chen at tu-dortmund.de
>>> https://ls12-www.cs.tu-dortmund.de/~khchen
>>>
>>>
>>>
>>>
>>> *Wichtiger Hinweis: Die Information in dieser E-Mail ist vertraulich.
>>> Sie ist ausschließlich für den Adressaten bestimmt. Sollten Sie nicht der
>>> für diese E-Mail bestimmte Adressat sein, unterrichten Sie bitte den
>>> Absender und vernichten Sie diese Mail. Vielen Dank. Unbeschadet der
>>> Korrespondenz per E-Mail, sind unsere Erklärungen ausschließlich final
>>> rechtsverbindlich, wenn sie in herkömmlicher Schriftform (mit eigenhändiger
>>> Unterschrift) oder durch Übermittlung eines solchen Schriftstücks per
>>> Telefax erfolgen. Important note: The information included in this e-mail
>>> is confidential. It is solely intended for the recipient. If you are not
>>> the intended recipient of this e-mail please contact the sender and delete
>>> this message. Thank you. Without prejudice of e-mail correspondence, our
>>> statements are only legally binding when they are made in the conventional
>>> written form (with personal signature) or when such documents are sent by
>>> fax. *
>>>
>>>
>>>
>>>
>>> *Wichtiger Hinweis: Die Information in dieser E-Mail ist vertraulich.
>>> Sie ist ausschließlich für den Adressaten bestimmt. Sollten Sie nicht der
>>> für diese E-Mail bestimmte Adressat sein, unterrichten Sie bitte den
>>> Absender und vernichten Sie diese Mail. Vielen Dank. Unbeschadet der
>>> Korrespondenz per E-Mail, sind unsere Erklärungen ausschließlich final
>>> rechtsverbindlich, wenn sie in herkömmlicher Schriftform (mit eigenhändiger
>>> Unterschrift) oder durch Übermittlung eines solchen Schriftstücks per
>>> Telefax erfolgen. Important note: The information included in this e-mail
>>> is confidential. It is solely intended for the recipient. If you are not
>>> the intended recipient of this e-mail please contact the sender and delete
>>> this message. Thank you. Without prejudice of e-mail correspondence, our
>>> statements are only legally binding when they are made in the conventional
>>> written form (with personal signature) or when such documents are sent by
>>> fax. *
>>>
>> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210615/3b28c013/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hooks-output.png
Type: image/png
Size: 96240 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210615/3b28c013/attachment-0001.png>