clp-core: Refactor FileReader to use RAII. #496
Conversation
| m_segment_index_file_reader, | ||
| m_segment_index_decompressor | ||
| ); | ||
| read_new_entries(dictionary_path, segment_index_path); |
There was a problem hiding this comment.
Make it to be a separate function so the code looks cleaner. we can also put code directly into open and remove the function?
| @@ -262,38 +265,29 @@ void load_lexer_from_file( | |||
| } | |||
|
|
|||
| if (contains_delimiter) { | |||
| FileReader schema_reader; | |||
| ErrorCode error_code = schema_reader.try_open(schema_ast->m_file_path); | |||
| if (ErrorCode_Success != error_code) { | |||
There was a problem hiding this comment.
The exception throw here doesn't make sense. actually it would be hard to imagine that the file was opened once but failed to be opened again. so I would rather leave it to throw the default exception from File_reader
|
|
||
| // Read dictionary header | ||
| auto num_dictionary_entries = read_dictionary_header(m_dictionary_file_reader); | ||
| uint64_t num_dictionary_entries{}; |
There was a problem hiding this comment.
technically we can turn this part into a helper function and put into the dictionary_utils.cpp. But given how simple the code is, I feel it's also ok to leave it as it is.
Let me know if you think it's necessary
| ) { | ||
| constexpr size_t cDecompressorFileReadBufferCapacity = 64 * 1024; // 64 KB | ||
|
|
||
| FileReader dictionary_file_reader{dictionary_path}; |
There was a problem hiding this comment.
put them here instead of right in front of the code that uses them, so that if either file fails to open, we can skip the processing.
haiqi96
changed the title
haiqi96
changed the title
| uint64_t read_dictionary_header(FileReader& file_reader) { | ||
| auto dictionary_file_reader_pos = file_reader.get_pos(); | ||
| file_reader.seek_from_begin(0); | ||
| uint64_t num_dictionary_entries; | ||
| uint64_t num_dictionary_entries{}; |
| @@ -39,7 +16,7 @@ uint64_t read_segment_index_header(FileReader& file_reader) { | |||
| // Read segment index header | |||
| auto segment_index_file_reader_pos = file_reader.get_pos(); | |||
| file_reader.seek_from_begin(0); | |||
| uint64_t num_segments; | |||
| uint64_t num_segments{}; | |||
| unique_ptr<FileReader> grouped_file_path_reader; | ||
| try { | ||
| grouped_file_path_reader = make_unique<FileReader>(list_path); | ||
| } catch (FileReader::OperationFailed const& err) { |
There was a problem hiding this comment.
This is minor, but we usually use e, ex, or exception for exceptions.
| FileReader dictionary_file_reader; | ||
| FileReader segment_index_file_reader; | ||
| FileReader dictionary_file_reader{dictionary_path}; | ||
|
|
There was a problem hiding this comment.
Sorry, I didn't spot this earlier. This method was written as a way to reload the dictionary so that we can continue writing an archive, so although we don't use it right now, to maintain correctness, we'd need to also load the segment index, right?
I think an alternative solution is to get rid of open_and_preload entirely until we add support for opening and rewriting archives.
There was a problem hiding this comment.
If I have to choose between the two, maybe removing it.
There was a problem hiding this comment.
You also need to remove the method prototype.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
haiqi96
changed the title
Description
This PR refactors the FileReader so it does not use the open()/close() API. instead, it requires the FileReader to be constructed with a path to open. This prevents the potential risk of leaving the FileReader in an invalid state.
The PR also updates how FileReader is used in the code in multiple places. Originally, the code keeps a FileReader object and re-open and close it whenever a new file need to be read. With the new design, the code either create a static FileReader object with the target path and let it go out of scope by itself, or use a unique_ptr to keep manage the FileReader.
Validation performed