| Message ID | 1355395656-5838-2-git-send-email-richard.genoud@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Richard Genoud, > ifdefs in the code are making it harder to read. > The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. > (the code is discarded by the compiler and linker instead of the > preprocessor.) > > and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not > defined. > > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > --- > fs/fat/fat.c | 55 > +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | > 11 ++------- > 2 files changed, 32 insertions(+), 34 deletions(-) > > diff --git a/fs/fat/fat.c b/fs/fat/fat.c > index 393c378..c79e3e3 100644 > --- a/fs/fat/fat.c > +++ b/fs/fat/fat.c > @@ -34,6 +34,12 @@ > #include <malloc.h> > #include <linux/compiler.h> > > +#ifdef CONFIG_SUPPORT_VFAT > +#define VFAT_ENABLED 1 > +#else > +#define VFAT_ENABLED 0 > +#endif [...] Make it static const int maybe ? Best regards, Marek Vasut
2012/12/13 Marek Vasut <marex@denx.de>: > Dear Richard Genoud, > >> ifdefs in the code are making it harder to read. >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. >> (the code is discarded by the compiler and linker instead of the >> preprocessor.) >> >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not >> defined. >> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> >> --- >> fs/fat/fat.c | 55 >> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | >> 11 ++------- >> 2 files changed, 32 insertions(+), 34 deletions(-) >> >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c >> index 393c378..c79e3e3 100644 >> --- a/fs/fat/fat.c >> +++ b/fs/fat/fat.c >> @@ -34,6 +34,12 @@ >> #include <malloc.h> >> #include <linux/compiler.h> >> >> +#ifdef CONFIG_SUPPORT_VFAT >> +#define VFAT_ENABLED 1 >> +#else >> +#define VFAT_ENABLED 0 >> +#endif > [...] > > Make it static const int maybe ? I hesitate to make them static const, I can't figure why it's better to use static const variable instead of defines. Could you enlighten me ? Best regards, Richard.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 12/13/12 05:47, Richard Genoud wrote: > ifdefs in the code are making it harder to read. The use of simple > if(VFAT_ENABLED) makes no more code and is cleaner. (the code is > discarded by the compiler and linker instead of the preprocessor.) > > and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is > not defined. > > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- > fs/fat/fat.c | 55 > +++++++++++++++++++++++++++------------------------ > fs/fat/fat_write.c | 11 ++------- 2 files changed, 32 > insertions(+), 34 deletions(-) > > diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 > 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ > #include <malloc.h> #include <linux/compiler.h> > > +#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define > VFAT_ENABLED 0 +#endif + /* * Convert a string to lowercase. */ @@ > -441,7 +447,6 @@ getit: } while (1); } > > -#ifdef CONFIG_SUPPORT_VFAT /* * Extract the file name information > from 'slotptr' into 'l_name', * starting at l_name[*idx]. @@ -576,7 > +581,6 @@ static __u8 mkcksum(const char name[8], const char > ext[3]) > > return ret; } -#endif /* CONFIG_SUPPORT_VFAT */ > > /* * Get the directory entry associated with 'filename' from the > directory @@ -617,8 +621,8 @@ static dir_entry > *get_dentfromdir(fsdata *mydata, int startsect, continue; } if > ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - > if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if > (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT > && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = > ((dir_slot *)dentptr)->alias_checksum; get_vfatname(mydata, > curclust, @@ -658,9 +662,7 @@ static dir_entry > *get_dentfromdir(fsdata *mydata, int startsect, continue; } > debug("vfatname: |%s|\n", l_name); - } else -#endif - { + > } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ > -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata > *mydata, int startsect, debug("Dentname == NULL - %d\n", i); return > NULL; } -#ifdef CONFIG_SUPPORT_VFAT - __u8 csum = > mkcksum(dentptr->name, dentptr->ext); - if (dols && csum == > prevcksum) { - prevcksum = 0xffff; - dentptr++; - > continue; + if (VFAT_ENABLED) { + __u8 csum = > mkcksum(dentptr->name, dentptr->ext); + if (dols && csum == > prevcksum) { + prevcksum = 0xffff; + dentptr++; + > continue; + } } -#endif + get_name(dentptr, s_name); if (dols) > { int isdir = (dentptr->attr & ATTR_DIR); @@ -884,9 +887,9 @@ > do_fat_read_at(const char *filename, unsigned long pos, void > *buffer, return -1; } > > -#ifdef CONFIG_SUPPORT_VFAT - debug("VFAT Support enabled\n"); > -#endif + if (VFAT_ENABLED) + debug("VFAT Support enabled\n"); + > debug("FAT%d, fat_sect: %d, fatlength: %d\n", mydata->fatsize, > mydata->fat_sect, mydata->fatlength); debug("Rootdir begins at > cluster: %d, sector: %d, offset: %x\n" @@ -952,10 +955,12 @@ > do_fat_read_at(const char *filename, unsigned long pos, void > *buffer, continue; } > > - csum = mkcksum(dentptr->name, dentptr->ext); + if > (VFAT_ENABLED) + csum = mkcksum(dentptr->name, dentptr->ext); + > if (dentptr->attr & ATTR_VOLUME) { -#ifdef CONFIG_SUPPORT_VFAT - > if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if > (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT > && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = > ((dir_slot *)dentptr)->alias_checksum; @@ -999,9 +1004,7 @@ > do_fat_read_at(const char *filename, unsigned long pos, void > *buffer, } debug("Rootvfatname: |%s|\n", l_name); - } else > -#endif - { + } else { /* Volume label or VFAT entry */ > dentptr++; continue; @@ -1015,13 +1018,13 @@ do_fat_read_at(const > char *filename, unsigned long pos, void *buffer, } goto exit; } > -#ifdef CONFIG_SUPPORT_VFAT - else if (dols == LS_ROOT && csum == > prevcksum) { + else if (VFAT_ENABLED && + dols == LS_ROOT && > csum == prevcksum) { prevcksum = 0xffff; dentptr++; continue; } > -#endif + get_name(dentptr, s_name); > > if (dols == LS_ROOT) { diff --git a/fs/fat/fat_write.c > b/fs/fat/fat_write.c index 4a1bda0..ed3eaa0 100644 --- > a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -248,7 +248,6 @@ > static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return > ret; } > > -#ifdef CONFIG_SUPPORT_VFAT /* * Set the file name information from > 'name' into 'slotptr', */ @@ -468,8 +467,6 @@ > get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster, > return 0; } > > -#endif Note that we don't use --gc-sections on all archs so I'm not sure we discard the unused VFAT functions on say ARM. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQydOVAAoJENk4IS6UOR1WDYQP/2P6ywBeeE/qnx9C13qJnCcr wXG1IJWEpjgAEOViQZ77kE5Knn9ltiuXKj6EYc7e3oY4S2mBREkG/sHNmG+e+9/0 WHuZJoaiLPaia9ztL/d8BCyVPkSbJ88iyA20TgefR94j7SqITiVT+3yDA4XEB3bW C8rXZdmYzGjLzMBPI458uxNmIKSeHVfUZnDdWdA0rIOK8t6QN+OKLJRMezOYwx8e jM8Td4hDmJ20EGfFTpcO2spdUMCqi6FfO6bXJHeFHeUqGXqz482xyIxiZGpETut9 tFOVegHIpbKf0eBoMvs+3ajHprAsMGFlAqv0YSxh4w04ZIXUBT+DibjGiQtf/Xjl HDvAkBGEbQjljZaQDYlaAfNWFctU7lkapujiMuOKjv04DvClp0AgKEvOjAUW/P2N ERIQzc67dUdomhmdl0sl+yFYJDB4zjT1VnlYXGx/yz4KLIiD5t+gaQAyq3pGwYjH jZ3q5gAprYNR0VKl6QM8gQFdAjUKT0owJqUb0iZ2YqblIr+IHLIFHddhwOHB76cg ViLKtbJ7Y43aShdQUE2t8Ibth+ywba3EDBaieqkz4Q928k//kScGSal6nxqE4mhR AZ4ca5euEpADicacHZmKNSbtgeBYvhsC/c4ly27hdLPQjpAGOT/axPoaIfUxIe9H PKIJx4VC14A2u4tS1A6Q =1PBB -----END PGP SIGNATURE-----
Dear Richard Genoud, > 2012/12/13 Marek Vasut <marex@denx.de>: > > Dear Richard Genoud, > > > >> ifdefs in the code are making it harder to read. > >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. > >> (the code is discarded by the compiler and linker instead of the > >> preprocessor.) > >> > >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not > >> defined. > >> > >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > >> --- > >> > >> fs/fat/fat.c | 55 > >> > >> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | > >> 11 ++------- > >> > >> 2 files changed, 32 insertions(+), 34 deletions(-) > >> > >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c > >> index 393c378..c79e3e3 100644 > >> --- a/fs/fat/fat.c > >> +++ b/fs/fat/fat.c > >> @@ -34,6 +34,12 @@ > >> > >> #include <malloc.h> > >> #include <linux/compiler.h> > >> > >> +#ifdef CONFIG_SUPPORT_VFAT > >> +#define VFAT_ENABLED 1 > >> +#else > >> +#define VFAT_ENABLED 0 > >> +#endif > > > > [...] > > > > Make it static const int maybe ? > > I hesitate to make them static const, I can't figure why it's better > to use static const variable instead of defines. > Could you enlighten me ? Because you get the type-checking. Preprocessor is evil ;-) Best regards, Marek Vasut
2012/12/13 Tom Rini <trini@ti.com>: > Note that we don't use --gc-sections on all archs so I'm not sure we > discard the unused VFAT functions on say ARM. > I tested it on at91sam9x5ek, and the vfat functions are not present in the System.map. The problem is in my commit message: the linker doesn"t have anything to do with that because all discarded functions are static. So I think it's ok on all archs (but maybe not with -O0) Richard
2012/12/13 Marek Vasut <marex@denx.de>: > Dear Richard Genoud, > >> 2012/12/13 Marek Vasut <marex@denx.de>: >> > Dear Richard Genoud, >> > >> >> ifdefs in the code are making it harder to read. >> >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. >> >> (the code is discarded by the compiler and linker instead of the >> >> preprocessor.) >> >> >> >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not >> >> defined. >> >> >> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> >> >> --- >> >> >> >> fs/fat/fat.c | 55 >> >> >> >> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | >> >> 11 ++------- >> >> >> >> 2 files changed, 32 insertions(+), 34 deletions(-) >> >> >> >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c >> >> index 393c378..c79e3e3 100644 >> >> --- a/fs/fat/fat.c >> >> +++ b/fs/fat/fat.c >> >> @@ -34,6 +34,12 @@ >> >> >> >> #include <malloc.h> >> >> #include <linux/compiler.h> >> >> >> >> +#ifdef CONFIG_SUPPORT_VFAT >> >> +#define VFAT_ENABLED 1 >> >> +#else >> >> +#define VFAT_ENABLED 0 >> >> +#endif >> > >> > [...] >> > >> > Make it static const int maybe ? >> >> I hesitate to make them static const, I can't figure why it's better >> to use static const variable instead of defines. >> Could you enlighten me ? > > Because you get the type-checking. Preprocessor is evil ;-) > > Best regards, > Marek Vasut Ok ! Thanks, I'll resend it with the change. Richard.
Dear Richard Genoud, > 2012/12/13 Marek Vasut <marex@denx.de>: > > Dear Richard Genoud, > > > >> 2012/12/13 Marek Vasut <marex@denx.de>: > >> > Dear Richard Genoud, > >> > > >> >> ifdefs in the code are making it harder to read. > >> >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. > >> >> (the code is discarded by the compiler and linker instead of the > >> >> preprocessor.) > >> >> > >> >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not > >> >> defined. > >> >> > >> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > >> >> --- > >> >> > >> >> fs/fat/fat.c | 55 > >> >> > >> >> +++++++++++++++++++++++++++------------------------ > >> >> fs/fat/fat_write.c | 11 ++------- > >> >> > >> >> 2 files changed, 32 insertions(+), 34 deletions(-) > >> >> > >> >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c > >> >> index 393c378..c79e3e3 100644 > >> >> --- a/fs/fat/fat.c > >> >> +++ b/fs/fat/fat.c > >> >> @@ -34,6 +34,12 @@ > >> >> > >> >> #include <malloc.h> > >> >> #include <linux/compiler.h> > >> >> > >> >> +#ifdef CONFIG_SUPPORT_VFAT > >> >> +#define VFAT_ENABLED 1 > >> >> +#else > >> >> +#define VFAT_ENABLED 0 > >> >> +#endif > >> > > >> > [...] > >> > > >> > Make it static const int maybe ? > >> > >> I hesitate to make them static const, I can't figure why it's better > >> to use static const variable instead of defines. > >> Could you enlighten me ? > > > > Because you get the type-checking. Preprocessor is evil ;-) > > > > Best regards, > > Marek Vasut > > Ok ! Thanks, I'll resend it with the change. Thanks! Yes, preprocessor eats kittens too ;-) Best regards, Marek Vasut
Dear Tom Rini, [...] > Note that we don't use --gc-sections on all archs so I'm not sure we > discard the unused VFAT functions on say ARM. Valid point, Albert? Best regards, Marek Vasut
2012/12/13 Marek Vasut <marex@denx.de>: > Dear Tom Rini, > > [...] > >> Note that we don't use --gc-sections on all archs so I'm not sure we >> discard the unused VFAT functions on say ARM. > > Valid point, Albert? > > Best regards, > Marek Vasut I check and the code is discarded (on ARM at91sam9x5ek). This is the case because the function to discard are all in the same file (fat.c or fat_write.c). The compiler will discard unused function with -Os. (but not with -O0)
On Thu, Dec 13, 2012 at 03:24:08PM +0100, Richard Genoud wrote: > 2012/12/13 Marek Vasut <marex@denx.de>: > > Dear Tom Rini, > > > > [...] > > > >> Note that we don't use --gc-sections on all archs so I'm not sure we > >> discard the unused VFAT functions on say ARM. > > > > Valid point, Albert? > > > > Best regards, > > Marek Vasut > > I check and the code is discarded (on ARM at91sam9x5ek). > This is the case because the function to discard are all in the same > file (fat.c or fat_write.c). > The compiler will discard unused function with -Os. (but not with -O0) Thanks for checking.
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ #include <malloc.h> #include <linux/compiler.h> +#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif + /* * Convert a string to lowercase. */ @@ -441,7 +447,6 @@ getit: } while (1); } -#ifdef CONFIG_SUPPORT_VFAT /* * Extract the file name information from 'slotptr' into 'l_name', * starting at l_name[*idx]. @@ -576,7 +581,6 @@ static __u8 mkcksum(const char name[8], const char ext[3]) return ret; } -#endif /* CONFIG_SUPPORT_VFAT */ /* * Get the directory entry associated with 'filename' from the directory @@ -617,8 +621,8 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; get_vfatname(mydata, curclust, @@ -658,9 +662,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, debug("Dentname == NULL - %d\n", i); return NULL; } -#ifdef CONFIG_SUPPORT_VFAT - __u8 csum = mkcksum(dentptr->name, dentptr->ext); - if (dols && csum == prevcksum) { - prevcksum = 0xffff; - dentptr++; - continue; + if (VFAT_ENABLED) { + __u8 csum = mkcksum(dentptr->name, dentptr->ext); + if (dols && csum == prevcksum) { + prevcksum = 0xffff; + dentptr++; + continue; + } } -#endif + get_name(dentptr, s_name); if (dols) { int isdir = (dentptr->attr & ATTR_DIR); @@ -884,9 +887,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, return -1; } -#ifdef CONFIG_SUPPORT_VFAT - debug("VFAT Support enabled\n"); -#endif + if (VFAT_ENABLED) + debug("VFAT Support enabled\n"); + debug("FAT%d, fat_sect: %d, fatlength: %d\n", mydata->fatsize, mydata->fat_sect, mydata->fatlength); debug("Rootdir begins at cluster: %d, sector: %d, offset: %x\n" @@ -952,10 +955,12 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, continue; } - csum = mkcksum(dentptr->name, dentptr->ext); + if (VFAT_ENABLED) + csum = mkcksum(dentptr->name, dentptr->ext); + if (dentptr->attr & ATTR_VOLUME) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; @@ -999,9 +1004,7 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } debug("Rootvfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -1015,13 +1018,13 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } goto exit; } -#ifdef CONFIG_SUPPORT_VFAT - else if (dols == LS_ROOT && csum == prevcksum) { + else if (VFAT_ENABLED && + dols == LS_ROOT && csum == prevcksum) { prevcksum = 0xffff; dentptr++; continue; } -#endif + get_name(dentptr, s_name); if (dols == LS_ROOT) { diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 4a1bda0..ed3eaa0 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -248,7 +248,6 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return ret; } -#ifdef CONFIG_SUPPORT_VFAT /* * Set the file name information from 'name' into 'slotptr', */ @@ -468,8 +467,6 @@ get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster, return 0; } -#endif - /* * Set the entry at index 'entry' in a FAT (16/32) table. */ @@ -853,16 +850,14 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { get_long_file_name(mydata, curclust, get_dentfromdir_block, &dentptr, l_name); debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; if (is_next_clust(mydata, dentptr))
ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.) and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined. Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- fs/fat/fat.c | 55 +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++------- 2 files changed, 32 insertions(+), 34 deletions(-)