Replace StackErrorPtr in filesystem by exceptions #506
Conversation
|
Looking at the test cases this has convinced me that this is the way to go! Shall we merge #495 and then address your further comments either here or in smaller PRs? If the tests pass here i'm happy for this to be merged on top of 495 :) |
| StackErrorPtr enumerate_files(const std::string &file_path, const std::string &extension, std::vector<std::string> *files); | ||
| StackErrorPtr enumerate_files_recursive(const std::string &file_path, const std::string &extension, std::vector<std::string> *files); | ||
| StackErrorPtr enumerate_folders_recursive(const std::string &file_path, std::vector<std::string> *folders); | ||
| virtual bool folder_exists(const std::string &folder_path) const override; |
There was a problem hiding this comment.
It's a small thing. But worth pointing out should keep override and remove virtual.
There was a problem hiding this comment.
This is mainly my fault. Although its not needed, i've gotten into the habit of adding virtual to all virtual functions. We should address this in a new PR for the entire branch.
| { | ||
| if (!asset_manager) | ||
| { | ||
| return 0; | ||
| throw std::runtime_error("vfs/android_aasset_manager.cpp line" + std::to_string(__LINE__) + "AAsset Manager not initialized"); |
There was a problem hiding this comment.
Does it make sense to make this a macro and use something like:
#define ERROR_STR(...) (( FILE ":" LINE " " VA_ARGS))
Something to make the repeated tasks like listing out the file name a little less required and only capture the actual error response is what I'm looking for. Can change the above as needed.
There was a problem hiding this comment.
This should be addressed in a new PR, I will create an issue to follow up on this.
There was a problem hiding this comment.
I'm not a fan of having the filename hard-wired like this. When someone wants to change the filename at some point in the future, it's going to be a little irritating to have to change all these too.
FILE seems like a perfectly reasonable thing to use here, and hiding that inside a macro seems fine to me. If you really don't want the full path to appear in the output then maybe do some processing on the path to strip the front off.
|
I like this direction on the whole. I think it's good to take this after #495 |
TomAtkinsonArm
changed the title
more tests and fixes recursive tests more tests remove order specific checks from tests review comments
Note that android_aasset_manager.cpp is just edited, not compiled!
asuessenbach
force-pushed
the
exceptions
branch
from
b31f2cf
to
4d949d1
Compare
|
Merged! issues can be addressed in later PRs see #505 |
* std filesystem more tests and fixes recursive tests more tests remove order specific checks from tests review comments * fix duplicated check * review changes * Replace StackErrorPtr in vfs/filesystem.hpp by exceptions. Note that android_aasset_manager.cpp is just edited, not compiled! Co-authored-by: Thomas Atkinson <thomas.atkinson@arm.com>
Description
This PR shows how exceptions could be used in the new framework -> see #505.
All functions in vfs/filesystem.hpp that previously returned a StackErrorPtr have been changed to throw an exception on failure.
Besides that, the non-modifying functions have been marked as const, and some enumeration functions have been moved from RootFileSystem to FileSystem.
Note that
cwdin default.cpp. That test failed in my environment even before any of my changes.