Fix memory leak in MF2Factory.cpp (issue/131) #132
Conversation
Need to delete the return of toSpeakableString() and getFeatureValue() return from InflectableStringConcept. Fix ICU cache
| return FormattedPlaceholder("inflection"); | ||
| } | ||
| result += string->getPrint(); | ||
| delete string; |
There was a problem hiding this comment.
I've been leaning towards using unique_ptr these days, especially in these short contexts. It handles all of the cleanup. Also null is a valid value and not an error, but it's exceptional, like null was provided to format. Can you switch this to std::unique_ptr?
Can you clarify the purpose of the FormattedPlaceholder("inflection") part? Why is inflection provided instead of some other value?
There was a problem hiding this comment.
icu::message2::FormattedPlaceholder::FormattedPlaceholder ( const UnicodeString & s )
inlineexplicit
Fallback constructor.
Constructs a value that represents a formatting error, without recording an input Formattable as the source.
Parameters
s An error string. (See the MessageFormat specification for details on fallback strings.)
There was a problem hiding this comment.
Also null is a valid value and not an error, but it's exceptional, like null was provided to format.
The API doc does not mention that. How can we distinguish the case that it return nullptr because out of memory, error condition or the input is null?
/**
* Converts the concept to a Speakable String
*
* @return Speakable String
*/
SpeakableString* toSpeakableString() const override;
In the code above, in what case it could be the input is null? I do not belive it is possible, right? so the only possible it return null is error condition, right?
Maybe we should file a bug to add more detail to the API doc. Currently it only say "return Speakable String" without mention it could return nullptr neither require the caller to free it.
There was a problem hiding this comment.
You'll have to see inflection::dialog::SemanticConcept or inflection::dialog::PronounConcept for the important part of the API that says that null can be returned.
/**
* Returns the requested value for the provided constraints.
* @return null when the constrained values is an empty set.
*/
::inflection::dialog::SpeakableString* toSpeakableString() const override;
Clarifying the documentation further is a good idea.
There was a problem hiding this comment.
If FormattedPlaceholder is being used instead of a Morphun exception because of an API contract, then it should provide an informative message. An exception may still be thrown in other conditions, like a memory allocation error or an unexpected file loading error.
There was a problem hiding this comment.
What better informative message other than "inflection" would you suggest I replace here. I will replace with your recommended error message.
There was a problem hiding this comment.
ICU internally will not throw exception. Please read https://unicode-org.github.io/icu/userguide/icu4c/faq.html#how-are-errors-handled-in-icu
There was a problem hiding this comment.
You'll have to see inflection::dialog::SemanticConcept or inflection::dialog::PronounConcept
But the API doc of the class I am using InflectableStringConcept, didn't mention I need to also read the API doc SemanticConcept or PronounConceptof in order to understand how to use this class.
In fact, the API doc of InflectableStringConcept didn't mention a single word of SemanticConcept or PronounConceptof, how could any developers know they need to also read the API doc of that two classes?
It will not be developer friendly to request them to read every single .hpp file before they start to write code to use a single class, right? BTW, I still have no clue about the relationship between SemanticConcept, PronounConcept and InflectableStringConcept. Are the relationship between these three class documented in anywhere in the repo?
Do we need to know the detail of SemanticConcept and PronounConcept before we can use InflectableStringConcept? I thought InflectableStringConcept is very high level and could be used with dealing with SemanticConcept and PronounConcept , right?
There was a problem hiding this comment.
I've been leaning towards using unique_ptr these days, especially in these short contexts
Sure. changed in this file. I also filed #134 for switching to std::unique_ptr on other usage and will work on smaller PR to move them one by one. But this PR is now blocking the verificaiton of them because of the leak and the ICU cache fix
There was a problem hiding this comment.
Do we need to know the detail of SemanticConcept and PronounConcept before we can use InflectableStringConcept? I thought InflectableStringConcept is very high level and could be used with dealing with SemanticConcept and PronounConcept , right?
I agree that the documentation needs polishing. Each SpeakableConcept has its nuanced reason for existing for different contexts.
InflectableStringConcept is a blunt object when it comes to complete personal pronoun support, and it's not as customizable and disambiguable as SemanticConcept. It can't handle inflect numbers as well as NumberConcept. It can't use lists, but it can be used in a SemanticConceptList. Each subclass has a reason for existing, and this is where documentation can help.
With that being said, please change FormattedPlaceholder("inflection") to either FormattedPlaceholder(arg, FormattedValue(std::move(result))) (some implementations do map null to an empty string for easier matching), FormattedPlaceholder(u"U_MF_FORMATTING_ERROR"), or some meaningful error message. If the goal is to support null, then an empty string should be used if the equivalent of null can't be returned, but that would mean not setting the UErrorCode. If it's an supposed to be an error, something should be returned so that the result can be searchable, debuggable, and meaningful enough to know how to call the API differently.
| } | ||
| feature = result->getPrint(); | ||
| delete result; |
There was a problem hiding this comment.
This condition should most definitely be handled, but null is never an error. It's a value. When the gender is ambiguous or unknown, null will be returned. For a pronoun like "they" the gender is unknown or epicene, which is returned as null.
There are plenty of times that you need to provide default behavior in the message for unknown values. If the MF API does not provide a null value, then an empty string should be provided instead.
There was a problem hiding this comment.
The API doc currently just states "return Speakable String" and "Returns the Speakable String representing the given semantic feature" without mention it will return nullptr and the sematic of a returned nullptr. How about you first improve the API doc to include these details so the semantic of these function is clearly defined. I lack the power to read the following comment to conclude what you stated above and I am pretty sure this review comment will be lost inside this PR and most of the developers won't be able to reach the same conclusion untill we clearly mention that in some API document.
/**
* Returns the Speakable String representing the given semantic feature.
*
* @param feature - the semantic feature for which the value is requested.
* @return Speakable String
*/
SpeakableString* getFeatureValue(const SemanticFeature& feature) const override;
There was a problem hiding this comment.
Feel free to create an issue to improve the documentation in this area.
|
PTAL and let me know what else need to fix before merging. this is blocking all other PR to be clear. Let's fix the important thing first |
Need to delete the return of toSpeakableString() and getFeatureValue() return from InflectableStringConcept. Fix #131
Also need to fix a github change to properly save icu into cache