Re: GSoC - Code Formatting and Style Checking for RTEMS score
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?
On Thu, May 13, 2021 at 3:49 PM Gedare Bloom wrote:
> On Wed, May 12, 2021 at 2:18 PM Ida Delphine wrote:
> >
> > Hello everyone,
> > Still waiting for some feedback :)
> >
> > Cheers,
> > Ida.
> >
> > On Mon, 10 May 2021, 5:59 am Ida Delphine, 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 wrote:
> >>>
> >>>
> >>>
> >>> On Thu, May 6, 2021 at 12:47 PM Christian Mauderer
> 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
> 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 do
RE: [PATCH v1 0/2] [libbsd] Install correct machine include headers
Ping :)
> -Original Message-
> From: Gedare Bloom
> Sent: Monday, May 10, 2021 10:55 PM
> To: Sommer, Jan
> Cc: devel@rtems.org
> Subject: Re: [PATCH v1 0/2] [libbsd] Install correct machine include headers
>
> I can't review these currently, just want to put a note out there.
> Hopefully someone else gets to it. Otherwise ping it in a week or two and I
> might have more time to look myself..
>
> On Mon, May 10, 2021 at 11:26 AM Jan Sommer
> wrote:
> >
> > Hello,
> >
> > This is a follow-up on this discussion regarding the installed header
> > files in libbsd:
> > https://lists.rtems.org/pipermail/devel/2021-April/066211.html
> >
> > The current situation is, that for example for all machines the bus.h
> > is installed from within the rtemsbsd directory
> (https://git.rtems.org/rtems-libbsd/tree/rtemsbsd/include/machine/bus.h).
> >
> > According to the file docu it originates from the amd64 version of this
> > file.
> > It also has the following section in it which we ran into when compiling
> Chris' ptpd2 archive:
> >
> > #ifdef __i386__
> > #error "your include paths are wrong"
> > #endif
> >
> > This patchset does the following:
> > - Add the target dependent machine include directory to 'header-paths'
> > in libbsd.py
> > - Import (mostly) '_bus.h', 'bus.h' and 'cpufunc.h' for the targets
> > from freebsd-org
> > - Remove those header files from rtemsbsd directory
> >
> > As a result the matching versions for machine dependent header files
> > are now installed for each BSP.
> > Would this be an acceptable solution?
> >
> > So far I compiled some BSPs for i386, arm, aarch64, powerpc, riscv, sparc
> and sparc64 to check that they still compile after the changes.
> > Are there any other architectures which should be included?
> >
> > I ran into one problem regarding the compilation of
> > rtemsbsd/sys/dev/dw_mmc/dw_mmc.c:105
> >
> > > return (bus_space_read_4(0, sc->bushandle, off));
> >
> > The first parameter creates an error for riscv (I think because
> > dereferencing
> a NULL pointer).
> > Are there any suggestion how to solve it (I am not familiar with the bus
> space and there is a lot of macro magic going on)?
> >
> > Also, I am not sure if I always added the header files in the right
> > module in libbsd.py. Any suggestions where to put them instead would be
> welcome.
> >
> > Best regards,
> >
> > Jan
> >
> > Jan Sommer (2):
> > rtemsbd: Remove machine dependent files and use the ones from
> freebsd
> > machine: Add machine dependent header files to libbsd.py
> >
> > freebsd/sys/arm/include/machine/_bus.h| 47 +
> > freebsd/sys/arm/include/machine/bus.h | 769
> > freebsd/sys/arm64/include/machine/_bus.h | 46 +
> > freebsd/sys/arm64/include/machine/bus.h | 469 ++
> > freebsd/sys/powerpc/include/machine/_bus.h| 50 +
> > freebsd/sys/powerpc/include/machine/bus.h | 467 ++
> > freebsd/sys/riscv/include/machine/_bus.h | 46 +
> > freebsd/sys/riscv/include/machine/bus.h | 469 ++
> > freebsd/sys/riscv/include/machine/cpufunc.h | 135 +++
> > freebsd/sys/riscv/include/machine/riscvreg.h | 246 +
> > .../sys/sparc}/include/machine/_bus.h | 0
> > .../sys/sparc}/include/machine/bus.h | 0
> > freebsd/sys/sparc/include/machine/cpufunc.h | 0
> > freebsd/sys/sparc64/include/machine/_bus.h| 41 +
> > freebsd/sys/sparc64/include/machine/bus.h | 852
> ++
> > libbsd.py | 26 +-
> > rtemsbsd/include/machine/cpufunc.h| 1 -
> > waf_libbsd.py | 2 -
> > 18 files changed, 3660 insertions(+), 6 deletions(-) create mode
> > 100644 freebsd/sys/arm/include/machine/_bus.h
> > create mode 100644 freebsd/sys/arm/include/machine/bus.h
> > create mode 100644 freebsd/sys/arm64/include/machine/_bus.h
> > create mode 100644 freebsd/sys/arm64/include/machine/bus.h
> > create mode 100644 freebsd/sys/powerpc/include/machine/_bus.h
> > create mode 100644 freebsd/sys/powerpc/include/machine/bus.h
> > create mode 100644 freebsd/sys/riscv/include/machine/_bus.h
> > create mode 100644 freebsd/sys/riscv/include/machine/bus.h
> > create mode 100644 freebsd/sys/riscv/include/machine/cpufunc.h
> > create mode 100644 freebsd/sys/riscv/include/machine/riscvreg.h
> > rename {rtemsbsd => freebsd/sys/sparc}/include/machine/_bus.h (100%)
> > rename {rtemsbsd => freebsd/sys/sparc}/include/machine/bus.h (100%)
> > create mode 100644 freebsd/sys/sparc/include/machine/cpufunc.h
> > create mode 100644 freebsd/sys/sparc64/include/machine/_bus.h
> > create mode 100644 freebsd/sys/sparc64/include/machine/bus.h
> > delete mode 100644 rtemsbsd/include/machine/cpufunc.h
> >
> > --
> > 2.17.1
> >
> > ___
> > devel mailing list
> > devel@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
__
