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

consider reverting #27 #28

Closed
yerden opened this issue Mar 20, 2018 · 5 comments
Closed

consider reverting #27 #28

yerden opened this issue Mar 20, 2018 · 5 comments

Comments

@yerden
Copy link
Contributor

yerden commented Mar 20, 2018

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
Copy link
Contributor

marfer commented Mar 20, 2018

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
Copy link
Contributor Author

yerden commented Mar 20, 2018

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
Copy link
Contributor

marfer commented Mar 20, 2018

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 😄

@yerden
Copy link
Contributor Author

yerden commented Mar 20, 2018

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.

@yerden yerden closed this as completed Mar 20, 2018
@dingmaotu
Copy link
Owner

@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.

@dingmaotu dingmaotu reopened this Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants