Hi, Thanks for the tip! I've created an issue and a PR, now waiting for a review.
Regards, Saurav On Sat, Feb 3, 2024 at 1:57 AM Alan C. Assis <acas...@gmail.com> wrote: > Hi Saurav, > > You can follow the steps here: > https://nuttx.apache.org/docs/latest/contributing/index.html > > After running the checkpatch.sh and confirming everything is fine, you can > create a commit. > > Basically your commit should be like this: > > > ------------------------------------------------------------------------------ > fs/vfat: Fix typo in the macro DIRSEC_BYTENDX > > Some comments explaining what the commit is supposed to do! > > Signed-off-by: Saurav Pal <resyfer....@gmail.com> > > ------------------------------------------------------------------------------ > > Best Regards, > > Alan > > On Fri, Feb 2, 2024 at 5:14 PM Saurav Pal <resyfer....@gmail.com> wrote: > > > Hi Alan, > > > > Yeah I'll add an issue in the repo, and put a PR for fixing this. A quick > > question...is there any standard on commit names, or should I just use my > > own like "FIX: VFAT macro Issue#123"? > > > > About why side effects have not been seen...like Lwazi explained, > > macro DIRSEC_BYTENDX(f, > > i) is always used as DIRSEC_BYTENDX(fs, inode), and so, in its expansion, > > DIRSEC_NDXMASK(f) should become DIRSEC_NDXMASK(fs), and that weirdly > > coincides with what the typo is, and so, it weirdly works how it was > > intended to and does not give any errors. > > > > Since it is a macro, even language servers (as far as the ones I know) do > > not visually show their "unused parameters" warning like in the case > proper > > functions. > > > > Regards, > > Saurav > > > > On Sat, Feb 3, 2024 at 1:11 AM Alan C. Assis <acas...@gmail.com> wrote: > > > > > Hi Saurav, > > > > > > I think you found a BUG! > > > > > > Please report it at https://github.com/apache/nuttx/issues to keep a > > track > > > of it and when you we submit a PR it could be closed automatically > (since > > > you link it at your PR). > > > > > > In fact looking both macros the f to fs mistake becomes clear: > > > > > > #define DIRSEC_NDXMASK(f) (((f)->fs_hwsectorsize - 1) >> 5) > > > > > > #define DIRSEC_BYTENDX(f,i) (((i) & DIRSEC_NDXMASK(fs)) << 5) > > > > > > I'm curious to know what this mistake could cause to VFAT on NuttX? > > > > > > Why haven't we seen any side effects in products using NuttX with VFAT? > > > > > > Best Regards, > > > > > > Alan > > > > > > On Fri, Feb 2, 2024 at 1:55 PM Saurav Pal <resyfer....@gmail.com> > wrote: > > > > > > > Hi Alan, > > > > > > > > Thank you for looking at our code base and planning to add > > Documentation, > > > > > that is really important! > > > > > > > > > > NuttX has a long history but our Documentation is still lagging > > behind, > > > > so > > > > > your work will be very beneficial for our community. > > > > > > > > > > > > > I'll try my best to contribute some documentation for the various > file > > > > systems I look into, and I, in turn, hope they are informative, and > > more > > > > importantly, correct, as I'm pretty new to the VFS layer of NuttX. > > > > > > > > > > > > > BTW, why do you think it is wrong, could you please share your > > > thoughts? > > > > > > > > > > > > > The problem according to me is on L264...where it's defined as: > > > > > > > > #define DIRSEC_BYTENDX(*f*,i) (((i) & DIRSEC_NDXMASK(*fs*)) << 5) > > > > > > > > I can't seem to find anything defined as *fs *in the namespace at > that > > > > point (used in the fat_findsfnentry > > > > < > > > > > > https://github.com/apache/nuttx/blob/master/fs/fat/fs_fat32dirent.c#L1137 > > > > >function), > > > > and I was wondering if it was a simple typo or a mistake on my part > as > > > it's > > > > code that's quite old, which usually don't have such mistakes due to > > > > extensive testing. > > > > > > > > Best Regards, > > > > Saurav > > > > > > > > > >
