GSoC - Code Formatting and Style Checking for RTEMS score

Fri Jun 4 21:50:15 UTC 2021

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210604/116eba32/attachment-0001.html>