Move impure-related string allocation to helper methods #8609
Conversation
src/common/dsc.h
Outdated
| { | ||
| return isText(); // TODO: add isJson(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This does not look proper to me, as CHAR is not dynamic-length generally (except inside the impure). IMHO, it should be up to the caller (impure class) to check what string types are stored as variable-length.
There was a problem hiding this comment.
By this method I mean that dsc_length of content is not fixed. Now I remembered that there is an array type_lengths that checks exactly this, so I replace this method with the array
src/jrd/evl.cpp
Outdated
| string = value->vlu_string = FB_NEW_RPT(*pool, length) VaryingString(); | ||
| string->str_length = length; | ||
| } | ||
| VaryingString* string = value->getAllocateString(from.dsc_length, *pool); |
There was a problem hiding this comment.
Shouldn't it be value->getAllocateString(length, *pool) ?
src/jrd/val.h
Outdated
|
|
||
| template<class T> | ||
| VaryingString* getAllocateString(T requeuedLength, MemoryPool& pool) = delete; // Prevent dangerous length shrink | ||
| VaryingString* getAllocateString(USHORT requeuedLength, MemoryPool& pool); |
There was a problem hiding this comment.
I'd call it just getString() for simplicity but I'm not going to insist.
There was a problem hiding this comment.
getAllocateString is not a name that sounds good.
There was a problem hiding this comment.
Usually pool comes first in parameter list.
There was a problem hiding this comment.
Changed the name to getString with the pool as the first parameter
src/jrd/val.h
Outdated
| VaryingString* getAllocateString(USHORT requeuedLength, MemoryPool& pool); | ||
|
|
||
| void makeImpureDscAddress(MemoryPool& pool); | ||
| void allocateTextImpureDscAddress(MemoryPool& pool); |
There was a problem hiding this comment.
We're already inside the impure class, so please simplify the names, e.g. makeDscAddress() / setupDynamicDscAddress().
There was a problem hiding this comment.
These names are weird and maybe due to it, the methods too.
With current names, I prefer the original code.
There was a problem hiding this comment.
Names are my weak point. I tried others in the last commit, but it would be nice to get your suggestions.
src/jrd/val.h
Outdated
| this->vlu_desc.dsc_address = reinterpret_cast<UCHAR*>(&this->vlu_misc.vlu_int128); | ||
| } | ||
|
|
||
| inline VaryingString* impure_value::getAllocateString(USHORT requeuedLength, MemoryPool& pool) |
There was a problem hiding this comment.
I'd suggest newLength or just length instead of requeuedLength.
4 participants
This is also a preparation for the JSON Type implementation