fat16_new_fat_item_for_directory_item

[mark-marinas]

On lines 559-566 of fat16.c:

if (item->attribute & FAT_FILE_SUBDIRECTORY)

{

    f_item->directory = fat16_load_fat_directory(disk, item);

    f_item->type = FAT_ITEM_TYPE_DIRECTORY;

}

f_item->type = FAT_ITEM_TYPE_FILE;

f_item->item = fat16_clone_directory_item(item, sizeof(struct fat_directory_item));

should this be:

if (item->attribute & FAT_FILE_SUBDIRECTORY)

{

    f_item->directory = fat16_load_fat_directory(disk, item);

    f_item->type = FAT_ITEM_TYPE_DIRECTORY;

} **else {**

f_item->type = FAT_ITEM_TYPE_FILE;

f_item->item = fat16_clone_directory_item(item, sizeof(struct fat_directory_item));

}

but an item is only either a file or a directory. On the original code, the last 2 lines in the snippet always gets executed, so everything is a file.

[nibblebits]

Sorry mark I mistakingly gave you the wrong repository, this is the correct one: https://github.com/nibblebits/PeachCompiler

[nibblebits]

Wait it is the correct repository, thanks for posting this I will look into it and get back to you

[mark-marinas]

Hi Dan,

I was referring to the peach-os, not the compiler (though I plan to take that up after I finish the peach-os course). So the repository should be https://github.com/nibblebits/PeachOS.

Thanks.

On Fri, Aug 4, 2023 at 4:13 PM Daniel McCarthy @.***> wrote:

Sorry mark I mistakingly gave you the wrong repository, this is the correct one: https://github.com/nibblebits/PeachCompiler

— Reply to this email directly, view it on GitHub https://github.com/nibblebits/PeachOS/issues/23#issuecomment-1665199285, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADS3L2NVOKDDCLBNJBVYJHDXTSVL3ANCNFSM6AAAAAA3D3EUEM . You are receiving this because you authored the thread.Message ID: @.***>

[nibblebits]

Okay I have taken a look the original code is correct because if we look at the final code in the last change to this function you will see

struct fat_item *fat16_new_fat_item_for_directory_item(struct disk *disk, struct fat_directory_item *item)
{
    struct fat_item *f_item = kzalloc(sizeof(struct fat_item));
    if (!f_item)
    {
        return 0;
    }

    if (item->attribute & FAT_FILE_SUBDIRECTORY)
    {
        f_item->directory = fat16_load_fat_directory(disk, item);
        f_item->type = FAT_ITEM_TYPE_DIRECTORY;
        // HERE WE RETURN 
        return f_item;
    }
    // THIS IS NOT REACHED IN THE CASE OF A SUBDIRECTORY
    f_item->type = FAT_ITEM_TYPE_FILE;
    f_item->item = fat16_clone_directory_item(item, sizeof(struct fat_directory_item));
    return f_item;
}

This is a coding style I use because I consider "else" to be ugly so I tend to avoid it unless I have a reason to use it. You will see me using "else if" but if I get too many of them switch is more appropiate. However when it comes to "else" I very rarely use it because it can lead to code becoming pretty ugly when if statements nest. Therefore its best to return if you can. Though this is a preference and if you wish to use else statement your more than welcome.

Thanks for reporting but their is no bug here, please feel free to always post issue reports as I am not always correct and some bug reports are legitimate.

Thanks Dan

[mark-marinas]

ah, i was looking at a different commit (3ee00c3) - the one that is linked to the course lecture (there is no early return on that one). I'll refer to your latest commit from now on.

Thanks for your quick reply. I am so far enjoying your course.

On Fri, Aug 4, 2023 at 4:22 PM Daniel McCarthy @.***> wrote:

Okay I have taken a look the original code is correct because if we look at the final code in the last change to this function you will see

struct fat_item fat16_new_fat_item_for_directory_item(struct disk disk, struct fat_directory_item item) { struct fat_item f_item = kzalloc(sizeof(struct fat_item)); if (!f_item) { return 0; }

if (item->attribute & FAT_FILE_SUBDIRECTORY)
{
    f_item->directory = fat16_load_fat_directory(disk, item);
    f_item->type = FAT_ITEM_TYPE_DIRECTORY;
    // HERE WE RETURN
    return f_item;
}
// THIS IS NOT REACHED IN THE CASE OF A SUBDIRECTORY
f_item->type = FAT_ITEM_TYPE_FILE;
f_item->item = fat16_clone_directory_item(item, sizeof(struct fat_directory_item));
return f_item;

}

This is a coding style I use because I consider "else" to be ugly so I tend to avoid it unless I have a reason to use it. You will see me using "else if" but if I get too many of them switch is more appropiate. However when it comes to "else" I very rarely use it because it can lead to code becoming pretty ugly when if statements nest. Therefore its best to return if you can. Though this is a preference and if you wish to use else statement your more than welcome.

Thanks for reporting but their is no bug here, please feel free to always post issue reports as I am not always correct and some bug reports are legitimate.

Thanks Dan

— Reply to this email directly, view it on GitHub https://github.com/nibblebits/PeachOS/issues/23#issuecomment-1665209520, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADS3L2NA2AHNSRNRTF4K72TXTSWLNANCNFSM6AAAAAA3D3EUEM . You are receiving this because you authored the thread.Message ID: @.***>

[nibblebits]

Your welcome and I am glad your enjoying the course