| Message ID | 1439691396-28809-3-git-send-email-marcel@ziswiler.com |
|---|---|
| State | Accepted |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Marcel, On 15 August 2015 at 20:16, Marcel Ziswiler <marcel@ziswiler.com> wrote: > Cleaning up order of include files by sorting them alphabetically > keeping in mind to leave common.h on top. > > Signed-off-by: Marcel Ziswiler <marcel@ziswiler.com> > --- > > drivers/mmc/tegra_mmc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c > index 6f8b4d0..078df39 100644 > --- a/drivers/mmc/tegra_mmc.c > +++ b/drivers/mmc/tegra_mmc.c > @@ -7,14 +7,14 @@ > * SPDX-License-Identifier: GPL-2.0+ > */ > > -#include <bouncebuf.h> > #include <common.h> > -#include <asm/gpio.h> > -#include <asm/io.h> > #include <asm/arch/clock.h> > #include <asm/arch-tegra/clk_rst.h> > #include <asm/arch-tegra/mmc.h> > #include <asm/arch-tegra/tegra_mmc.h> > +#include <asm/gpio.h> > +#include <asm/io.h> > +#include <bouncebuf.h> > #include <mmc.h> > > DECLARE_GLOBAL_DATA_PTR; > -- > 2.4.3 > Thanks for tidying this up. But the ordering should be: <common.h> <others.h> <asm/...> <arch/arm/...> <linux/...> "local.h" Regards, Simon
On Sun, 2015-08-16 at 15:11 -0600, Simon Glass wrote: > Thanks for tidying this up. But the ordering should be: > > <common.h> > <others.h> > <asm/...> > <arch/arm/...> > <linux/...> > "local.h" Says who? I was only aware that common.h needs to go on top, the local stuff (e.g. in double quotes) on the bottom and the rest I assumed can go alphabetically ordered in between, not?
Hi Marcel, On 16 August 2015 at 15:19, Marcel Ziswiler <marcel@ziswiler.com> wrote: > On Sun, 2015-08-16 at 15:11 -0600, Simon Glass wrote: > >> Thanks for tidying this up. But the ordering should be: >> >> <common.h> >> <others.h> >> <asm/...> >> <arch/arm/...> >> <linux/...> >> "local.h" > > Says who? I was only aware that common.h needs to go on top, the local > stuff (e.g. in double quotes) on the bottom and the rest I assumed can > go alphabetically ordered in between, not? I originally got it from here (and another thread I can't find): http://lists.denx.de/pipermail/u-boot/2012-January/114965.html Regards, Simon
On Sun, 2015-08-16 at 15:26 -0600, Simon Glass wrote: > > Says who? I was only aware that common.h needs to go on top, the > > local > > stuff (e.g. in double quotes) on the bottom and the rest I assumed > > can > > go alphabetically ordered in between, not? > > I originally got it from here (and another thread I can't find): > > http://lists.denx.de/pipermail/u-boot/2012-January/114965.html Hm, interesting. But that one suggests a different order yet again. Unfortunately it misses giving any reason for any of this. Me seriously wondering now. BTW: Marek was happy with my order of things for the PXA stuff: http://git.denx.de/?p=u-boot/u-boot-pxa.git;a=commitdiff;h=940ed38dd673 ecfbe03ed5e36c25bc4238356947
Hi Marcel, On 16 August 2015 at 15:34, Marcel Ziswiler <marcel@ziswiler.com> wrote: > On Sun, 2015-08-16 at 15:26 -0600, Simon Glass wrote: > >> > Says who? I was only aware that common.h needs to go on top, the >> > local >> > stuff (e.g. in double quotes) on the bottom and the rest I assumed >> > can >> > go alphabetically ordered in between, not? >> >> I originally got it from here (and another thread I can't find): >> >> http://lists.denx.de/pipermail/u-boot/2012-January/114965.html > > Hm, interesting. But that one suggests a different order yet again. > Unfortunately it misses giving any reason for any of this. Me seriously > wondering now. > > BTW: Marek was happy with my order of things for the PXA stuff: > > http://git.denx.de/?p=u-boot/u-boot-pxa.git;a=commitdiff;h=940ed38dd673 > ecfbe03ed5e36c25bc4238356947 The nice thing about sorting things in groups is that you can see at a glance what is missing. It doesn't make sense to have: <asm/aaa.h> <bbb.c> <asm/ccc.h> since the asm/ includes are quite a different category. Regards, Simon
On Mon, 2015-08-17 at 16:14 -0600, Simon Glass wrote: > The nice thing about sorting things in groups is that you can see at > a > glance what is missing. It doesn't make sense to have: > > <asm/aaa.h> > <bbb.c> > <asm/ccc.h> > > since the asm/ includes are quite a different category. But that's not at all what I proposed. What I was talking about looks as follows: <common.h> <asm/aaa.h> <asm/ccc.h> <bbb.c> <linux/aaa.h> "local.h" Which is exactly what I actually did in the patch we are talking about. OK, I agree that there seems to be one nitty-gritty detail concerning the sort order of - aka dash vs. / aka slash going back to GNU sort (which is what I used) not seeming to enforce the same but I actually see that as a minor detail.
On Mon, 2015-08-17 at 20:55 +0000, Tom Warren wrote: > I have no problem with this sort order. Let me know if you want me to > take this in to Tegra or if it should go into u-boot-mmc tree via > Panto. I would be OK either way but as Panto has not responded I would be more than grateful if you can pull it in. Thanks, Tom.
Hi Marcel, On 18 August 2015 at 02:11, Marcel Ziswiler <marcel@ziswiler.com> wrote: > On Mon, 2015-08-17 at 16:14 -0600, Simon Glass wrote: > >> The nice thing about sorting things in groups is that you can see at >> a >> glance what is missing. It doesn't make sense to have: >> >> <asm/aaa.h> >> <bbb.c> >> <asm/ccc.h> >> >> since the asm/ includes are quite a different category. > > But that's not at all what I proposed. What I was talking about looks > as follows: > > <common.h> > <asm/aaa.h> > <asm/ccc.h> > <bbb.c> > <linux/aaa.h> > "local.h" > > Which is exactly what I actually did in the patch we are talking about. > > OK, I agree that there seems to be one nitty-gritty detail concerning > the sort order of - aka dash vs. / aka slash going back to GNU sort > (which is what I used) not seeming to enforce the same but I actually > see that as a minor detail. No I think you misunderstand. Another way of explaining this is sorting fruit and animals: apple grapefruit orange elephant lion zebra We don't want to mix up the fruit and animals, so each has its own position in the table.We can then easily see what animals are in the table. This is not a case of running 'sort' on the includes. The 'asm' files are arch-specific includes and should go after all the 'generic' includes, like <bbb.c>, etc. So to repeat, the ordering should be: <common.h> <- most general <others.h> <asm/...> <arch/arm/...> <linux/...> "local.h" <- least general Regards, Simon
On 18 August 2015 14:44:01 CEST, Simon Glass <sjg@chromium.org> wrote: >No I think you misunderstand. Another way of explaining this is >sorting fruit and animals: > >apple >grapefruit >orange >elephant >lion >zebra > >We don't want to mix up the fruit and animals, so each has its own >position in the table.We can then easily see what animals are in the >table. I see but I still don't think your example matches our problem at hand quite that accurately. >This is not a case of running 'sort' on the includes. The 'asm' files >are arch-specific includes and should go after all the 'generic' >includes, like <bbb.c>, etc. > >So to repeat, the ordering should be: > ><common.h> <- most general ><others.h> ><asm/...> ><arch/arm/...> ><linux/...> >"local.h" <- least general Your proposal sounds quite honourable but the problem I see with it is that your order is based on some generality rule I don't quite see where it could be looked up. At the end I believe the order actually does not even matter apart from U-Boot's special common.h having to go first. And even that could be worked around by the build system itself (see e.g. Linux kernel for that matter). So for me such ordering is just cosmetic and might at best help us humble programmers when comparing stuff or looking through to make sure a certain include is indeed already there or the like. The only practical way I see to achieve this would be to plain simply alphabetically sort them. As I don't think this is much worth discussing any further I'm fine with dropping this patch even though I actually think the actual file in question is seriously buggy as it does not put common.h first.
Hi Marcel, On 19 August 2015 at 10:00, Marcel Ziswiler <marcel@ziswiler.com> wrote: > > > > On 18 August 2015 14:44:01 CEST, Simon Glass <sjg@chromium.org> wrote: > > >No I think you misunderstand. Another way of explaining this is > >sorting fruit and animals: > > > >apple > >grapefruit > >orange > >elephant > >lion > >zebra > > > >We don't want to mix up the fruit and animals, so each has its own > >position in the table.We can then easily see what animals are in the > >table. > > I see but I still don't think your example matches our problem at hand quite that accurately. > > >This is not a case of running 'sort' on the includes. The 'asm' files > >are arch-specific includes and should go after all the 'generic' > >includes, like <bbb.c>, etc. > > > >So to repeat, the ordering should be: > > > ><common.h> <- most general > ><others.h> > ><asm/...> > ><arch/arm/...> > ><linux/...> > >"local.h" <- least general > > Your proposal sounds quite honourable but the problem I see with it is that your order is based on some generality rule I don't quite see where it could be looked up. > > At the end I believe the order actually does not even matter apart from U-Boot's special common.h having to go first. And even that could be worked around by the build system itself (see e.g. Linux kernel for that matter). > > So for me such ordering is just cosmetic and might at best help us humble programmers when comparing stuff or looking through to make sure a certain include is indeed already there or the like. > > The only practical way I see to achieve this would be to plain simply alphabetically sort them. > > As I don't think this is much worth discussing any further I'm fine with dropping this patch even though I actually think the actual file in question is seriously buggy as it does not put common.h first. As you say the compiler doesn't care about the order apart from common.h. But the code is written as much for us software engineers as the compiler. Linux puts the asm includes at the end and we more-or-less follow this in U-Boot. I don't want to be annoying and this is Tegra code so I will leave it to you and Tom to sort out. Regards, Simon
diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index 6f8b4d0..078df39 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -7,14 +7,14 @@ * SPDX-License-Identifier: GPL-2.0+ */ -#include <bouncebuf.h> #include <common.h> -#include <asm/gpio.h> -#include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch-tegra/clk_rst.h> #include <asm/arch-tegra/mmc.h> #include <asm/arch-tegra/tegra_mmc.h> +#include <asm/gpio.h> +#include <asm/io.h> +#include <bouncebuf.h> #include <mmc.h> DECLARE_GLOBAL_DATA_PTR;
Cleaning up order of include files by sorting them alphabetically keeping in mind to leave common.h on top. Signed-off-by: Marcel Ziswiler <marcel@ziswiler.com> --- drivers/mmc/tegra_mmc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)