Added buffer files #101
Open
Added buffer files #101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kirkrodrigues
requested changes
Feb 9, 2023
| }; | ||
| } | ||
|
|
||
| #endif // COMPRESSOR_FRONTEND_BUFFER_HPP |
There was a problem hiding this comment.
Add empty newline at end of file.
| // Dynamic storage performs better as c-style arrays than as vectors | ||
| type* m_active_storage; | ||
| std::vector<type*> m_dynamic_storages; | ||
| type m_static_storage[cStaticByteBuffSize]; |
There was a problem hiding this comment.
Suggested change
| type m_static_storage[cStaticByteBuffSize]; | |
| std::array<type, cStaticByteBuffSize> m_static_storage; |
| } | ||
| m_dynamic_storages.clear(); | ||
| m_active_storage = m_static_storage; | ||
| m_curr_storage_size = cStaticByteBuffSize; |
There was a problem hiding this comment.
Suggested change
| m_curr_storage_size = cStaticByteBuffSize; | |
| m_curr_storage_size = m_static_storage.size(); |
| // Project Headers | ||
| #include "Constants.hpp" | ||
|
|
||
| /** |
There was a problem hiding this comment.
- Class header comment should go above the class definition.
- Comment lines should be less than 80 characters.
- This description is vague. You should ideally explain why you would need a static and a dynamic buffer, grouped together in one class.
| class Buffer { | ||
| public: | ||
| // Prevent copying of buffer as this will be really slow | ||
| Buffer (Buffer&&) = delete; |
There was a problem hiding this comment.
- If you're deleting the move constructor, you should delete the copy constructor as well (see rule of 5).
- Why're you deleting the move constructor?
- The move constructor should come after the constructor.
| public: | ||
|
|
||
| /** | ||
| * Increment buffer pos, swaps to a dynamic buffer (or doubles its size) if needed |
There was a problem hiding this comment.
- Should be in imperative form.
- Swapping to a dynamic buffer seems like an implementation detail (so it shouldn't be in the API header comment).
- It's not really clear to a reader what
posis. Perhaps this should be calledadvance_to_next_token?
|
|
||
| /** | ||
| * Resets output buffer | ||
| * @return |
| (Token*)malloc(2 * m_curr_storage_size * sizeof(Token))); | ||
| if (m_dynamic_storages.back() == nullptr) { | ||
| SPDLOG_ERROR("Failed to allocate output buffer of size {}.", m_curr_storage_size); | ||
| /// TODO: update exception when they're properly |
There was a problem hiding this comment.
When they're properly what?
| */ | ||
| namespace compressor_frontend { | ||
|
|
||
| class OutputBuffer : public Buffer<Token> { |
| } | ||
| m_dynamic_storages.clear(); | ||
| m_active_storage = m_static_storage; | ||
| m_curr_storage_size = cStaticByteBuffSize; |
There was a problem hiding this comment.
It's not in this review, but I would suggest changing cStaticByteBuffSize to be something like cStaticSingleBufferSize and make it be half of its current value. Then multiply it by 2 wherever necessary.
- Dropping "Byte" is because it's not just used for a byte buffer.
- Dividing it by 2 means we avoid a case where a developer later changes the size to something that's not evenly divisible.
|
Can we also add some tests, even if they're basic? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added 5 new files for buffer classes.