FileWriter and some FileReader refactoring #3383
Conversation
|
I'd be happy to get input as for how tests should be conducted for this specific part of infrastructure - iirc @daniel-j-h mentioned that all Cucumber tests cover the IO layer implicitly anyway, and I couldn't find additional existing unit tests for the |
| } | ||
|
|
||
| FileWriter(const boost::filesystem::path &filename_, const FingerprintFlag flag) | ||
| : filename(filename_.string()) |
There was a problem hiding this comment.
Why don't we store a boost::fs::path to begin with? :)
There was a problem hiding this comment.
Sure, will do so across both FileReader and FileWriter.
| { | ||
| output_stream.open(filename_, std::ios::binary); | ||
| if (!output_stream) | ||
| throw util::exception("Error opening " + filename + ":" + std::strerror(errno)); |
There was a problem hiding this comment.
Would it make sense to have separate IO exceptions? (Check the Gist in #2242 (comment)).
(btw all those commends apply to the FileReader, too cc @ghoshkaj @danpat)
There was a problem hiding this comment.
Yea I generally agree, however, I'd suggest opening another issue for that - at the moment exception (https://github.com/Project-OSRM/osrm-backend/blob/master/include/util/exception.hpp) is marked final, we'd be better of differentiating exceptions across the board in a refactor.
| const auto &result = output_stream.write(reinterpret_cast<char *>(src), count * sizeof(T)); | ||
| if (!result) | ||
| { | ||
| throw util::exception("Error writing to " + filename + ": " + std::strerror(errno)); |
There was a problem hiding this comment.
Hrm does an error here really set errno?
There was a problem hiding this comment.
Probably not, will remove.
| @@ -54,7 +54,8 @@ template <typename simple_type> | |||
| bool serializeVectorIntoAdjacencyArray(const std::string &filename, | |||
| const std::vector<std::vector<simple_type>> &data) | |||
| { | |||
| std::ofstream out_stream(filename, std::ios::binary); | |||
| storage::io::FileWriter file(filename, storage::io::FileWriter::HasNoFingerprint); | |||
There was a problem hiding this comment.
There's a clang-format script at scripts/format.sh - once your done with changes it would be great to run this.
(if you don't have clang-format we can do this for you before merging, too)
duizendnegen
force-pushed
the
feature/file-writer
branch
from
156c13b to
21455c5
Compare
|
@daniel-j-h for further review |
duizendnegen
changed the title
duizendnegen
force-pushed
the
feature/file-writer
branch
from
6c749c8 to
260249d
Compare
|
Travis fails because of variant include path. I think @danpat fixed it yesterday. Try squashing, rebasing, force pushing. |
| if (!result) | ||
| { | ||
| throw util::exception("Error writing to " + filepath.string() + ": " + | ||
| std::strerror(errno)); |
There was a problem hiding this comment.
I still can't find the documentation for o(f)streams setting errno.
There was a problem hiding this comment.
Yea I dropped it from the FileWriter but forgot to remove it from the FileReader, will do so.
There was a problem hiding this comment.
FYI it does set errno, at least, based on the tests it does.
There was a problem hiding this comment.
@duizendnegen it is an ad-hoc platform-dependent result that may not correspond to a stream method call.
@daniel-j-h errno shows the thread-local result of the last syscall and without knowing stream internals it can be unrelated and will not be fixed in gcc.
There was a problem hiding this comment.
Thanks for the pointer @oxidase, I updated the PR again to reflect these learnings.
duizendnegen
force-pushed
the
feature/file-writer
branch
from
260249d to
9ac199e
Compare
|
Rebased, squashed, let's do this! 🎉 |
duizendnegen
force-pushed
the
feature/file-writer
branch
from
9ac199e to
87f8e8a
Compare
…ter and FileReader
duizendnegen
force-pushed
the
feature/file-writer
branch
from
87f8e8a to
6503aae
Compare
|
Looks good to me 👍 merging. |
Issue
FileWritercounterpart toFileReaderand one example implementation.Tasklist
Requirements / Relations
iostreamwithFileReaderFileabstraction