Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core,developer): use NDEBUG flag to disable assertions in release build #12715

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions common/include/test_assert.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
#include <string>
#include "test_color.h"

#ifdef _assert_failed
#undef _assert_failed
#ifdef _test_assert_failed
#undef _test_assert_failed
#endif
#define _assert_failed(result, exprText) { \
#define _test_assert_failed(result, exprText) { \
std::wcerr << console_color::fg(console_color::BRIGHT_RED) \
<< "Test failed with " << (result) \
<< " at " << __FILE__ << ":" << __LINE__ << ":" \
Expand All @@ -31,23 +31,23 @@
#define try_status(expr) { \
auto __s = (expr); \
if (__s != KM_CORE_STATUS_OK) { \
_assert_failed(__s, u ## #expr); \
_test_assert_failed(__s, u ## #expr); \
} \
}

#ifdef assert
#undef assert
#ifdef test_assert
#undef test_assert
#endif
#define assert(expr) { \
#define test_assert(expr) { \
if (!(expr)) { \
_assert_failed(0, u ## #expr); \
_test_assert_failed(0, u ## #expr); \
} \
}

#ifdef assert_equal
#undef assert_equal
#ifdef test_assert_equal
#undef test_assert_equal
#endif
#define assert_equal(actual, expected) { \
#define test_assert_equal(actual, expected) { \
if ((actual) != (expected)) { \
std::wcerr << console_color::fg(console_color::BRIGHT_RED) \
<< "Test failed at " << __FILE__ << ":" << __LINE__ << ":" \
Expand All @@ -59,10 +59,10 @@
} \
}

#ifdef assert_string_equal
#undef assert_string_equal
#ifdef test_assert_string_equal
#undef test_assert_string_equal
#endif
#define assert_string_equal(actual, expected) { \
#define test_assert_string_equal(actual, expected) { \
if (u16cmp((actual), (expected)) != 0) { \
std::wcerr << console_color::fg(console_color::BRIGHT_RED) \
<< "Test failed at " << __FILE__ << ":" << __LINE__ << ":" \
Expand Down
8 changes: 6 additions & 2 deletions common/include/test_color.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@

#include <ostream>

#ifdef _MSC_VER
#include <io.h>
#else
#include <unistd.h>
#endif

namespace console_color {

enum ansi_code {
Expand Down Expand Up @@ -65,12 +71,10 @@ __define_ansi_code__(reversed, "7");
#undef __define_ansi_code__

#ifdef _MSC_VER
#include <io.h>
inline bool isaterminal() {
return _isatty(_fileno(stdout));
}
#else
#include <unistd.h>
inline bool isaterminal() {
return isatty(STDOUT_FILENO);
}
Expand Down
8 changes: 7 additions & 1 deletion common/windows/delphi/general/KeymanPaths.pas
Original file line number Diff line number Diff line change
Expand Up @@ -436,12 +436,18 @@ class function TKeymanPaths.RunningFromSource(var keyman_root: string): Boolean;
class function TKeymanPaths.KeymanCoreLibraryPath(const Filename: string): string;
var
keyman_root: string;
configuration: string;
begin
// Look up KEYMAN_ROOT development variable -- if found and executable
// within that path then use that as source path
if TKeymanPaths.RunningFromSource(keyman_root) then
begin
Exit(keyman_root + 'core\build\x86\debug\src\' + Filename);
{$IFDEF DEBUG}
configuration := 'debug';
{$ELSE}
configuration := 'release';
{$ENDIF}
Exit(keyman_root + 'core\build\x86\'+configuration+'\src\' + Filename);
Comment on lines +445 to +450
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows us to test both release and debug builds of Core in the Debugger when running within the keymanapp/keyman repo

end;

Result := GetDebugPath('KeymanCoreLibraryPath', '');
Expand Down
4 changes: 4 additions & 0 deletions core/src/action.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ bool km::core::action_item_list_to_actions_object(
if(output.empty()) {
actions->code_points_to_delete++;
} else {
#ifndef NDEBUG
auto last_context_item = output.back();
#endif
output.pop_back();
assert(last_context_item.type == KM_CORE_CT_CHAR);
assert(last_context_item.character == action_items->backspace.expected_value);
Expand All @@ -71,7 +73,9 @@ bool km::core::action_item_list_to_actions_object(
if(output.empty()) {
// deleting a marker has no effect on the application
} else {
#ifndef NDEBUG
auto last_context_item = output.back();
#endif
output.pop_back();
assert(last_context_item.type == KM_CORE_CT_MARKER);
assert(last_context_item.marker == action_items->backspace.expected_value);
Expand Down
6 changes: 6 additions & 0 deletions core/src/ldml/ldml_markers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,19 @@ void add_back_markers(std::u32string &str, const std::u32string &src, marker_map
str.clear();
// iterator over the marker map
auto marki = map2.rbegin();
#ifndef NDEBUG
// number of markers left to processnfd
size_t max_markers = count_markers(map);
size_t processed_markers = 0;
#endif

// add any end-of-text markers
while(marki != map2.rend() && marki->ch == MARKER_BEFORE_EOT) {
if (!marki->end) {
prepend_marker(str, marki->marker, encoding);
#ifndef NDEBUG
processed_markers++;
#endif
}
marki->processed = true; // mark as done
marki++;
Expand All @@ -88,7 +92,9 @@ void add_back_markers(std::u32string &str, const std::u32string &src, marker_map
break;
} else {
prepend_marker(str, i->marker, encoding);
#ifndef NDEBUG
processed_markers++;
#endif
mcdurdin marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions core/src/ldml/ldml_processor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,10 @@ ldml_event_state::remove_text(std::u32string &str, size_t length) {
/** track how many context items have been removed, via push_backspace() */
size_t contextRemoved = 0;
for (auto c = state->context().rbegin(); length > 0 && c != state->context().rend(); c++, contextRemoved++) {
#ifndef NDEBUG
/** last char of context */
km_core_usv lastCtx = str.back();
#endif
uint8_t type = c->type;
assert(type == KM_CORE_BT_CHAR || type == KM_CORE_BT_MARKER);
if (type == KM_CORE_BT_CHAR) {
Expand Down
17 changes: 10 additions & 7 deletions core/tests/kmx_test_source/kmx_test_source.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@

#include "kmx_test_source.hpp"

#include <test_assert.h>
#include <test_color.h>

namespace km {
namespace tests {

Expand Down Expand Up @@ -51,15 +54,15 @@ KmxTestSource::parse_source_string(std::string const &s) {
if (*p == '\\') {
p++;
km_core_usv v;
assert(p != s.end());
test_assert(p != s.end());
if (*p == 'u' || *p == 'U') {
// Unicode value
p++;
size_t n;
std::string s1 = s.substr(p - s.begin(), 8);
v = std::stoul(s1, &n, 16);
// Allow deadkey_number (U+0001) characters and onward
assert(v >= 0x0001 && v <= 0x10FFFF);
test_assert(v >= 0x0001 && v <= 0x10FFFF);
p += n - 1;
if (v < 0x10000) {
t += km_core_cu(v);
Expand All @@ -70,7 +73,7 @@ KmxTestSource::parse_source_string(std::string const &s) {
} else if (*p == 'd') {
// Deadkey
// TODO, not yet supported
assert(false);
test_assert(false);
}
} else {
t += *p;
Expand Down Expand Up @@ -212,7 +215,7 @@ KmxTestSource::get_keyboard_options(kmx_options options) {

key_event
KmxTestSource::char_to_event(char ch) {
assert(ch >= 32);
test_assert(ch >= 32);
return {
km::core::kmx::s_char_to_vkey[(int)ch - 32].vk,
(uint16_t)(km::core::kmx::s_char_to_vkey[(int)ch - 32].shifted ? KM_CORE_MODIFIER_SHIFT : 0)};
Expand Down Expand Up @@ -258,8 +261,8 @@ KmxTestSource::vkey_to_event(std::string const &vk_event) {
}

// The string should be empty at this point
assert(!std::getline(f, s, ' '));
assert(vk != 0);
test_assert(!std::getline(f, s, ' '));
test_assert(vk != 0);

return {vk, modifier_state};
}
Expand All @@ -276,7 +279,7 @@ KmxTestSource::next_key(std::string &keys) {
return char_to_event(ch);
}
auto n = keys.find(']');
assert(n != std::string::npos);
test_assert(n != std::string::npos);
auto vkey = keys.substr(1, n - 1);
keys.erase(0, n + 1);
return vkey_to_event(vkey);
Expand Down
2 changes: 1 addition & 1 deletion core/tests/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#

# Note: this version of cmpfiles ignores line endings, which is better for platform independence
cmpfiles = ['-c', 'import sys; a = open(sys.argv[1], \'r\').read(); b = open(sys.argv[2], \'r\').read(); exit(not (a==b))']
cmpfiles = ['-c', 'import sys; a = open(sys.argv[1], \'r\').read(); b = open(sys.argv[2], \'r\').read(); sys.exit(not (a==b))']
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated ... but maybe due to my python version? In any case sys.exit is probably safer

stnds = join_paths(meson.current_source_dir(), 'standards')

libsrc = include_directories(
Expand Down
6 changes: 3 additions & 3 deletions core/tests/unit/emscripten_filesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

#include <emscripten.h>
#include <iostream>
#include <cassert>
#include <test_assert.h>

const std::string get_wasm_file_path(const std::string& filename) {
// Verify that we are passing a fully-qualified path
Expand All @@ -13,13 +13,13 @@ const std::string get_wasm_file_path(const std::string& filename) {
std::cout << "get_wasm_file_path ENTER (" << filename << ")" << std::endl;
#endif

assert(
test_assert(
(filename.length() > 0 && filename.at(0) == '/') ||
(filename.length() > 1 && filename.at(1) == ':')
);

#if _DEBUG_FOPEN
std::cout << "get_wasm_file_path assert passed " << std::endl;
std::cout << "get_wasm_file_path test_assert passed " << std::endl;
#endif

EM_ASM_({
Expand Down
Loading