Use non-throwing std::filesystem functions in the type registry #1472
Conversation
| @@ -77,18 +77,23 @@ caf::error type_registry_state::save_to_disk() const { | |||
|
|
|||
| caf::error type_registry_state::load_from_disk() { | |||
| // Nothing to load is not an error. | |||
| if (!exists(dir)) { | |||
| std::error_code err{}; | |||
There was a problem hiding this comment.
Was this picked up via ADL? My goodness.
There was a problem hiding this comment.
dir is of type std::filesystem::path, so what else would you expect?
There was a problem hiding this comment.
No surprise we didn't catch this one during review.
There was a problem hiding this comment.
Ouch. Sorry I missed this when I introduced this change 😞
Thanks for the fix.
There was a problem hiding this comment.
No worries, these things happen.
There was a problem hiding this comment.
This is one of those things that are only obvious in hindsight. We probably would have run into the exact same issue.
There was a problem hiding this comment.
Pedantically, can you help me understand why exists unqualified would pick up std::filesystem::exists? I don't see us do a using namespace std; or anything like that. Who's bringing it into scope?
There was a problem hiding this comment.
See this recent article on (among other things) ADL: https://preshing.com/20210315/how-cpp-resolves-a-function-call/
Not yet, I wanted to use the CI artifact for that. |
Consider it approved if the issue is fixed ;) |
tobim
force-pushed
the
story/ch23539/filesystem-exception
branch
from
c5f3dbc
to
0b2da3b
Compare
|
It is indeed fixed. |
| auto dirs = get_schema_dirs(self->system().config()); | ||
| concepts_map concepts; | ||
| models_map models; | ||
| for (const auto& dir : dirs) { | ||
| if (!exists(dir)) | ||
| auto dir_exists = std::filesystem::exists(dir, err); | ||
| if (err) |
There was a problem hiding this comment.
I think we should do err.clear(). In the case where more than one directory doesn't exist (and since we continue in the loop), I think we want to warn on just the ones that don't exist rather than erroneously warn on all of the dirs remaining after the first one fails (and thus err is set). Does that make sense? 🤔
There was a problem hiding this comment.
exists() calls err.clear() itself if it didn't fail. See https://en.cppreference.com/w/cpp/filesystem/exists.
... in the type registry.
tobim
force-pushed
the
story/ch23539/filesystem-exception
branch
from
0b2da3b
to
0836774
Compare
📔 Description
Change 2 use sites of
exists().📝 Checklist