consider reverting #27 - Githubissues

dingmaotu / mql4-lib

MQL4/5 Foundation Library For Professional Developers

Apache License 2.0

544 stars 251 forks source link

consider reverting #27 #28

Closed yerden closed 6 years ago

yerden commented 6 years ago

Issues with pull request #27:

build is broken since Nil constant is unknown;
if NULL constant was on @marfer's mind then the introduced check is redundant. SafeDelete() checks the pointer by CheckPointer() and does nothing with NULL pointer.

@dingmaotu: please check the build is ok before accepting pulls.

marfer commented 6 years ago

Hi, Nil constant is defined in RespBytes.mqh, it's a singleton that are used to encode NULLs to RESP format. If you clear RespArray it also delete this NULLs encoder through SafeDelete(), and on next time, we got Bad Pointer crash in MT

yerden commented 6 years ago

Oh, I see, my bad.

Then, I guess, we have a dependency issue here. RespArray.mqh doesn't #include RespBytes.mqh. Everybody who uses RespArray should include RespBytes.mqh from now on.

Or we can include it right in RespArray.mqh?

marfer commented 6 years ago

Exactly, probably need to include RespBytes.mqh to RespArray.mqh or change include order in Resp.mqh or move Nil definition to RespArray.mqh. I'm not so strong in mql, and any advise will be helpful from you guys. Sorry that I've broke the build :smile:

yerden commented 6 years ago

As stated in README: "To use the RESP protocol, you need to include Mql/Format/Resp.mqh". Looks like now the build is ok, it was my #include mistake.

dingmaotu commented 6 years ago

@yerden @marfer Thanks for discussing the issue and it is necessary to clarify things and expose problems. Though including Resp.mqh directly does not break the build but I think it is a good practice to include RespBytes.mqh in RespArray.mqh as the Nil entity is referenced. I will make the change.