Skip to content

test(linux): Add unit tests for mcompile#15892

Open
Markus-SWAG wants to merge 23 commits into
masterfrom
feat/linux/mnemonic-keyboard-support-unit-tests
Open

test(linux): Add unit tests for mcompile#15892
Markus-SWAG wants to merge 23 commits into
masterfrom
feat/linux/mnemonic-keyboard-support-unit-tests

Conversation

@Markus-SWAG
Copy link
Copy Markdown
Contributor

@Markus-SWAG Markus-SWAG commented Apr 28, 2026

@keymanapp-test-bot skip

This is a first try to add unit tests to mcompile.
There is a new file converter.cpp containing a minimal main. This allows the tests to have an own main provided by googletest.
Since gsettings are used, there is the need for new include files and new library dependencies.
The separation of converter.cpp required some refactoring of the header files.

@keymanapp-test-bot keymanapp-test-bot Bot added the user-test-missing User tests have not yet been defined for the PR label Apr 28, 2026
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot Bot commented Apr 28, 2026

User Test Results

Test specification and instructions

User tests are not required

@keymanapp-test-bot keymanapp-test-bot Bot added this to the A19S28 milestone Apr 28, 2026
@github-project-automation github-project-automation Bot moved this to Todo in Keyman Apr 28, 2026
This is temporary, when the transition to xkblibcommon is done,
wayland will be available.
@keymanapp-test-bot keymanapp-test-bot Bot removed the user-test-missing User tests have not yet been defined for the PR label Apr 28, 2026
Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:+1 Looks good.

As we talked in the huddle, I'm not yet approving it so that it doesn't get accidentally merged while the tests won't work on the noble ba.

Comment thread linux/mcompile/keymap/meson.build Outdated
include_directories : common_include_dir
)

gtest = subproject('gtest')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since gtest is a dependency for the tests, I think it would better to have this line in test/meson.build (which works even if the subprojects folder is in mcompile/keymap).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

}


void get_default_layout() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name get_default_layout suggests that it returns the default layout. A different name might be better (retrieve_default_layout? - not much better)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, anyway this method will go away.

59, 60, 61, 123, 94};


TEST_F(KeyboardConversionTest, KMXgetKeyValUnderlyingFromKeyCodeUnderlyingBase) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider making this (and the others) a parameterized test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, done.

Comment thread linux/mcompile/keymap/converter.cpp Outdated
Comment on lines +10 to +11
int main(int argc, char* argv[]) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
int main(int argc, char* argv[]) {
int main(int argc, char* argv[]) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

LPKMX_KEYBOARD kmxfile;

if (!KMX_LoadKeyboard(infile, &kmxfile)) {
KMX_LogError(L"Failed to load keyboard (%d)\n", errno);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to delete kmxfile here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On failure of KMX_LoadKeyboard, kmxfile should still be nullptr.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) { // I4552F
if(!KMX_SaveKeyboard(kmxfile, outfile)) {
KMX_LogError(L"Failed to save keyboard (%d)\n", errno);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
KMX_LogError(L"Failed to save keyboard (%d)\n", errno);
KMX_LogError(L"Failed to save keyboard (%d)\n", errno);
delete kmxfile;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Comment thread linux/mcompile/keymap/converter.cpp Outdated
return 3;
}

if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) { // I4552F
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the I* numbers - they refer to a no-longer existing bug tracking system from earlier days of Keyman. For new code they don't make sense and should be omitted IMO.

Suggested change
if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) { // I4552F
if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that those issue numbers can be found at https://github.com/keymanapp/legacy-issues

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left the numbers

* @brief print (error) messages
* @param fmt text to print
*/
void KMX_LogError(const wchar_t* fmt, ...) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to use C++ style output in this function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I found a nice one liner, but this is only possible with C++ 20. So I suggest to wait until we are there.

Comment thread linux/mcompile/keymap/main.cpp
@darcywong00
Copy link
Copy Markdown
Contributor

We have a convention for pull request titles

Reference

https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes#title

I suggest changing the title to something like test(linux): Add unit tests for mcompile

This way, the (linux) scope ensures this pull request title automatically gets added to the "Keyman for Linux changlog at
https://help.keyman.com/products/linux/version-history/

Comment thread linux/mcompile/keymap/test/keymap.tests.cpp
@@ -0,0 +1,78 @@
#include "mcompile.h"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add our standard header.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to all files, where the header was missing.

Comment thread linux/mcompile/keymap/converter.cpp Outdated
return 3;
}

if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) { // I4552F
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that those issue numbers can be found at https://github.com/keymanapp/legacy-issues

Comment thread linux/mcompile/keymap/converter.cpp Outdated
//
// 3. Write the new keyman keyboard file

LPKMX_KEYBOARD kmxfile;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LPKMX_KEYBOARD kmxfile;
LPKMX_KEYBOARD kmxfile = nullptr;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

LPKMX_KEYBOARD kmxfile;

if (!KMX_LoadKeyboard(infile, &kmxfile)) {
KMX_LogError(L"Failed to load keyboard (%d)\n", errno);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On failure of KMX_LoadKeyboard, kmxfile should still be nullptr.

@@ -0,0 +1,299 @@
#include <gtest/gtest.h>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add standard header comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

- adding header files
- renaming files and methods
- restructuring meson build
@Markus-SWAG Markus-SWAG changed the title adding unit tests for Linux mcompile test(linux): Add unit tests for mcompile May 5, 2026
Comment thread linux/mcompile/keymap/test/keymap.tests.cpp
@@ -1,3 +1,11 @@
/*
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Org got renamed from "SIL International" to "SIL Global" last year.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) SIL Global. MIT License.

According to the style guide, we don't put the year into the header because it just adds noise. Year can be found from the git history.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Comment thread linux/mcompile/keymap/deadkey.h Outdated
Comment thread linux/mcompile/keymap/keymap.h Outdated
@@ -1,3 +1,9 @@
/*
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Comment thread linux/mcompile/keymap/keymap.cpp Outdated
@@ -1,5 +1,5 @@
/*
* Keyman is copyright (C) 2004 - 2024 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread linux/mcompile/keymap/main.cpp Outdated
@@ -1,3 +1,11 @@
/*
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Comment thread linux/mcompile/keymap/main.cpp Outdated
}
}

delete kmxfile;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, back again

Comment thread linux/mcompile/keymap/mc_import_rules.h Outdated
@@ -1,3 +1,9 @@
/*
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, years erased, too

Comment thread linux/mcompile/keymap/mc_kmxfile.cpp Outdated
@@ -1,5 +1,5 @@
/*
* Keyman is copyright (C) 2004 - 2024 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, years erased, too

Comment thread linux/mcompile/keymap/mc_kmxfile.h Outdated
@@ -1,3 +1,9 @@
/*
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Keyman is copyright (C) 2004 - 2026 SIL International. MIT License.
* Keyman is copyright (C) 2004 - 2026 SIL Global. MIT License.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, years erased, too

@github-actions github-actions Bot added the test Any acceptance test issue label May 6, 2026
Comment on lines +32 to +35
this->keycode = k;
this->expected_char = e;
this->layout = l;
this->shiftstate = s;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary if we have keycode(k) etc above? Seems to be doppelt gemoppelt 😄

}

guint get_keycode() {
return keycode;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation is wrong. Per coding style we indent each level by 2. The .editorconfig file should give you that for free if the extension is installed in vscode.

Suggested change
return keycode;
return keycode;

Comment on lines +60 to +62
expected_keysyms = e;
layout = l;
shiftstate = s;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above

Comment on lines +81 to +82
EXPECT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";
for (guint k = 0; k < keycodes.size() && k < expected_keysyms.size(); k++) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't make sense to continue if the the two vectors have different size, so we could use ASSERT_EQ here. That would also simplify the for loop slightly. Also, since we're using a std vector it doesn't really make sense to use a Gnome type for k - doesn't really matter but will reduce dependency. (I'm not sure - is there a uint or should it be unsigned int).

Suggested change
EXPECT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";
for (guint k = 0; k < keycodes.size() && k < expected_keysyms.size(); k++) {
ASSERT_EQ(keycodes.size(), expected_keysyms.size()) << "Keycodes and expected keysyms vectors must be of the same size.";
for (uint k = 0; k < keycodes.size(); k++) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When ASSERT_* fails, doesn't it prevent all further instantiations from running?

INSTANTIATE_TEST_SUITE_P(AltGrUs,
GetKeyValUnderlyingFromKeyCodeUnderlyingTest,
testing::ValuesIn(KeyboardTestParameters(
{u'a', u'b', u'c', u'd', u'e', u'f', u'g', u'h', u'i', u'j', u'k',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to break the lines at the same place for all the tests so that it's easier to see the differences in the data between tests.

Comment on lines +298 to +301
keycode = k;
expected_char = e;
layout = l;
shiftstate = s;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

necessary?


};

class GetKeyValFromKeyCodeTestParameters {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference to KeyboardTestParameters ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more element for caps.

Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

AltGrDe,
GetKeyValUnderlyingFromKeyCodeUnderlyingTest,
testing::ValuesIn(KeyboardTestParameters(
{u'æ', u'\xfffe', u'¢', u'ð', u'\xfffe', u'\xfffe', u'\xfffe', u'\xfffe', u'\xfffe', u'\xffff',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it that we have u'\xffff' for some values and u'\xfffe' or u'\000' for others?

Comment on lines +291 to +294
expected_keysyms = e;
layout = l;
shiftstate = s;
caps = c;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expected_keysyms = e;
layout = l;
shiftstate = s;
caps = c;

Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, forgot to check devin.ai! That found a few issues that might cause problems in the future.

Comment thread linux/mcompile/keymap/test/meson.build Outdated
Comment thread linux/mcompile/keymap/mc_kmxfile.cpp
Comment thread linux/mcompile/keymap/meson.build Outdated
Comment on lines +12 to +13
giomm_dep = dependency('giomm-2.4')
glibmm_dep = dependency('glibmm-2.4')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are only used in tests we should probably move them to test/meson.build - unless you'll need them to be here in a later PR.

Flagged by devin.ai:

giomm/glibmm dependencies fetched unconditionally despite only being needed for tests

In linux/mcompile/keymap/meson.build:12-13, giomm-2.4 and glibmm-2.4 are declared as unconditional dependencies at configure time, even though they are only used in the test executable (test/meson.build:9). This means the main mcompile build will fail at configure time if these packages are not installed, even though the main binary doesn't need them. Consider wrapping these in a required: false or conditionalizing on whether tests are enabled, or moving the dependency declarations into test/meson.build.

Comment thread linux/mcompile/keymap/deadkey.h Outdated
Comment thread linux/mcompile/keymap/main.cpp Outdated
Comment thread linux/mcompile/keymap/mc_kmxfile.cpp Outdated
Comment on lines 582 to 586
@@ -582,3 +586,28 @@ FILE* Open_File(const KMX_CHAR* filename, const KMX_CHAR* mode) {
return fopen(cpath.c_str(), cmode.c_str());
Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagged by devin.ai:

Suggested change
return fopen(cpath.c_str(), cmode.c_str());
return fopen(filename, mode);

Hmm, looks GH has a problem displaying this change. What I try to suggest is only keeping the first line (line 582, return fopen(filename, mode);) and deleting the next 4 lines that are unreachable (lines 583-586).

@keyman-server keyman-server modified the milestones: A19S28, A19S29 May 11, 2026
Markus-SWAG and others added 6 commits May 11, 2026 16:15
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
Comment on lines +139 to +142
if (test_display) {
gdk_display_close(test_display);
test_display = nullptr;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From devin.ai:

gdk_display_close in TearDown breaks subsequent parameterized test instances

The KeyboardConversionTest fixture calls gdk_display_close(test_display) in TearDown() (keymap.tests.cpp:140). This fixture is used with testing::WithParamInterface, meaning GTest creates a new fixture instance per parameter value, calling SetUp/TearDown for each. However, gdk_init() in SetUp (keymap.tests.cpp:89) is a one-time initialization — subsequent calls are no-ops and will not reopen the display. After the first test's TearDown closes the display, all subsequent test instances will call gdk_display_get_default() and get a closed display. The GDK documentation explicitly states gdk_display_close "is not a reversible operation" and "should not be called normally." This could cause the ASSERT_NE(test_display, nullptr) or ASSERT_NE(test_keymap, nullptr) checks to fail for every test after the first, or cause undefined behavior when using the keymap from a closed display.

Comment on lines +10 to +11
include_directories: [common_include_dir],
include_directories: [common_include_dir],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
include_directories: [common_include_dir],
include_directories: [common_include_dir],
include_directories: [common_include_dir],

Comment on lines +78 to +88
if (KMX_DoConvert(kmxfile, bDeadkeyConversion, argc, (gchar**)argv)) {
if(!KMX_SaveKeyboard(kmxfile, outfile)) {
KMX_LogError(L"Failed to save keyboard (%d)\n", errno);
delete[] kmxfile;
return 3;
}
}

delete[] kmxfile;

return 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From devin.ai:

Program returns success (0) when KMX_DoConvert fails — pre-existing issue

In main.cpp:78-86, when KMX_DoConvert returns FALSE, the code falls through to delete[] kmxfile; return 0;, returning success even though conversion failed. This is the same behavior as the old code in mcompile.cpp and is consistent with the comment at mcompile.cpp:8-9 that the program 'deliberately leaks memory as it has a very short life cycle.' However, a non-zero exit code would be more appropriate for a failed conversion.

* Mnemonic layout support for Linux
*/

#include "mcompile.h"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From devin.ai:

main.cpp relies on transitive includes for strcmp, printf, and errno

main.cpp uses strcmp (line 24), printf (lines 29, 44), and errno (lines 73, 80) but only directly includes mcompile.h. These symbols are available through transitive includes: mcompile.h<gdk/gdk.h><glib.h> → various C standard headers. This works but is fragile — a change in GDK's internal includes could break compilation. The old code had the same issue. Consider adding explicit #include <cstring>, #include <cstdio>, and #include <cerrno>.

}
}

delete[] kmxfile;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From devin.ai - took me a while to understand this, but I think the analysis is correct. And I guess the other two delete[] lines will have to be changed as well:

delete[] on LPKMX_KEYBOARD is technically UB but an improvement over the old delete

In main.cpp:74,81,86, delete[] kmxfile is used where kmxfile is LPKMX_KEYBOARD (i.e., KMX_KEYBOARD*), but the underlying memory was allocated as new KMX_BYTE[size] in KMX_LoadKeyboard (mc_kmxfile.cpp:439-441). Using delete[] with a different type than new[] is technically undefined behavior per the C++ standard. However, both KMX_BYTE and KMX_KEYBOARD have trivial destructors, so in practice this is benign. The old code used delete kmxfile (scalar delete on array allocation), which was even more wrong. The PR improves this, though the correct fix would be to cast back to PKMX_BYTE before deleting: delete[] reinterpret_cast<PKMX_BYTE>(kmxfile);.

Suggested change
delete[] kmxfile;
delete[] reinterpret_cast<PKMX_BYTE>(kmxfile);

}
}

delete[] kmxfile;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider a further refactoring (in a separate PR):
add a field (buf) to the end of KMX_KEYBOARD that holds the original data. That would be cleaner. Then we could do delete kmxfile. The d'tor of KMX_KEYBOARD could then delete[] buf.

Also, the variable name kmxfile should probably be kmxkeyboard.

@ermshiperete ermshiperete self-requested a review May 12, 2026 14:30
Copy link
Copy Markdown
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always more...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

linux/ test Any acceptance test issue

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

5 participants