C++/WASM support for EA31337 code#792
Open
nseam wants to merge 3 commits into
Open
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (47)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note
|
Reviewer's GuideIntroduces C++/WASM-compatible time handling and null/string semantics across the EA31337 codebase, routing time queries through PlatformTime and tick indicators, updating various utilities and bindings to use NULL_STRING and EMSCRIPTEN guards, and adding inline PlatformTime implementations plus WASM-specific filesystem and serialization fixes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 8 issues, and left some high level feedback:
- In
BarOHLC::operator==the comparisonopen == (double)_r.timelooks like a copy‑paste error and should likely beopen == _r.open, otherwise equality checks on bars will behave incorrectly. - The
_NULL_VALUEclass inStd.hnow has conflicting/overlapping APIs (anoperator std::string, a templatedoperator T, twoas<T>specializations plus a generictemplate <typename T> T as() const { return 0; }) and is also both defined and declaredextern; this is prone to ambiguity and ODR issues and should be simplified so there is a single, consistent implementation and a single definition. PlatformTime::current_tick_indicatoris defined inPlatformTime.inline.h, which is included as a header; this will emit a non‑inline static definition in every translation unit that includes it—consider marking itinlineor moving the definition to a single .cpp to avoid ODR violations, and review the heavyPrint/DebugBreakusage in these inline functions to avoid noisy logging in normal flows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `BarOHLC::operator==` the comparison `open == (double)_r.time` looks like a copy‑paste error and should likely be `open == _r.open`, otherwise equality checks on bars will behave incorrectly.
- The `_NULL_VALUE` class in `Std.h` now has conflicting/overlapping APIs (an `operator std::string`, a templated `operator T`, two `as<T>` specializations plus a generic `template <typename T> T as() const { return 0; }`) and is also both defined and declared `extern`; this is prone to ambiguity and ODR issues and should be simplified so there is a single, consistent implementation and a single definition.
- `PlatformTime::current_tick_indicator` is defined in `PlatformTime.inline.h`, which is included as a header; this will emit a non‑inline static definition in every translation unit that includes it—consider marking it `inline` or moving the definition to a single .cpp to avoid ODR violations, and review the heavy `Print`/`DebugBreak` usage in these inline functions to avoid noisy logging in normal flows.
## Individual Comments
### Comment 1
<location path="Std.h" line_range="417-426" />
<code_context>
// Converter of NULL_VALUE into expected type. e.g., "int x = NULL_VALUE" will end up with "x = 0".
-struct _NULL_VALUE {
+class _NULL_VALUE {
+ public:
+
+ // Explicit conversion operator for string
+ operator std::string() const {
+ return _empty_string;
+ }
+
template <typename T>
operator T() const {
return std::numeric_limits<T>::max();
}
+
+ // Helper method to get value as type T - needed for template contexts
+ template <typename T>
+ typename std::enable_if<!std::is_same<T, string>::value, T>::type as() const {
+ return std::numeric_limits<T>::max();
+ }
+
+ template <typename T>
+ typename std::enable_if<std::is_same<T, string>::value, T>::type as() const {
+ return _empty_string;
+ }
+
+ template <typename T> T as() const {
+ return 0;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The _NULL_VALUE class has conflicting template overloads and a duplicated global definition, which is likely to break compilation and violate the one-definition rule.
The third `template <typename T> T as() const` conflicts with the two SFINAE `as` overloads (same signature) and will cause redefinition/overload ambiguity, as well as contradicting their behavior.
You also both define and declare `NULL_VALUE`:
```cpp
} NULL_VALUE;
extern _NULL_VALUE NULL_VALUE;
```
This violates the one-definition rule and can cause multiple-definition link errors or UB.
Please:
- Remove the generic `as<T>()` and rely on the SFINAE overloads, or replace all three with a single templated `as<T>()` using `if constexpr`.
- Keep only one definition of `NULL_VALUE`: an `extern` declaration in a header and a single non-`extern` definition in one translation unit (or adjust accordingly).
</issue_to_address>
### Comment 2
<location path="Platform/PlatformTime.inline.h" line_range="43" />
<code_context>
+
+// Static member definition (must appear in exactly one translation unit).
+// Since IndicatorData.h is designed to be included from a single TU in WASM builds, this is safe here.
+IndicatorBase* PlatformTime::current_tick_indicator = nullptr;
+
+inline int64 PlatformTime::TimeCurrent() {
</code_context>
<issue_to_address>
**issue (bug_risk):** Defining the static member in this header will cause multiple definition errors when included in multiple translation units.
To fix this, either:
- Mark the definition as `inline` (C++17+): `inline IndicatorBase* PlatformTime::current_tick_indicator = nullptr;`, ensuring all TUs see the same definition; or
- Move the definition to a `.cpp` file and leave only `static IndicatorBase* current_tick_indicator;` declared in the header.
Relying on a single-TU convention for WASM is fragile; the second option is safer if this code may be reused in other builds.
</issue_to_address>
### Comment 3
<location path="Platform/Chart/Bar.struct.h" line_range="247-248" />
<code_context>
string ToCSV() { return StringFormat("%d,%g,%g,%g,%g", time, open, high, low, close); }
// Operators.
bool operator==(const BarOHLC &_r) {
- return time == _r.time && open == _r.time && high == _r.high && low == _r.low && close == _r.close;
+ return time == _r.time && open == (double)_r.time && high == _r.high && low == _r.low && close == _r.close;
}
bool operator!=(const BarOHLC &_r) { return !(THIS_REF == _r); }
</code_context>
<issue_to_address>
**issue (bug_risk):** BarOHLC equality comparison uses the right-hand bar's time instead of its open value, so equality checks are incorrect.
`operator==` currently reads:
```cpp
bool operator==(const BarOHLC &_r) {
return time == _r.time && open == (double)_r.time && high == _r.high && low == _r.low && close == _r.close;
}
```
It should compare `open` fields directly, e.g.:
```cpp
return time == _r.time && open == _r.open && high == _r.high && low == _r.low && close == _r.close;
```
As written, any code using this for equality (e.g., deduplication or lookups) will behave incorrectly.
</issue_to_address>
### Comment 4
<location path="Platform/PlatformTime.inline.h" line_range="45-52" />
<code_context>
+// Since IndicatorData.h is designed to be included from a single TU in WASM builds, this is safe here.
+IndicatorBase* PlatformTime::current_tick_indicator = nullptr;
+
+inline int64 PlatformTime::TimeCurrent() {
+ if (current_tick_indicator == nullptr) {
+ Print("Error: Current tick indicator is not set. TimeCurrent() will return 0 and is unusable. You should use IndicatorTest/Platform::Tick() method to run ticks.");
+ DebugBreak();
+ return 0;
+ }
+
+ Print("Retrieving current time from current tick indicator: ", current_tick_indicator PTR_DEREF GetFullName(), " with time ", current_tick_indicator PTR_DEREF GetTimeCurrent());
+
+ return current_tick_indicator PTR_DEREF GetTimeCurrent();
</code_context>
<issue_to_address>
**suggestion (performance):** Using Print/DebugBreak in these time-accessors for non-error paths can be noisy and expensive in tight tick loops.
`TimeCurrent()` currently logs every call and uses `DebugBreak()` when `current_tick_indicator` is null. Given these are per‑tick calls and may be invoked by many indicators, this will flood logs and degrade performance, especially in backtests.
Please gate these prints behind a debug flag or remove them from non‑debug builds, and replace `DebugBreak()` in normal code paths with assertions or a dedicated error‑reporting mechanism so diagnostics remain useful without impacting runtime behavior.
Suggested implementation:
```c
#endif
#include <cassert>
// Includes the full indicator types (safe here because IndicatorData.h is already processed).
#include "../Indicator/IndicatorBase.h"
#include "../Indicator/IndicatorData.h"
```
```c
inline int64 PlatformTime::TimeCurrent() {
if (current_tick_indicator == nullptr) {
#ifndef NDEBUG
Print("Error: Current tick indicator is not set. TimeCurrent() will return 0 and is unusable. You should use IndicatorTest/Platform::Tick() method to run ticks.");
assert(false && "PlatformTime::TimeCurrent called with null current_tick_indicator");
#endif
return 0;
}
#ifndef NDEBUG
Print("Retrieving current time from current tick indicator: ", current_tick_indicator PTR_DEREF GetFullName(), " with time ", current_tick_indicator PTR_DEREF GetTimeCurrent());
#endif
return current_tick_indicator PTR_DEREF GetTimeCurrent();
}
```
</issue_to_address>
### Comment 5
<location path="Indicator/IndicatorTick.h" line_range="121-122" />
<code_context>
+ /**
+ * Updates time of the last tick. Called by EmitEntry() from the Tick indicator.
+ */
+ virtual void UpdateLastTickTimeMs(int64 _time_ms) override {
+ Print("Updating last tick time (sec): ", _time_ms / 1000, " for indicator ", this->GetFullName());
+ _last_tick_time.Update(_time_ms / 1000);
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Per-tick Print in IndicatorTick::UpdateLastTickTimeMs will severely impact performance and spam logs.
Because `UpdateLastTickTimeMs` is called for every tick, this log line will generate extremely large log volumes and noticeably slow both live and backtest runs.
Please either guard this behind a compile-time debug flag / debug-only build, or remove it once the feature is validated. If you still need detailed tracing, consider a rate-limited logger instead of unconditional per-tick printing.
Suggested implementation:
```c
/**
* Updates time of the last tick. Called by EmitEntry() from the Tick indicator.
* Logging is guarded by DEBUG_INDICATOR_TICK_LOG to avoid per-tick log spam in normal builds.
*/
virtual void UpdateLastTickTimeMs(int64 _time_ms) override {
#ifdef DEBUG_INDICATOR_TICK_LOG
Print("Updating last tick time (sec): ", _time_ms / 1000, " for indicator ", this->GetFullName());
#endif
_last_tick_time.Update(_time_ms / 1000);
}
```
1. Define `DEBUG_INDICATOR_TICK_LOG` only in debug builds or when you explicitly need detailed per-tick tracing. For example, in a central config/header used only for debugging, or via your build system’s preprocessor definitions.
2. Once the feature is fully validated, you may want to remove the guarded `Print` entirely if you no longer need per-tick tracing.
</issue_to_address>
### Comment 6
<location path="Indicator/tests/classes/IndicatorTfDummy.h" line_range="79" />
<code_context>
}
};
-#ifdef EMSCRIPTEN
+#ifdef __EMSCRIPTEN__
#include <emscripten.h>
#include <emscripten/bind.h>
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the new WASM/EMSCRIPTEN bindings and time-handling changes (PlatformTime/Indicator time plumbing).
These changes alter how time is sourced and propagated for the WASM build (e.g., PlatformTime depending on the current tick indicator, new GetTimeCurrent/UpdateLastTickTimeMs behavior, INDI_EMITTED_ENTRY_TYPE_TICK in EmitEntry), but there are no new tests validating this, particularly in the C++/WASM path.
Please add a dedicated C++/WASM test (e.g., `Indicator/tests/PlatformTime.test.cpp`) that:
- Runs a Tick provider plus one or more indicators under the WASM-aware test harness.
- Asserts that `TimeCurrent()`, `PlatformTime::TimeCurrent()`, and `IndicatorBase::GetTimeCurrent()` all return the tick time from the Tick indicator.
- Covers edge cases such as: no current tick indicator set, indicator hierarchies (time propagates from the Tick indicator), and multiple ticks with increasing timestamps to confirm DateTime/period-start behavior.
This will ensure the new WASM time plumbing matches MQL runtime expectations and catches regressions early.
</issue_to_address>
### Comment 7
<location path="Platform/PlatformTime.h" line_range="51" />
<code_context>
+class IndicatorBase;
+class IndicatorData;
+
class PlatformTime {
- static MqlDateTime current_time;
- static int64 current_timestamp_s;
</code_context>
<issue_to_address>
**issue (complexity):** Consider keeping `PlatformTime` indicator-agnostic and moving platform-specific logic back into a dedicated implementation unit to reduce coupling and indirection.
You’ve increased coupling and indirection without needing to for the new feature. You can keep all functionality (indicators providing tick time) and simplify by pushing indicator‑specific logic out of `PlatformTime` and back into the indicator layer.
### 1. Remove indicator coupling from `PlatformTime`
Instead of storing an `IndicatorBase*` and accepting `IndicatorData*`, just expose primitive‑based APIs. Let indicators call these APIs, rather than `PlatformTime` knowing about indicators.
```cpp
// PlatformTime.h
class PlatformTime {
static MqlDateTime current_time;
static int64 current_timestamp_s;
static int64 current_timestamp_ms;
public:
static int64 TimeCurrent() { return current_timestamp_s; }
static int64 CurrentTimestampMs(){ return current_timestamp_ms; }
static MqlDateTime CurrentTime(){ return current_time; }
// Called by the indicator layer when it has a new tick time (ms since epoch).
static void UpdateLastTickTimeMs(int64 tick_time_ms);
};
```
Then in the indicator code (where you *do* know about `IndicatorData`):
```cpp
// Indicator code (example)
void OnTick(IndicatorData* indi) {
// however you derive ms since epoch from the indicator:
int64 tick_ms = indi->GetTickTimeMs();
PlatformTime::UpdateLastTickTimeMs(tick_ms);
}
```
This removes:
- `IndicatorBase* current_tick_indicator`
- `SetCurrentIndicator(IndicatorData* _indi)`
- `UpdateLastTickTimeMs(int64 tick_time_ms, IndicatorData* _source_indi)`
and the forward declarations `class IndicatorBase; class IndicatorData;` from `PlatformTime.h`, while preserving “tick time comes from indicators”.
### 2. Keep platform logic in one TU (avoid special inline header)
You can move the non‑MQL implementation back into a `.cpp` (or a single TU guarded by `#ifndef __MQL__`) to avoid the extra inline header indirection:
```cpp
// PlatformTime.cpp (non-MQL build)
#include "PlatformTime.h"
#include <chrono>
#include <ctime>
MqlDateTime PlatformTime::current_time = {0};
int64 PlatformTime::current_timestamp_s = 0;
int64 PlatformTime::current_timestamp_ms = 0;
void PlatformTime::UpdateLastTickTimeMs(int64 tick_time_ms) {
using namespace std::chrono;
current_timestamp_ms = tick_time_ms;
current_timestamp_s = current_timestamp_ms / 1000;
std::time_t t = current_timestamp_s;
std::tm* now = std::localtime(&t);
current_time.day = now->tm_mday;
current_time.day_of_week = now->tm_wday;
current_time.day_of_year = now->tm_yday;
current_time.hour = now->tm_hour;
current_time.min = now->tm_min;
current_time.mon = now->tm_mon;
current_time.sec = now->tm_sec;
current_time.year = now->tm_year;
}
```
For MQL, you can keep a thin wrapper that calls `UpdateLastTickTimeMs` with tick time derived from `TimeCurrent()` / `GetTickCount()`:
```cpp
// PlatformTime.mqh (MQL build, if needed)
class PlatformTime {
// ... same static members ...
public:
static void Tick() {
// derive tick_time_ms using MQL APIs
int64 tick_time_ms = /* TimeCurrent/GetTickCount combo */;
UpdateLastTickTimeMs(tick_time_ms);
}
};
```
This keeps `PlatformTime`:
- Indicator‑agnostic.
- Free of circular includes and forward declarations.
- Still capable of using indicator‑provided tick time via a simple, primitive API.
</issue_to_address>
### Comment 8
<location path="Indicator/IndicatorData.h" line_range="895" />
<code_context>
/**
* Returns number of points per pip.
*/
</code_context>
<issue_to_address>
**issue (complexity):** Consider letting the tick indicator store and update the current tick time directly instead of delegating through GetTick() and PlatformTime, simplifying ownership and data flow.
You can simplify this without changing behavior by making the tick indicator itself the clear owner of the “current tick time” and removing the delegation chain through `GetTick()`.
Right now:
- `IndicatorData::GetTimeCurrent()` → `GetTick()->GetTimeCurrent()`
- `IndicatorData::UpdateLastTickTimeMs()` → `GetTick()->UpdateLastTickTimeMs(...)`
- `EmitEntry()` → `GetTick()->UpdateLastTickTimeMs(_entry.timestamp * 1000)`
Combined with `PlatformTime` calling back into the indicator, this creates the circular indirection the other reviewer highlighted.
### 1. Store the tick time directly in `IndicatorData`
Give `IndicatorData` (specifically the tick indicator instance) a simple field to own the last tick time and implement the two methods directly, instead of delegating to `GetTick()`:
```cpp
// in IndicatorData (private or protected)
int64 last_tick_time_ms = 0;
// ...
// Returns time of the current tick.
datetime GetTimeCurrent() override {
// If you need datetime in seconds:
return (datetime)(last_tick_time_ms / 1000);
}
// Updates time of the last tick (ms).
void UpdateLastTickTimeMs(int64 _time_ms) override {
last_tick_time_ms = _time_ms;
}
```
This removes the back-and-forth between different indicators for a value that is conceptually owned by the tick indicator.
### 2. Update `EmitEntry` to use the local setter
When emitting a tick entry, you can update the time via the local API, not via `GetTick()`:
```cpp
void EmitEntry(IndicatorDataEntry& _entry,
ENUM_INDI_EMITTED_ENTRY_TYPE _type = INDI_EMITTED_ENTRY_TYPE_PARENT) {
if (_type == INDI_EMITTED_ENTRY_TYPE_TICK) {
// Tick indicator updates its own tick time
UpdateLastTickTimeMs(_entry.timestamp * 1000);
}
for (int i = 0; i < ArraySize(listeners); ++i) {
if (listeners[i].ObjectExists()) {
listeners[i].Ptr() PTR_DEREF OnDataSourceEntry(_entry, _type);
}
}
}
```
If you still need a top-level “tick indicator in the hierarchy” abstraction, `GetTick()` can remain, but it no longer needs to be involved in the time plumbing.
### 3. Simplify `PlatformTime` usage (unidirectional flow)
With the above, `PlatformTime` can work with a primitive and avoid knowing about the indicator hierarchy:
- When a tick is emitted, push the time into `PlatformTime` directly:
```cpp
// wherever you integrate with PlatformTime:
if (_type == INDI_EMITTED_ENTRY_TYPE_TICK) {
const int64 tick_time_ms = _entry.timestamp * 1000;
UpdateLastTickTimeMs(tick_time_ms); // indicator field
PlatformTime::UpdateLastTickTimeMs(tick_time_ms); // platform field
}
```
- `PlatformTime` no longer needs to call back into the indicator to fetch/set `tick_time_ms`; it just stores the primitive. That allows you to:
* remove the need for `PlatformTime` to know about `IndicatorData` internals
* likely remove or at least reduce the need to include `PlatformTime.inline.h` at the end of `IndicatorData` to resolve circular dependencies
If you still need inline definitions, you can keep `PlatformTime` APIs purely in terms of primitive types:
```cpp
// PlatformTime.inline.h
inline void PlatformTime::UpdateLastTickTimeMs(int64 tick_time_ms) {
last_tick_time_ms = tick_time_ms;
}
inline datetime PlatformTime::TimeCurrent() const {
return (datetime)(last_tick_time_ms / 1000);
}
```
This keeps all functionality (tick indicators still drive both indicator hierarchy and `PlatformTime`), but the flow is now unidirectional and the coupling/indirection are significantly reduced.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+417
to
426
| class _NULL_VALUE { | ||
| public: | ||
|
|
||
| // Explicit conversion operator for string | ||
| operator std::string() const { | ||
| return _empty_string; | ||
| } | ||
|
|
||
| template <typename T> | ||
| operator T() const { |
There was a problem hiding this comment.
issue (bug_risk): The _NULL_VALUE class has conflicting template overloads and a duplicated global definition, which is likely to break compilation and violate the one-definition rule.
The third template <typename T> T as() const conflicts with the two SFINAE as overloads (same signature) and will cause redefinition/overload ambiguity, as well as contradicting their behavior.
You also both define and declare NULL_VALUE:
} NULL_VALUE;
extern _NULL_VALUE NULL_VALUE;This violates the one-definition rule and can cause multiple-definition link errors or UB.
Please:
- Remove the generic
as<T>()and rely on the SFINAE overloads, or replace all three with a single templatedas<T>()usingif constexpr. - Keep only one definition of
NULL_VALUE: anexterndeclaration in a header and a single non-externdefinition in one translation unit (or adjust accordingly).
|
|
||
| // Static member definition (must appear in exactly one translation unit). | ||
| // Since IndicatorData.h is designed to be included from a single TU in WASM builds, this is safe here. | ||
| IndicatorBase* PlatformTime::current_tick_indicator = nullptr; |
There was a problem hiding this comment.
issue (bug_risk): Defining the static member in this header will cause multiple definition errors when included in multiple translation units.
To fix this, either:
- Mark the definition as
inline(C++17+):inline IndicatorBase* PlatformTime::current_tick_indicator = nullptr;, ensuring all TUs see the same definition; or - Move the definition to a
.cppfile and leave onlystatic IndicatorBase* current_tick_indicator;declared in the header.
Relying on a single-TU convention for WASM is fragile; the second option is safer if this code may be reused in other builds.
Comment on lines
247
to
+248
| bool operator==(const BarOHLC &_r) { | ||
| return time == _r.time && open == _r.time && high == _r.high && low == _r.low && close == _r.close; | ||
| return time == _r.time && open == (double)_r.time && high == _r.high && low == _r.low && close == _r.close; |
There was a problem hiding this comment.
issue (bug_risk): BarOHLC equality comparison uses the right-hand bar's time instead of its open value, so equality checks are incorrect.
operator== currently reads:
bool operator==(const BarOHLC &_r) {
return time == _r.time && open == (double)_r.time && high == _r.high && low == _r.low && close == _r.close;
}It should compare open fields directly, e.g.:
return time == _r.time && open == _r.open && high == _r.high && low == _r.low && close == _r.close;As written, any code using this for equality (e.g., deduplication or lookups) will behave incorrectly.
Comment on lines
+45
to
+52
| inline int64 PlatformTime::TimeCurrent() { | ||
| if (current_tick_indicator == nullptr) { | ||
| Print("Error: Current tick indicator is not set. TimeCurrent() will return 0 and is unusable. You should use IndicatorTest/Platform::Tick() method to run ticks."); | ||
| DebugBreak(); | ||
| return 0; | ||
| } | ||
|
|
||
| Print("Retrieving current time from current tick indicator: ", current_tick_indicator PTR_DEREF GetFullName(), " with time ", current_tick_indicator PTR_DEREF GetTimeCurrent()); |
There was a problem hiding this comment.
suggestion (performance): Using Print/DebugBreak in these time-accessors for non-error paths can be noisy and expensive in tight tick loops.
TimeCurrent() currently logs every call and uses DebugBreak() when current_tick_indicator is null. Given these are per‑tick calls and may be invoked by many indicators, this will flood logs and degrade performance, especially in backtests.
Please gate these prints behind a debug flag or remove them from non‑debug builds, and replace DebugBreak() in normal code paths with assertions or a dedicated error‑reporting mechanism so diagnostics remain useful without impacting runtime behavior.
Suggested implementation:
#endif
#include <cassert>
// Includes the full indicator types (safe here because IndicatorData.h is already processed).
#include "../Indicator/IndicatorBase.h"
#include "../Indicator/IndicatorData.h"inline int64 PlatformTime::TimeCurrent() {
if (current_tick_indicator == nullptr) {
#ifndef NDEBUG
Print("Error: Current tick indicator is not set. TimeCurrent() will return 0 and is unusable. You should use IndicatorTest/Platform::Tick() method to run ticks.");
assert(false && "PlatformTime::TimeCurrent called with null current_tick_indicator");
#endif
return 0;
}
#ifndef NDEBUG
Print("Retrieving current time from current tick indicator: ", current_tick_indicator PTR_DEREF GetFullName(), " with time ", current_tick_indicator PTR_DEREF GetTimeCurrent());
#endif
return current_tick_indicator PTR_DEREF GetTimeCurrent();
}
Comment on lines
+121
to
+122
| virtual void UpdateLastTickTimeMs(int64 _time_ms) override { | ||
| Print("Updating last tick time (sec): ", _time_ms / 1000, " for indicator ", this->GetFullName()); |
There was a problem hiding this comment.
suggestion (performance): Per-tick Print in IndicatorTick::UpdateLastTickTimeMs will severely impact performance and spam logs.
Because UpdateLastTickTimeMs is called for every tick, this log line will generate extremely large log volumes and noticeably slow both live and backtest runs.
Please either guard this behind a compile-time debug flag / debug-only build, or remove it once the feature is validated. If you still need detailed tracing, consider a rate-limited logger instead of unconditional per-tick printing.
Suggested implementation:
/**
* Updates time of the last tick. Called by EmitEntry() from the Tick indicator.
* Logging is guarded by DEBUG_INDICATOR_TICK_LOG to avoid per-tick log spam in normal builds.
*/
virtual void UpdateLastTickTimeMs(int64 _time_ms) override {
#ifdef DEBUG_INDICATOR_TICK_LOG
Print("Updating last tick time (sec): ", _time_ms / 1000, " for indicator ", this->GetFullName());
#endif
_last_tick_time.Update(_time_ms / 1000);
}- Define
DEBUG_INDICATOR_TICK_LOGonly in debug builds or when you explicitly need detailed per-tick tracing. For example, in a central config/header used only for debugging, or via your build system’s preprocessor definitions. - Once the feature is fully validated, you may want to remove the guarded
Printentirely if you no longer need per-tick tracing.
| } | ||
| }; | ||
|
|
||
| #ifdef EMSCRIPTEN |
There was a problem hiding this comment.
suggestion (testing): Add tests for the new WASM/EMSCRIPTEN bindings and time-handling changes (PlatformTime/Indicator time plumbing).
These changes alter how time is sourced and propagated for the WASM build (e.g., PlatformTime depending on the current tick indicator, new GetTimeCurrent/UpdateLastTickTimeMs behavior, INDI_EMITTED_ENTRY_TYPE_TICK in EmitEntry), but there are no new tests validating this, particularly in the C++/WASM path.
Please add a dedicated C++/WASM test (e.g., Indicator/tests/PlatformTime.test.cpp) that:
- Runs a Tick provider plus one or more indicators under the WASM-aware test harness.
- Asserts that
TimeCurrent(),PlatformTime::TimeCurrent(), andIndicatorBase::GetTimeCurrent()all return the tick time from the Tick indicator. - Covers edge cases such as: no current tick indicator set, indicator hierarchies (time propagates from the Tick indicator), and multiple ticks with increasing timestamps to confirm DateTime/period-start behavior.
This will ensure the new WASM time plumbing matches MQL runtime expectations and catches regressions early.
| class IndicatorBase; | ||
| class IndicatorData; | ||
|
|
||
| class PlatformTime { |
There was a problem hiding this comment.
issue (complexity): Consider keeping PlatformTime indicator-agnostic and moving platform-specific logic back into a dedicated implementation unit to reduce coupling and indirection.
You’ve increased coupling and indirection without needing to for the new feature. You can keep all functionality (indicators providing tick time) and simplify by pushing indicator‑specific logic out of PlatformTime and back into the indicator layer.
1. Remove indicator coupling from PlatformTime
Instead of storing an IndicatorBase* and accepting IndicatorData*, just expose primitive‑based APIs. Let indicators call these APIs, rather than PlatformTime knowing about indicators.
// PlatformTime.h
class PlatformTime {
static MqlDateTime current_time;
static int64 current_timestamp_s;
static int64 current_timestamp_ms;
public:
static int64 TimeCurrent() { return current_timestamp_s; }
static int64 CurrentTimestampMs(){ return current_timestamp_ms; }
static MqlDateTime CurrentTime(){ return current_time; }
// Called by the indicator layer when it has a new tick time (ms since epoch).
static void UpdateLastTickTimeMs(int64 tick_time_ms);
};Then in the indicator code (where you do know about IndicatorData):
// Indicator code (example)
void OnTick(IndicatorData* indi) {
// however you derive ms since epoch from the indicator:
int64 tick_ms = indi->GetTickTimeMs();
PlatformTime::UpdateLastTickTimeMs(tick_ms);
}This removes:
IndicatorBase* current_tick_indicatorSetCurrentIndicator(IndicatorData* _indi)UpdateLastTickTimeMs(int64 tick_time_ms, IndicatorData* _source_indi)
and the forward declarations class IndicatorBase; class IndicatorData; from PlatformTime.h, while preserving “tick time comes from indicators”.
2. Keep platform logic in one TU (avoid special inline header)
You can move the non‑MQL implementation back into a .cpp (or a single TU guarded by #ifndef __MQL__) to avoid the extra inline header indirection:
// PlatformTime.cpp (non-MQL build)
#include "PlatformTime.h"
#include <chrono>
#include <ctime>
MqlDateTime PlatformTime::current_time = {0};
int64 PlatformTime::current_timestamp_s = 0;
int64 PlatformTime::current_timestamp_ms = 0;
void PlatformTime::UpdateLastTickTimeMs(int64 tick_time_ms) {
using namespace std::chrono;
current_timestamp_ms = tick_time_ms;
current_timestamp_s = current_timestamp_ms / 1000;
std::time_t t = current_timestamp_s;
std::tm* now = std::localtime(&t);
current_time.day = now->tm_mday;
current_time.day_of_week = now->tm_wday;
current_time.day_of_year = now->tm_yday;
current_time.hour = now->tm_hour;
current_time.min = now->tm_min;
current_time.mon = now->tm_mon;
current_time.sec = now->tm_sec;
current_time.year = now->tm_year;
}For MQL, you can keep a thin wrapper that calls UpdateLastTickTimeMs with tick time derived from TimeCurrent() / GetTickCount():
// PlatformTime.mqh (MQL build, if needed)
class PlatformTime {
// ... same static members ...
public:
static void Tick() {
// derive tick_time_ms using MQL APIs
int64 tick_time_ms = /* TimeCurrent/GetTickCount combo */;
UpdateLastTickTimeMs(tick_time_ms);
}
};This keeps PlatformTime:
- Indicator‑agnostic.
- Free of circular includes and forward declarations.
- Still capable of using indicator‑provided tick time via a simple, primitive API.
| HasSpecificValueStorage(INDI_DATA_VS_TYPE_VOLUME) && HasSpecificValueStorage(INDI_DATA_VS_TYPE_TICK_VOLUME); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
issue (complexity): Consider letting the tick indicator store and update the current tick time directly instead of delegating through GetTick() and PlatformTime, simplifying ownership and data flow.
You can simplify this without changing behavior by making the tick indicator itself the clear owner of the “current tick time” and removing the delegation chain through GetTick().
Right now:
IndicatorData::GetTimeCurrent()→GetTick()->GetTimeCurrent()IndicatorData::UpdateLastTickTimeMs()→GetTick()->UpdateLastTickTimeMs(...)EmitEntry()→GetTick()->UpdateLastTickTimeMs(_entry.timestamp * 1000)
Combined with PlatformTime calling back into the indicator, this creates the circular indirection the other reviewer highlighted.
1. Store the tick time directly in IndicatorData
Give IndicatorData (specifically the tick indicator instance) a simple field to own the last tick time and implement the two methods directly, instead of delegating to GetTick():
// in IndicatorData (private or protected)
int64 last_tick_time_ms = 0;
// ...
// Returns time of the current tick.
datetime GetTimeCurrent() override {
// If you need datetime in seconds:
return (datetime)(last_tick_time_ms / 1000);
}
// Updates time of the last tick (ms).
void UpdateLastTickTimeMs(int64 _time_ms) override {
last_tick_time_ms = _time_ms;
}This removes the back-and-forth between different indicators for a value that is conceptually owned by the tick indicator.
2. Update EmitEntry to use the local setter
When emitting a tick entry, you can update the time via the local API, not via GetTick():
void EmitEntry(IndicatorDataEntry& _entry,
ENUM_INDI_EMITTED_ENTRY_TYPE _type = INDI_EMITTED_ENTRY_TYPE_PARENT) {
if (_type == INDI_EMITTED_ENTRY_TYPE_TICK) {
// Tick indicator updates its own tick time
UpdateLastTickTimeMs(_entry.timestamp * 1000);
}
for (int i = 0; i < ArraySize(listeners); ++i) {
if (listeners[i].ObjectExists()) {
listeners[i].Ptr() PTR_DEREF OnDataSourceEntry(_entry, _type);
}
}
}If you still need a top-level “tick indicator in the hierarchy” abstraction, GetTick() can remain, but it no longer needs to be involved in the time plumbing.
3. Simplify PlatformTime usage (unidirectional flow)
With the above, PlatformTime can work with a primitive and avoid knowing about the indicator hierarchy:
- When a tick is emitted, push the time into
PlatformTimedirectly:
// wherever you integrate with PlatformTime:
if (_type == INDI_EMITTED_ENTRY_TYPE_TICK) {
const int64 tick_time_ms = _entry.timestamp * 1000;
UpdateLastTickTimeMs(tick_time_ms); // indicator field
PlatformTime::UpdateLastTickTimeMs(tick_time_ms); // platform field
}-
PlatformTimeno longer needs to call back into the indicator to fetch/settick_time_ms; it just stores the primitive. That allows you to:- remove the need for
PlatformTimeto know aboutIndicatorDatainternals - likely remove or at least reduce the need to include
PlatformTime.inline.hat the end ofIndicatorDatato resolve circular dependencies
- remove the need for
If you still need inline definitions, you can keep PlatformTime APIs purely in terms of primitive types:
// PlatformTime.inline.h
inline void PlatformTime::UpdateLastTickTimeMs(int64 tick_time_ms) {
last_tick_time_ms = tick_time_ms;
}
inline datetime PlatformTime::TimeCurrent() const {
return (datetime)(last_tick_time_ms / 1000);
}This keeps all functionality (tick indicators still drive both indicator hierarchy and PlatformTime), but the flow is now unidirectional and the coupling/indirection are significantly reduced.
There was a problem hiding this comment.
Pull request overview
This PR updates EA31337’s cross-platform layer to better support C++/WASM builds (Emscripten), including changes to default “null” string handling and a refactor of how “current time” is sourced during indicator ticks.
Changes:
- Standardize Emscripten build guards (
__EMSCRIPTEN__) and adjust several bindings-related sections for WASM builds. - Refactor time handling to source “current time” from the currently ticking indicator/tick-provider chain (
PlatformTime::TimeCurrent()), and propagate tick timestamps viaEmitEntry(..., INDI_EMITTED_ENTRY_TYPE_TICK). - Replace
NULL/0string returns and default params withNULL_STRING, plus add MemoryFileSystem support forFileWriteArray().
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| Trade/TradeSignalManager.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Trade/TradeSignal.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Trade/tests/TradeSignalManager.test.cpp | Adjust includes for C++ compilation test. |
| Trade.mqh | Use NULL_STRING instead of NULL for optional symbol parameter. |
| Tick/Tick.struct.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Task/TaskManager.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Task/Task.struct.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Task/Task.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Storage/ValueStorage.indicator.h | Fix indicator buffer fetch shift semantics (avoid RealShift() here). |
| Storage/MemoryFileSystem.h | Add FileWriteArray() support for byte arrays. |
| Storage/ItemsHistory.h | Replace IndicatorData.h include with forward declaration to avoid circular deps. |
| Storage/Dict/Dict.h | Adjust internal types (position unsigned) and iterator base type. |
| Storage/DateTime.static.h | Route “now” through PlatformTime::TimeCurrent() instead of old timestamp API. |
| Storage/DateTime.h | Refactor DateTime construction/update and add optional timestamp update path. |
| Storage/DateTime.extern.h | Add struct include and provide C++ TimeToStruct() implementation. |
| Storage/DateTime.entry.h | Change initialization behavior and add GetTime() helper. |
| Storage/Array.h | Replace NULL defaults with NULL_STRING / 0 in ArrayPrint. |
| Std.h | Add REF_TYPE macro and refactor NULL_VALUE conversion helpers; update Emscripten guard. |
| Serializer/SerializerJson.h | Return NULL_STRING instead of NULL for string-returning API. |
| Serializer/SerializerCsv.h | Return NULL_STRING instead of NULL on error paths. |
| Refs.struct.h | Switch Emscripten macro guard to __EMSCRIPTEN__; make operator== const. |
| Platform/Terminal.h | Return NULL_STRING instead of 0 for string-returning stub. |
| Platform/PlatformTime.inline.h | New inline definitions for PlatformTime based on tick-indicator time. |
| Platform/PlatformTime.h | Replace prior static time storage with tick-indicator-based time API; include DateTime externs. |
| Platform/Platform.h | Remove old platform-level DateTime period tracking; set current indicator for PlatformTime. |
| Platform/Platform.extern.h | Switch DateTime include to DateTime.extern.h; add missing String extern include. |
| Platform/Orders.h | Use NULL_STRING defaults for optional symbol parameters. |
| Platform/Order.struct.h | Use NULL_STRING for default symbol; apply REF_TYPE assignment in proxies. |
| Platform/Order.h | Return NULL_VALUE without casts in templated param getter. |
| Platform/Chart/Chart.struct.static.h | Use NULL_STRING defaults for optional symbol parameters. |
| Platform/Chart/Chart.enum.h | Switch Emscripten macro guard to __EMSCRIPTEN__ for bindings. |
| Platform/Chart/Bar.struct.h | Modify BarOHLC::operator== comparison logic. |
| Indicators/Tick/Indi_TickProvider.h | Emit tick entries with explicit tick type; fix Emscripten binding name/guard. |
| Indicators/Oscillator/Indi_RSI.h | Use NULL_STRING defaults; add debug break for unsupported mode; switch Emscripten guard. |
| Indicator/tests/classes/IndicatorTfDummy.h | Improve TimeToString formatting; switch Emscripten guard. |
| Indicator/IndicatorTick.h | Track last tick time via DateTime and expose tick-based GetTimeCurrent(). |
| Indicator/IndicatorTf.struct.h | Initialize ChartTf via ctor initializer. |
| Indicator/IndicatorTf.h | Ensure base-class construction happens for TF indicators. |
| Indicator/IndicatorData.h | Add time propagation hooks; fix Emscripten binding arg index; include PlatformTime.inline.h. |
| Indicator/IndicatorBase.h | Add virtual tick-time APIs; switch Emscripten guard. |
| Indicator/Indicator.h | Adjust params assignment via REF_TYPE. |
| File.mqh | Return NULL_STRING on read failure; fix FileOpen delimiter param type; use ARRAY(unsigned char, ...). |
| File.extern.h | Add FileWriteArray() wrapper for MemoryFileSystem. |
| Exchange/SymbolInfo/SymbolInfo.struct.static.h | Return NULL_STRING instead of 0 for string-returning stub. |
| Exchange/SymbolInfo/SymbolInfo.h | Use NULL_STRING default symbol in ctor. |
| Exchange/Account/AccountMt.h | Use NULL_STRING default symbol for margin check. |
| Convert.mqh | Use NULL_STRING defaults for optional symbol parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+430
to
+443
| // Helper method to get value as type T - needed for template contexts | ||
| template <typename T> | ||
| typename std::enable_if<!std::is_same<T, string>::value, T>::type as() const { | ||
| return std::numeric_limits<T>::max(); | ||
| } | ||
|
|
||
| template <typename T> | ||
| typename std::enable_if<std::is_same<T, string>::value, T>::type as() const { | ||
| return _empty_string; | ||
| } | ||
|
|
||
| template <typename T> T as() const { | ||
| return 0; | ||
| } |
Comment on lines
+41
to
42
| DateTimeEntry(bool _init_with_curr_time = true) { if (_init_with_curr_time) Set(); } | ||
| DateTimeEntry(datetime _dt) { Set(_dt); } |
Comment on lines
+218
to
+226
| void Update(int64 time = 0) { | ||
| if (time == dt_last.GetTime()) { | ||
| // No time change, no need to update. | ||
| return; | ||
| } | ||
|
|
||
| dt_last = dt_curr; | ||
| dt_curr.Set(time != 0 ? time : PlatformTime::TimeCurrent()); | ||
| } |
Comment on lines
86
to
99
| @@ -92,7 +95,7 @@ class IndicatorTick : public Indicator<TS> { | |||
| Init(); | |||
| } | |||
| IndicatorTick(string _symbol, ENUM_INDICATOR_TYPE _itype = INDI_CANDLE, int _shift = 0, string _name = "") | |||
| : Indicator<TS>(_itype, _shift, _name), history(THIS_PTR) { | |||
| : Indicator<TS>(_itype, _shift, _name), history(THIS_PTR), last_tick_time(false) { | |||
| symbol = _symbol; | |||
| // Operators. | ||
| bool operator==(const BarOHLC &_r) { | ||
| return time == _r.time && open == _r.time && high == _r.high && low == _r.low && close == _r.close; | ||
| return time == _r.time && open == (double)_r.time && high == _r.high && low == _r.low && close == _r.close; |
Comment on lines
+52
to
+54
| Print("Retrieving current time from current tick indicator: ", current_tick_indicator PTR_DEREF GetFullName(), " with time ", current_tick_indicator PTR_DEREF GetTimeCurrent()); | ||
|
|
||
| return current_tick_indicator PTR_DEREF GetTimeCurrent(); |
Comment on lines
+84
to
+99
| bool TimeToStruct(datetime dt, MqlDateTime &dt_struct) { | ||
| time_t now = (time_t)dt; | ||
|
|
||
| tm *ltm = localtime(&now); | ||
|
|
||
| dt_struct.day = ltm->tm_mday; | ||
| dt_struct.day_of_week = ltm->tm_wday; | ||
| dt_struct.day_of_year = ltm->tm_yday; | ||
| dt_struct.hour = ltm->tm_hour; | ||
| dt_struct.min = ltm->tm_min; | ||
| dt_struct.mon = ltm->tm_mon; | ||
| dt_struct.sec = ltm->tm_sec; | ||
| dt_struct.year = ltm->tm_year; | ||
|
|
||
| return true; | ||
| } |
Comment on lines
+84
to
+97
| bool TimeToStruct(datetime dt, MqlDateTime &dt_struct) { | ||
| time_t now = (time_t)dt; | ||
|
|
||
| tm *ltm = localtime(&now); | ||
|
|
||
| dt_struct.day = ltm->tm_mday; | ||
| dt_struct.day_of_week = ltm->tm_wday; | ||
| dt_struct.day_of_year = ltm->tm_yday; | ||
| dt_struct.hour = ltm->tm_hour; | ||
| dt_struct.min = ltm->tm_min; | ||
| dt_struct.mon = ltm->tm_mon; | ||
| dt_struct.sec = ltm->tm_sec; | ||
| dt_struct.year = ltm->tm_year; | ||
|
|
Comment on lines
267
to
278
| #ifndef __MQL__ | ||
|
|
||
| datetime TimeCurrent() { return PlatformTime::CurrentTimestamp(); } | ||
| datetime TimeCurrent() { return PlatformTime::TimeCurrent(); } | ||
|
|
||
| /** | ||
| * Returns current tick's time and fill the dt_struct with the corresponding values. | ||
| */ | ||
| datetime TimeCurrent(MqlDateTime &dt_struct) { | ||
| dt_struct = PlatformTime::CurrentTime(); | ||
| return PlatformTime::CurrentTimestamp(); | ||
| datetime current_time = PlatformTime::TimeCurrent(); | ||
| TimeToStruct(current_time, dt_struct); | ||
| return current_time; | ||
| } |
| static ENUM_TIMEFRAMES period; | ||
|
|
||
| // Currently ticking indicator. | ||
| static IndicatorData* indi_current; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.