Re: [U-Boot] [PATCH v2 04/23] fs: fat: make directory iterator global for write use
On Wed, Sep 05, 2018 at 10:16:32AM +0200, Alexander Graf wrote: > > > On 05.09.18 04:14, AKASHI Takahiro wrote: > > On Tue, Sep 04, 2018 at 12:57:54PM +0200, Alexander Graf wrote: > >> > >> > >>> Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt : > >>> > >>> > >>> > On 09/04/2018 11:01 AM, Alexander Graf wrote: > > > > On 04.09.18 09:49, AKASHI Takahiro wrote: > > Directory iterator was introduced in major re-work of read operation by > > Rob. We want to use it for write operation extensively as well. > > Please indicate in the commit message that write operations are > implemented in a different .c file and so we have to export the > respective functions. > >>> > >>> Why? Look at this ugly code: > >>> > >>> fs/fat/fat_write.c:17:#include "fat.c" > >> > >> In that case we don't need this patch at all, no? > > > > Oops, I didn't notice this before. > > If, however, "include fat.c" makes any sense, theoretically we don't need > > "depends on FS_FAT" for FS_FAT_WRITE. > > There seems to be a contradiction between the code and config. > > I'm not sure it's contradictory. Someone just implemented a poor man's > LTO by including the 2 files with each other. Well, LTO has long existed since gcc4.8 ... (and improvement would be quite small, I guess.) > > I prefer just to remove the line, '#include "fat.c"' from fat_write.c > > and add more "extern" definitions in fat.h if necessary. > > We can worry about refactoring things later down the road. By making > functions go external, we lose the compiler's ability to inline > functions which may again bloat code which may trip random boards above > size limits. > > For now, I'd suggest you leave the ugly #include "fat.c" in and not move > anything into fat.h. That way the file is self-contained and you don't > have to worry about exporting just yet. Although I won't tolerate such an ugly code, I will follow your suggestion mainly because lots of existing defconfigs enable FAT_WRITE without FS_FAT. Thanks, -Takahiro AKASHI > > Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/23] fs: fat: make directory iterator global for write use
On 05.09.18 04:14, AKASHI Takahiro wrote: > On Tue, Sep 04, 2018 at 12:57:54PM +0200, Alexander Graf wrote: >> >> >>> Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt : >>> >>> >>> On 09/04/2018 11:01 AM, Alexander Graf wrote: > On 04.09.18 09:49, AKASHI Takahiro wrote: > Directory iterator was introduced in major re-work of read operation by > Rob. We want to use it for write operation extensively as well. Please indicate in the commit message that write operations are implemented in a different .c file and so we have to export the respective functions. >>> >>> Why? Look at this ugly code: >>> >>> fs/fat/fat_write.c:17:#include "fat.c" >> >> In that case we don't need this patch at all, no? > > Oops, I didn't notice this before. > If, however, "include fat.c" makes any sense, theoretically we don't need > "depends on FS_FAT" for FS_FAT_WRITE. > There seems to be a contradiction between the code and config. I'm not sure it's contradictory. Someone just implemented a poor man's LTO by including the 2 files with each other. > I prefer just to remove the line, '#include "fat.c"' from fat_write.c > and add more "extern" definitions in fat.h if necessary. We can worry about refactoring things later down the road. By making functions go external, we lose the compiler's ability to inline functions which may again bloat code which may trip random boards above size limits. For now, I'd suggest you leave the ugly #include "fat.c" in and not move anything into fat.h. That way the file is self-contained and you don't have to worry about exporting just yet. Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/23] fs: fat: make directory iterator global for write use
On Tue, Sep 04, 2018 at 12:57:54PM +0200, Alexander Graf wrote: > > > > Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt : > > > > > > > >> On 09/04/2018 11:01 AM, Alexander Graf wrote: > >> > >> > >>> On 04.09.18 09:49, AKASHI Takahiro wrote: > >>> Directory iterator was introduced in major re-work of read operation by > >>> Rob. We want to use it for write operation extensively as well. > >> > >> Please indicate in the commit message that write operations are > >> implemented in a different .c file and so we have to export the > >> respective functions. > > > > Why? Look at this ugly code: > > > > fs/fat/fat_write.c:17:#include "fat.c" > > In that case we don't need this patch at all, no? Oops, I didn't notice this before. If, however, "include fat.c" makes any sense, theoretically we don't need "depends on FS_FAT" for FS_FAT_WRITE. There seems to be a contradiction between the code and config. I prefer just to remove the line, '#include "fat.c"' from fat_write.c and add more "extern" definitions in fat.h if necessary. Thanks, -Takakahiro AKASHI > Alex > > > > > Best regards > > > > Heinrich > > > >> > >> Alex > >> ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/23] fs: fat: make directory iterator global for write use
> Am 04.09.2018 um 12:50 schrieb Heinrich Schuchardt : > > > >> On 09/04/2018 11:01 AM, Alexander Graf wrote: >> >> >>> On 04.09.18 09:49, AKASHI Takahiro wrote: >>> Directory iterator was introduced in major re-work of read operation by >>> Rob. We want to use it for write operation extensively as well. >> >> Please indicate in the commit message that write operations are >> implemented in a different .c file and so we have to export the >> respective functions. > > Why? Look at this ugly code: > > fs/fat/fat_write.c:17:#include "fat.c" In that case we don't need this patch at all, no? Alex > > Best regards > > Heinrich > >> >> Alex >> ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/23] fs: fat: make directory iterator global for write use
On 09/04/2018 11:01 AM, Alexander Graf wrote: > > > On 04.09.18 09:49, AKASHI Takahiro wrote: >> Directory iterator was introduced in major re-work of read operation by >> Rob. We want to use it for write operation extensively as well. > > Please indicate in the commit message that write operations are > implemented in a different .c file and so we have to export the > respective functions. Why? Look at this ugly code: fs/fat/fat_write.c:17:#include "fat.c" Best regards Heinrich > > Alex > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/23] fs: fat: make directory iterator global for write use
On 04.09.18 09:49, AKASHI Takahiro wrote: > Directory iterator was introduced in major re-work of read operation by > Rob. We want to use it for write operation extensively as well. Please indicate in the commit message that write operations are implemented in a different .c file and so we have to export the respective functions. Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
