Inflection 77: Adding C and C++ API for stating source grammemes and tests for the same. #174

BHK4321 · 2025-08-12T07:22:03Z

Issue #77
Could you please review this so that I could move forward for C API and XML parsing too.

grhoten · 2025-08-12T15:38:30Z

inflection/src/inflection/dialog/InflectableStringConcept.hpp

+     * @param value - The speakable string to convert to a concept
+     * @param intitialConstraints - The intitial constraints for the map.
+     */
+    InflectableStringConcept(const SemanticFeatureModel* model, const SpeakableString& value, const std::map<SemanticFeature, ::std::u16string> intitialConstraints);


The map should be a const & for the API.

grhoten · 2025-08-12T15:49:23Z

inflection/src/inflection/dialog/DisplayValue.hpp

+     * @param speakFeature The speakFeature from the SemanticFeatureModel that represents the SemanticFeature for the speak information for a SpeakableString.
+     * @param constraintMap The intitial constraint map.
+     */
+    DisplayValue(const SpeakableString& value, const SemanticFeature& speakFeature, const ::std::map<SemanticFeature, ::std::u16string> constraintMap);


The map should be a const & for the API.

grhoten · 2025-08-12T15:52:40Z

inflection/src/inflection/dialog/DisplayValue.cpp

-DisplayValue::DisplayValue(const SpeakableString& value, const SemanticFeature& speakFeature)
-    : DisplayValue(value.getPrint(), {})
+DisplayValue::DisplayValue(
+    const SpeakableString& value,
+    const SemanticFeature& speakFeature, 
+    const ::std::map<SemanticFeature, ::std::u16string> constraintMap)
+    : DisplayValue(value.getPrint(), constraintMap)
 {
    if (!value.speakEqualsPrint()) {
-        constraintMap.emplace(speakFeature, value.getSpeak());
+        this->constraintMap.emplace(speakFeature, value.getSpeak());
    }
 }

+DisplayValue::DisplayValue(
+    const SpeakableString& value,
+    const SemanticFeature& speakFeature)
+    : DisplayValue(value.getPrint(), {})
+{
+    if (!value.speakEqualsPrint()) {
+        this->constraintMap.emplace(speakFeature, value.getSpeak());
+    }
+}


Can you please declare these functions in the same order as the header? It makes it easier to navigate.

Also the 2 argument constructor with the speakFeature can now call the new constructor with an empty map. Then the redundant if statement can be removed. It's best to keep the copy and paste code to a minimum to improve the maintenance.

grhoten · 2025-08-12T15:54:41Z

inflection/src/inflection/dialog/InflectableStringConcept.cpp

-InflectableStringConcept::InflectableStringConcept(const SemanticFeatureModel* model, const SpeakableString& value)
+InflectableStringConcept::InflectableStringConcept(
+    const SemanticFeatureModel* model,
+    const SpeakableString& value,
+    const ::std::map<SemanticFeature, ::std::u16string> intitialConstraints
+)
+    : super(model)
+    , value(value)
+    , defaultDisplayValue(value, *npc(super::getSpeakFeature()), intitialConstraints)
+{
+}
+
+InflectableStringConcept::InflectableStringConcept(
+    const SemanticFeatureModel* model,
+    const SpeakableString& value
+)


Can you please declare these functions in the same order as the header? It makes it easier to navigate.

Also the 2 argument constructor with the SpeakableString can now call the new constructor with an empty map. Then the copied code can be removed. It's best to keep the copy and paste code to a minimum to improve the maintenance.

Sure I would keep that in mind.
Being a fresher I wasn't aware.

BHK4321 · 2025-08-24T11:39:46Z

@grhoten I have added the C API too can you please review and tell me the next steps.

grhoten · 2025-08-25T01:14:51Z

inflection/src/inflection/dialog/InflectableStringConcept.h

+ *        This is set to a failure when a failure has occurred during execution.
+ */
+INFLECTION_CAPI IDInflectableStringConcept* iinf_createWithConstraints(const IDSemanticFeatureModel* model, const IDSpeakableString* value, 
+                                                                       const inflection::dialog::SemanticFeature* features, const char16_t** values, int32_t SemanticFeaturesLen, UErrorCode* status);


C++ API is not allowed in C API. Please see ipron_createWithDefaults for how to pass in such values. That API also ensures that the 2 lengths are exactly the same.

For consistency with the PronounConcept API, it would be ideal if this was also named iinf_createWithDefaults to be consistent with the naming.

BHK4321 · 2025-08-25T13:31:26Z

@grhoten
I have updated the code accordingly, could you please consider merging this PR I will continue with this issue #77 (the testing and XML parsing) after some time.
Thanks

grhoten

All new API need tests. The code should always be in a releasable state. Please add tests for these changes. The current code should be well over 90% line code coverage.

Please also state which issue is being fixed with these changes. That is best done with prefixing your commits and pull request summary with "Inflection-77".

grhoten · 2025-08-25T15:10:57Z

inflection/src/inflection/dialog/InflectableStringConcept.h

 #include <inflection/dialog/SemanticFeatureConcept.h>
 #include <inflection/dialog/SemanticFeatureModel.h>
-
+#include <inflection/dialog/SemanticUtils.hpp>


This internal C++ header should not be used in public C API.

I will fix that

grhoten · 2025-08-25T15:13:53Z

inflection/src/inflection/dialog/PronounConcept.cpp

    if (pronounData->numValues() == 0) {
        throw ::inflection::exception::IllegalArgumentException(u"Display data can not be empty.");
    }
+


I don't think that this file needs to change.

Yup sure, I mistakenly added a line and forgot

…lections

BHK4321 · 2025-08-27T16:13:03Z

@grhoten
Please review this commit and I had some questions while working too:

Is the compareInflection function used anywhere else other than the InflectionTest file ? Because I changed the parameters of the function.
Could you please list all the tests which are required to be added after this optional <initial> parsing code has been completed.
Is there any C file too which I need to update just like this InflectionTest ?

BHK4321 · 2025-08-29T15:21:56Z

@grhoten
I did the test these are the results please tell what might be going wrong:

grhoten · 2025-09-10T05:31:16Z

Please review this commit and I had some questions while working too:

Is the compareInflection function used anywhere else other than the InflectionTest file ? Because I changed the parameters of the function.

static functions are not available outside of the file. It also compiles. So that implies that the changes are OK.

Could you please list all the tests which are required to be added after this optional <initial> parsing code has been completed.

As long as there are a couple of tests, that should be sufficient to show the utility of the API.

Is there any C file too which I need to update just like this InflectionTest ?

I recommend including a test of iinf_createWithDefaults in InflectableStringConceptTest-c.cpp.

I did the test these are the results please tell what might be going wrong:

DictionaryLookupFunction::getFeatureValue likely needs to be updated to query the relevant values from displayValue. It should probably query DisplayValue::getFeatureValue or DisplayValue::getConstraintMap

grhoten · 2025-09-10T05:32:07Z

inflection/test/resources/inflection/dialog/inflection/es.xml

+    <test><source definiteness="definite">cometa</source><initial gender="masculine"/><result number="singular" gender="masculine">el cometa</result></test>
+    <test><source definiteness="definite">cometa</source><initial gender="feminine"/><result number="singular" gender="feminine">la cometa</result></test>
+    <test><source definiteness="definite">QQQQ</source><initial gender="masculine" number="plural"/><result number="plural" gender="masculine">los QQQQ</result></test>
+    <test><source definiteness="definite">QQQQ</source><initial gender="feminine" number="plural"/><result number="plural" gender="masculine">las QQQQ</result></test>


These tests look reasonable.

BHK4321 · 2025-09-13T10:26:30Z

inflection/src/inflection/dialog/DictionaryLookupFunction.cpp

+    }
    return new SpeakableString(result);
 }



@grhoten
Was this the expected behavior in this function? If not, could you please point out where I am going wrong?

No. Within the DictionaryLookupFunction, you're only returning the grammeme value. This does not need to change.

DictionaryLookupFunction::getFeatureValue likely needs to be updated to query the relevant values from displayValue. It should probably query DisplayValue::getFeatureValue or DisplayValue::getConstraintMap

Okay sure I wanted to know exactly what I need to do, based on your previous comment I came up with this commit, could you please tell what is expected to make the tests pass so that i could implement the same and further add tests in InfectableStringConceptTest-c.cpp as well to complete this task ?

…createWithDefaults as InflectableStringConceptTest-c#testCreateWithDefaults

BHK4321 · 2025-11-03T18:02:09Z

Hello!!
@grhoten
As you can see all tests for the new API for C and C++ are passing as you can see in the Ubuntu build logs , and some error occured in mac os build, I don't know the reason behind it.

grhoten · 2025-11-06T04:09:54Z

inflection/src/inflection/dialog/InflectableStringConcept.cpp

+    auto initialConstraint = initialConstraintMap.find(feature);
+    if (initialConstraint != initialConstraintMap.end()) {
+        return new SpeakableString(initialConstraint->second);
+    }


These changes are confusing. Why are these changes needed?

More precisely, why is getFeatureValue being called here? Why is the feature name being checked for gender? What's special about gender or number? Why not grammatical case?

auto displayConstraint = displayConstraintMap.find(feature); if (displayConstraint != displayConstraintMap.end()) { auto numberFeature = npc(getModel())->getFeature(u"number"); if (feature.getName() == u"gender" && baseValue && numberFeature != nullptr && displayConstraintMap.find(*npc(numberFeature)) != displayConstraintMap.end()) { return baseValue.release(); } return new SpeakableString(displayConstraint->second); }

This is the test case which was failing before the changes:

<test> <source definiteness="definite">cometa</source> <initial gender="masculine"/> <result gender="masculine" number="singular">el cometa</result> </test>

InflectableStringConcept::getFeatureValue asks the default display function for a rendered DisplayValue, so feature lookups reflect whatever the display layer injected (articles, inflections, etc.). Spanish determiners add both gender and number to that map, and the gender can contradict the lexeme default (la cometa).

To avoid reporting the article’s guess as the noun’s gender, the code detects feature == "gender" and, when the number constraint is present too, returns the lexeme’s base value instead of the display-layer value. English articles never set gender, and no language mutates case in this path yet (I am not sure though), so only the gender/number pair needs a guard today.

InflectableStringConcept::getFeatureValue asks the default display function for a rendered DisplayValue, so feature lookups reflect whatever the display layer injected (articles, inflections, etc.).

Spanish determiners add both gender and number to that map, and the gender can contradict the lexeme default (la cometa).
To avoid reporting the article’s guess as the noun’s gender, the code detects feature == "gender" and, when the number constraint is present too, returns the lexeme’s base value instead of the display-layer value, as these conditions when satisfied point to the case when we have an article being inserted.

Grammatical case isn’t treated specially because the Spanish article logic never synthesizes case information—so there’s no regression to guard against. If a language later injected case-altering determiners we’d need a similar safeguard, but today only the gender/number pair exhibits the mismatch.

BHK4321 · 2025-11-06T13:06:32Z

@grhoten
This final commit resolves the macos build case too by removing the hard coded include path of xml2 in the CMakeLists and using:

find_package(LibXml2 REQUIRED)

instead to get the include path automatically.

Adding C++ API for stating source grammemes

c008f15

BHK4321 marked this pull request as draft August 12, 2025 07:40

BHK4321 marked this pull request as ready for review August 12, 2025 07:40

grhoten reviewed Aug 12, 2025

View reviewed changes

BHK4321 added 3 commits August 12, 2025 17:15

updates

c8f22ec

Merge branch 'unicode-org:main' into main

70fa3e1

C API

9c8ba42

grhoten reviewed Aug 25, 2025

View reviewed changes

Done

c41ec8c

grhoten requested changes Aug 25, 2025

View reviewed changes

BHK4321 changed the title ~~Adding C++ API for stating source grammemes~~ Inflection 77: Adding C++ API for stating source grammemes Aug 26, 2025

BHK4321 changed the title ~~Inflection 77: Adding C++ API for stating source grammemes~~ Inflection 77: Adding API for stating source grammemes Aug 26, 2025

Inflection-77 Adding optional <initial> tag in InflectionTest#testInf…

e676a21

…lections

Inflection-77 Test results

873db46

grhoten reviewed Sep 10, 2025

View reviewed changes

BHK4321 added 2 commits September 13, 2025 00:20

Merge branch 'unicode-org:main' into main

52dc3d1

Inflection-77 DictionaryLookupFunction::getFeatureValue

e3851a6

BHK4321 commented Sep 13, 2025

View reviewed changes

Inflection-77: Final issue resolved and added one CAPI test for iinf_…

0c4bbeb

…createWithDefaults as InflectableStringConceptTest-c#testCreateWithDefaults

grhoten reviewed Nov 6, 2025

View reviewed changes

Inflection-77: Use CMake’s LibXml2 package

fa7133e

BHK4321 changed the title ~~Inflection 77: Adding API for stating source grammemes~~ Inflection 77: Adding API for stating source grammemes and tests for the same Nov 6, 2025

BHK4321 changed the title ~~Inflection 77: Adding API for stating source grammemes and tests for the same~~ Inflection 77: Adding C and C++ API for stating source grammemes and tests for the same. Nov 6, 2025

Uh oh!

Inflection 77: Adding C and C++ API for stating source grammemes and tests for the same. #174

Are you sure you want to change the base?

Inflection 77: Adding C and C++ API for stating source grammemes and tests for the same. #174

Uh oh!

Conversation

BHK4321 commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 commented Aug 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grhoten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 commented Aug 27, 2025

Uh oh!

BHK4321 commented Aug 29, 2025

Uh oh!

grhoten commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BHK4321 commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BHK4321 commented Aug 12, 2025 •

edited

Loading

BHK4321 commented Aug 25, 2025 •

edited

Loading

BHK4321 Sep 13, 2025 •

edited

Loading

BHK4321 Sep 14, 2025 •

edited

Loading

BHK4321 commented Nov 6, 2025 •

edited

Loading