21364: Improves entity persistence: works as single transactional files as well as performance improvements, MINOR #315
Conversation
src/Amalgam/AssetManager.cpp
Outdated
| std::array<char, 201> buffer; | ||
| file.read(buffer.data(), 200); | ||
| buffer[200] = '\0'; | ||
| std::string str(buffer.data(), 200); |
There was a problem hiding this comment.
Should this code be split out into a helper function? (I think the byte-level char array manipulation should be sketchy in modern C++ but can't immediately find an alternative, and it's easier to justify in comments just once.)
There was a problem hiding this comment.
It's in two spots really close to each other, each of which needs its own regular expression. I could merge the methods, but don't know if that's better.
| } | ||
| } | ||
|
|
||
| EvaluableNodeReference AssetManager::LoadResource(AssetParameters *asset_params, EvaluableNodeManager *enm, |
There was a problem hiding this comment.
Should this be AssetParameters &asset_params (a reference and not a pointer) so that you don't have to change all of the uses of it? (Do you ever reassign which AssetParameters it is?). Should it be a const &? (Does the code ever change the parameters, or just read them?)
There was a problem hiding this comment.
If only you saw how many times I switch back and forth between passing as a reference and as a pointer in this work :-). Many of the other methods do use shared pointers. I could make two of the methods LoadResource and StoreResource work on AssetParams as a reference, but they'd be inconsistent with the rest that deal with pointers or shared pointers. Would changing just those two be preferable?
No description provided.