GSoC - Code Formatting and Style Checking for RTEMS score

Tue Jun 15 20:40:39 UTC 2021

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.
>
>
>
>> 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/8605c4b5/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/8605c4b5/attachment-0001.png>