GSoC - Code Formatting and Style Checking for RTEMS score

Tue Jun 15 14:44:46 UTC 2021

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.

> 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. *
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210615/b4706b0b/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/b4706b0b/attachment-0001.png>