Re: svn commit: r366993 - head/sys/net
On Tue, Oct 27, 2020 at 2:23 PM Konstantin Belousov wrote: > On Tue, Oct 27, 2020 at 02:52:38PM -0400, Ed Maste wrote: > > On Sat, 24 Oct 2020 at 06:46, Konstantin Belousov > wrote: > > > > > > > -#include > > > sys/systm.h should come right after sys/param.h. These two headers are > > > exceptions to the normal alphabetical rule. > > > > style(9) claims that sys/types.h and sys/param.h are the special headers: > > > > Kernel include files (sys/*.h) come first. If is > needed > > for __FBSDID(), include it first. If either or > > is needed, include it before other include files. > > ( includes ; do not include both.) The > > remaining kernel headers should be sorted alphabetically. > > > > We should add sys/systm.h to style(9) as another special case if > necessary. > Yes sys/systm.h is very special because it defines a lot of basic things, > like asserts, printf(9), and so on, and because it includes enough other > popular headers. Its intent is to gather all common stuff, which makes > it both convenient and necessary to go before other (but sys/param.h is > more > prioritized). > So sorted after sys/types.h or sys/param.h should be documented in style(9). https://reviews.freebsd.org/D26981 has a proposed change. Warner > > > > > > -infiniband_ipv4_multicast_map(uint32_t addr, > > > > -const uint8_t *broadcast, uint8_t *buf) > > > > +infiniband_ipv4_multicast_map( > > > > +uint32_t addr, const uint8_t *broadcast, uint8_t *buf) > > > And this is arguably regression, we fill line up to columns 72-80 > before > > > splitting to the continuation line. > > > > I agree with kib and arichardson has a clang-format fix for this in > > D26978. That said I can see an argument for it being easier to read > > with all function args on the same line. > > > > > > - m = NULL; /* mbuf is consumed by > resolver */ > > > > + m = NULL; /* mbuf is consumed by > resolver */ > > > This is a regression as well, in-line comment is typically preceeded > by tab. > > > > I suspect this is hard to do in an automated fashion in a way that's > > sensible overall (i.e., lining up inline comments on different lines). > > > > This could be a case where we decide to just accept it when someone > > uses clang-format on new code, and just discourage sweeping > > clang-format changes on existing code. > ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Tue, Oct 27, 2020 at 02:52:38PM -0400, Ed Maste wrote: > On Sat, 24 Oct 2020 at 06:46, Konstantin Belousov wrote: > > > > > -#include > > sys/systm.h should come right after sys/param.h. These two headers are > > exceptions to the normal alphabetical rule. > > style(9) claims that sys/types.h and sys/param.h are the special headers: > > Kernel include files (sys/*.h) come first. If is needed > for __FBSDID(), include it first. If either or > is needed, include it before other include files. > ( includes ; do not include both.) The > remaining kernel headers should be sorted alphabetically. > > We should add sys/systm.h to style(9) as another special case if necessary. Yes sys/systm.h is very special because it defines a lot of basic things, like asserts, printf(9), and so on, and because it includes enough other popular headers. Its intent is to gather all common stuff, which makes it both convenient and necessary to go before other (but sys/param.h is more prioritized). > > > > -infiniband_ipv4_multicast_map(uint32_t addr, > > > -const uint8_t *broadcast, uint8_t *buf) > > > +infiniband_ipv4_multicast_map( > > > +uint32_t addr, const uint8_t *broadcast, uint8_t *buf) > > And this is arguably regression, we fill line up to columns 72-80 before > > splitting to the continuation line. > > I agree with kib and arichardson has a clang-format fix for this in > D26978. That said I can see an argument for it being easier to read > with all function args on the same line. > > > > - m = NULL; /* mbuf is consumed by > > > resolver */ > > > + m = NULL; /* mbuf is consumed by resolver */ > > This is a regression as well, in-line comment is typically preceeded by tab. > > I suspect this is hard to do in an automated fashion in a way that's > sensible overall (i.e., lining up inline comments on different lines). > > This could be a case where we decide to just accept it when someone > uses clang-format on new code, and just discourage sweeping > clang-format changes on existing code. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, 24 Oct 2020 at 06:46, Konstantin Belousov wrote: > > > -#include > sys/systm.h should come right after sys/param.h. These two headers are > exceptions to the normal alphabetical rule. style(9) claims that sys/types.h and sys/param.h are the special headers: Kernel include files (sys/*.h) come first. If is needed for __FBSDID(), include it first. If either or is needed, include it before other include files. ( includes ; do not include both.) The remaining kernel headers should be sorted alphabetically. We should add sys/systm.h to style(9) as another special case if necessary. > > -infiniband_ipv4_multicast_map(uint32_t addr, > > -const uint8_t *broadcast, uint8_t *buf) > > +infiniband_ipv4_multicast_map( > > +uint32_t addr, const uint8_t *broadcast, uint8_t *buf) > And this is arguably regression, we fill line up to columns 72-80 before > splitting to the continuation line. I agree with kib and arichardson has a clang-format fix for this in D26978. That said I can see an argument for it being easier to read with all function args on the same line. > > - m = NULL; /* mbuf is consumed by > > resolver */ > > + m = NULL; /* mbuf is consumed by resolver */ > This is a regression as well, in-line comment is typically preceeded by tab. I suspect this is hard to do in an automated fashion in a way that's sensible overall (i.e., lining up inline comments on different lines). This could be a case where we decide to just accept it when someone uses clang-format on new code, and just discourage sweeping clang-format changes on existing code. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, 24 Oct 2020 at 13:52, Alexey Dokuchaev wrote:
>
> On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
> > New Revision: 366993
> > URL: https://svnweb.freebsd.org/changeset/base/366993
> >
> > Log:
> > Run code through "clang-format -style=file" with some additional fixes.
> > No functional change.
> >
> > ...
> > @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
> >
> > #ifdef INET6
> > static inline void
> > -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
> > -const uint8_t *broadcast, uint8_t *buf)
> > +infiniband_ipv6_multicast_map(
> > +const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
> > {
>
> This is not how we format these in FreeBSD, please revert. It was correct
> before and no "fix" is need here.
>
> ./danfe
Unfortunately this is a limitation of the current clang-format
version. I've submitted a patch upstream as
https://reviews.llvm.org/D90246 and the config file change is
https://reviews.freebsd.org/D26978.
Alex
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On 10/24/20 8:19 PM, Ed Maste wrote: On Sat, 24 Oct 2020 at 11:27, Warner Losh wrote: Given we already have nice .clang-format, that does most of the job, maybe it's worth considering looking into tweaking it further to fix this part? It would be nice if we could finally offload all formatting issues to the tool and focus on the actual code :-) It would be nice if it produced one of the style(9) acceptable formats without disrupting things already acceptable. That's been the big problem with the tweaks to date... some things are fixed, others break. It's getting a lot closer, though Upstream clang-format comes with a script that can integrate with git, adding a `git clang-format` command. It will apply formatting to modified lines, leaving unchanged ones alone. I doubt any script can match style(9) perfectly. indent(1) with bde's flags came near in some edge cases but generally did a horrible job. Also, out style(9) is not mandatory for userland. Pedro. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, Oct 24, 2020 at 7:20 PM Ed Maste wrote: > On Sat, 24 Oct 2020 at 11:27, Warner Losh wrote: > > > >> Given we already have nice .clang-format, that does most of the job, > maybe it's worth considering looking into tweaking it further to fix this > part? > >> It would be nice if we could finally offload all formatting issues to > the tool and focus on the actual code :-) > > > > It would be nice if it produced one of the style(9) acceptable formats > without disrupting things already acceptable. That's been the big problem > with the tweaks to date... some things are fixed, others break. It's > getting a lot closer, though > > Upstream clang-format comes with a script that can integrate with git, > adding a `git clang-format` command. It will apply formatting to > modified lines, leaving unchanged ones alone. > As long as it's optional... Warner ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, 24 Oct 2020 at 11:27, Warner Losh wrote: > >> Given we already have nice .clang-format, that does most of the job, maybe >> it's worth considering looking into tweaking it further to fix this part? >> It would be nice if we could finally offload all formatting issues to the >> tool and focus on the actual code :-) > > It would be nice if it produced one of the style(9) acceptable formats > without disrupting things already acceptable. That's been the big problem > with the tweaks to date... some things are fixed, others break. It's getting > a lot closer, though Upstream clang-format comes with a script that can integrate with git, adding a `git clang-format` command. It will apply formatting to modified lines, leaving unchanged ones alone. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, 24 Oct 2020, 16:27 Warner Losh, wrote:
>
>
> On Sat, Oct 24, 2020, 7:38 AM Alexander V. Chernikov
> wrote:
>
>> 24.10.2020, 14:08, "Hans Petter Selasky" :
>> > On 2020-10-24 14:52, Alexey Dokuchaev wrote:
>> >> On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
>> >>> New Revision: 366993
>> >>> URL: https://svnweb.freebsd.org/changeset/base/366993
>> >>>
>> >>> Log:
>> >>> Run code through "clang-format -style=file" with some additional
>> fixes.
>> >>> No functional change.
>> >>>
>> >>> ...
>> >>> @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
>> >>>
>> >>>#ifdef INET6
>> >>>static inline void
>> >>> -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
>> >>> - const uint8_t *broadcast, uint8_t *buf)
>> >>> +infiniband_ipv6_multicast_map(
>> >>> + const struct in6_addr *addr, const uint8_t *broadcast, uint8_t
>> *buf)
>> >>>{
>> >>
>> >> This is not how we format these in FreeBSD, please revert. It was
>> correct
>> >> before and no "fix" is need here.
>> Given we already have nice .clang-format, that does most of the job,
>> maybe it's worth considering looking into tweaking it further to fix this
>> part?
>> It would be nice if we could finally offload all formatting issues to the
>> tool and focus on the actual code :-)
>>
>
> It would be nice if it produced one of the style(9) acceptable formats
> without disrupting things already acceptable. That's been the big problem
> with the tweaks to date... some things are fixed, others break. It's
> getting a lot closer, though
>
I've upstreamed a few fixes, but haven't got to the line
wrapping/continuation indentation stuff yet. That part of clang format is
not particularly easy to modify without breaking other stuff and I'm also
rather short on time right now, so probably won't get to it any time soon.
Alex
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, Oct 24, 2020, 7:38 AM Alexander V. Chernikov
wrote:
> 24.10.2020, 14:08, "Hans Petter Selasky" :
> > On 2020-10-24 14:52, Alexey Dokuchaev wrote:
> >> On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
> >>> New Revision: 366993
> >>> URL: https://svnweb.freebsd.org/changeset/base/366993
> >>>
> >>> Log:
> >>> Run code through "clang-format -style=file" with some additional
> fixes.
> >>> No functional change.
> >>>
> >>> ...
> >>> @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
> >>>
> >>>#ifdef INET6
> >>>static inline void
> >>> -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
> >>> - const uint8_t *broadcast, uint8_t *buf)
> >>> +infiniband_ipv6_multicast_map(
> >>> + const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
> >>>{
> >>
> >> This is not how we format these in FreeBSD, please revert. It was
> correct
> >> before and no "fix" is need here.
> Given we already have nice .clang-format, that does most of the job, maybe
> it's worth considering looking into tweaking it further to fix this part?
> It would be nice if we could finally offload all formatting issues to the
> tool and focus on the actual code :-)
>
It would be nice if it produced one of the style(9) acceptable formats
without disrupting things already acceptable. That's been the big problem
with the tweaks to date... some things are fixed, others break. It's
getting a lot closer, though
Warner
> Done.
> >
> > --HPS
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
24.10.2020, 14:08, "Hans Petter Selasky" :
> On 2020-10-24 14:52, Alexey Dokuchaev wrote:
>> On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
>>> New Revision: 366993
>>> URL: https://svnweb.freebsd.org/changeset/base/366993
>>>
>>> Log:
>>> Run code through "clang-format -style=file" with some additional fixes.
>>> No functional change.
>>>
>>> ...
>>> @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
>>>
>>> #ifdef INET6
>>> static inline void
>>> -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
>>> - const uint8_t *broadcast, uint8_t *buf)
>>> +infiniband_ipv6_multicast_map(
>>> + const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
>>> {
>>
>> This is not how we format these in FreeBSD, please revert. It was correct
>> before and no "fix" is need here.
Given we already have nice .clang-format, that does most of the job, maybe it's
worth considering looking into tweaking it further to fix this part?
It would be nice if we could finally offload all formatting issues to the tool
and focus on the actual code :-)
> Done.
>
> --HPS
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On 2020-10-24 14:52, Alexey Dokuchaev wrote:
On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
New Revision: 366993
URL: https://svnweb.freebsd.org/changeset/base/366993
Log:
Run code through "clang-format -style=file" with some additional fixes.
No functional change.
...
@@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
#ifdef INET6
static inline void
-infiniband_ipv6_multicast_map(const struct in6_addr *addr,
-const uint8_t *broadcast, uint8_t *buf)
+infiniband_ipv6_multicast_map(
+const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
{
This is not how we format these in FreeBSD, please revert. It was correct
before and no "fix" is need here.
Done.
--HPS
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
> New Revision: 366993
> URL: https://svnweb.freebsd.org/changeset/base/366993
>
> Log:
> Run code through "clang-format -style=file" with some additional fixes.
> No functional change.
>
> ...
> @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
>
> #ifdef INET6
> static inline void
> -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
> -const uint8_t *broadcast, uint8_t *buf)
> +infiniband_ipv6_multicast_map(
> +const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
> {
This is not how we format these in FreeBSD, please revert. It was correct
before and no "fix" is need here.
./danfe
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On 2020-10-24 12:46, Konstantin Belousov wrote: sys/systm.h should come right after sys/param.h OK, fixed. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r366993 - head/sys/net
On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
> Author: hselasky
> Date: Sat Oct 24 10:23:21 2020
> New Revision: 366993
> URL: https://svnweb.freebsd.org/changeset/base/366993
>
> Log:
> Run code through "clang-format -style=file" with some additional fixes.
> No functional change.
>
> Suggested by: kib@ and emaste@
> Differential Revision: https://reviews.freebsd.org/D26254
> MFC after: 1 week
> Sponsored by: Mellanox Technologies // NVIDIA Networking
>
> Modified:
> head/sys/net/if_infiniband.c
>
> Modified: head/sys/net/if_infiniband.c
> ==
> --- head/sys/net/if_infiniband.c Sat Oct 24 05:52:29 2020
> (r366992)
> +++ head/sys/net/if_infiniband.c Sat Oct 24 10:23:21 2020
> (r366993)
> @@ -30,44 +30,42 @@
> __FBSDID("$FreeBSD$");
>
> #include
> -#include
> -#include
> -#include
sys/systm.h should come right after sys/param.h. These two headers are
exceptions to the normal alphabetical rule.
> +#include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> -#include
> -#include
> +#include
>
> -#include
> -#include
> -#include
> +#include
> #include
> #include
> -#include
> -#include
> -#include
> +#include
> +#include
> #include
> -#include
> #include
> #include
> -
> -#include
> +#include
> +#include
> +#include
> +#include
> #include
> +#include
> #include
> -
> #include
> #include
>
> #include
>
> /* if_lagg(4) support */
> -struct mbuf *(*lagg_input_infiniband_p)(struct ifnet *, struct mbuf *);
> +struct mbuf *(*lagg_input_infiniband_p)(struct ifnet *, struct mbuf *);
>
> #ifdef INET
> static inline void
> -infiniband_ipv4_multicast_map(uint32_t addr,
> -const uint8_t *broadcast, uint8_t *buf)
> +infiniband_ipv4_multicast_map(
> +uint32_t addr, const uint8_t *broadcast, uint8_t *buf)
And this is arguably regression, we fill line up to columns 72-80 before
splitting to the continuation line.
> {
> uint8_t scope;
>
> @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
>
> #ifdef INET6
> static inline void
> -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
> -const uint8_t *broadcast, uint8_t *buf)
> +infiniband_ipv6_multicast_map(
> +const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
> {
> uint8_t scope;
>
> @@ -128,7 +126,7 @@ infiniband_bpf_mtap(struct ifnet *ifp, struct mbuf *mb
> {
> struct infiniband_header *ibh;
> struct ether_header eh;
> -
> +
> if (mb->m_len < sizeof(*ibh))
> return;
>
> @@ -198,7 +196,7 @@ infiniband_output(struct ifnet *ifp, struct mbuf *m, c
> if (error) {
> if (error == EWOULDBLOCK)
> error = 0;
> - m = NULL; /* mbuf is consumed by resolver
> */
> + m = NULL; /* mbuf is consumed by resolver */
This is a regression as well, in-line comment is typically preceeded by tab.
> goto bad;
> }
> }
> @@ -265,7 +263,7 @@ infiniband_output(struct ifnet *ifp, struct mbuf *m, c
> if (error) {
> if (error == EWOULDBLOCK)
> error = 0;
> - m = NULL; /* mbuf is consumed by resolver
> */
> + m = NULL; /* mbuf is consumed by resolver */
> goto bad;
> }
> }
> @@ -410,8 +408,8 @@ done:
> }
>
> static int
> -infiniband_resolvemulti(struct ifnet *ifp, struct sockaddr **llsa,
> -struct sockaddr *sa)
> +infiniband_resolvemulti(
> +struct ifnet *ifp, struct sockaddr **llsa, struct sockaddr *sa)
> {
> struct sockaddr_dl *sdl;
> #ifdef INET
> @@ -442,8 +440,8 @@ infiniband_resolvemulti(struct ifnet *ifp, struct sock
> sdl = link_init_sdl(ifp, *llsa, IFT_INFINIBAND);
> sdl->sdl_alen = INFINIBAND_ADDR_LEN;
> e_addr = LLADDR(sdl);
> - infiniband_ipv4_multicast_map(sin->sin_addr.s_addr,
> ifp->if_broadcastaddr,
> - e_addr);
> + infiniband_ipv4_multicast_map(
> + sin->sin_addr.s_addr, ifp->if_broadcastaddr, e_addr);
> *llsa = (struct sockaddr *)sdl;
> return (0);
> #endif
> @@ -462,7 +460,8 @@ infiniband_resolvemulti(struct ifnet *ifp, struct sock
> sdl = link_init_sdl(ifp, *llsa, IFT_INFINIBAND);
> sdl->sdl_alen = INFINIBAND_ADDR_LEN;
> e_addr = LLADDR(sdl);
> - infiniband_ipv6_multicast_map(&sin6->sin6_addr,
> ifp->if_broadcastaddr, e_addr);
> + infiniband_ipv6_multicast_map(
> +
