-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
not enough stack, let's use heap instead(sdBrowserMenu) #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Дякую за PR! Декілька проблем:
- Змінна
File file
в sdcard.cpp:116 ініціалізується повторно всередині do {}. Це призводить до шедовінгу зовнішньої змінної, яка залишається неініціалізованою. Тому цикл виконується лише 1 раз. - Пам'ять виділяється лише на _numEntries файлів, але їх потрібно _numEntries + 1 (ще є пункт "<< Назад")
- Замість new, можна ініціалізувати entries/filenames/etc в стеку:
Це спростить менеджмент пам'яті, і не потрібно буде слідкувати за всіма return-ами, щоб там зробити delete.
lilka::Entry entries[_numEntries];
- Потрібно переформатувати код через clang-format (див. зафейлені перевірки)
- Я особисто б уникав
do {} while()
- як на мене,while() {}
читабельніший. Але то не критично. :) - Краще назвати метод
getEntryCount
- він натякає на отримання чогось
sdk/lib/lilka/src/lilka/sdcard.cpp
Outdated
serial_err("countFilesInDir listDir: not a directory: %s", path.c_str()); | ||
return 0; | ||
} | ||
File file; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ось перша ініціалізація, а в рядку 112 - повторна (shadowing). Відповідно, в while використовується оця (яка залишається неініціалізованою)
Якщо хочеш використовувати саме купу замість стеку - то можна залишити new, але оскільки нещовдавно в menu з'явився метод lilka::Menu menu("SD: " + path); // Створюємо меню відразу
size_t numEntries = lilka::sdcard.countFilesInDir(path);
lilka::Entry *entries = new lilka::Entry[numEntries]; // Видяліємо пам'ять для entries
lilka::sdcard.listDir(path, entries);
for (int i = 0; i < numEntries; i++) {
// Додаємо пункти на льоту, без виділення пам'яті для масивів назв/іконок/кольорів
menu.addItem(
entries[i].name,
entries[i].type == lilka::EntryType::ENT_DIRECTORY ? &folder : get_file_icon(filenames[i]),
lilka::EntryType::ENT_DIRECTORY ? lilka::display.color565(255, 255, 200) : get_file_color(filenames[i])
);
}
// Зразу звільняємо пам'ять:
delete[] entries;
// Все чисто, Menu само звільнить свою пам'ять, купа тепер чиста :) |
Там сама суть була якраз в тому що ми не можемо знати скільки файлів у директорії тому немає що і виділяти у стеку. Більше ніж 32 файли і воно гарантовано впаде. Повторної декларації змінної не помітив. :D та й таке |
не можу знайти в коді місце де оброблюється кнопка "<< Назад", та і в entries вона аж ніяк не додається, скоріше за все в попередній реалізації в стеку в entries[numEntries-1].name буде якесь сміття яке й заставить повернутися назад через return в попередній нескінченний цикл while :) |
Значно краще! Ще одне зауваження: оскільки там всякі break/return, в майбутньому може виникнути випадковий витік пам'яті, і потрібно буде пам'ятати викликати delete перед кожним return. Замість явного delete можна додати lilka::Entry* entries = new lilka::Entry[_numEntries];
+ std::unique_ptr<lilka::Entry[]> entriesPtr(entries);
// ...
- delete[] entries;
// ...
- delete[] entries; Коли |
звісно це вже зовсім прискіпливо, хоча на одну стрічку кода менше, чому ні =) |
Затестив - все працює. Дякую! |
Should work, but can't check :)