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

Breakup load into separate functions #360

Closed
wants to merge 1 commit into from

Conversation

jamesgol
Copy link

This splits off the actual loading of the JSON file and parsing into a new function. The idea is that eventually I'd like to make modifying the configuration file available to the code base.

I'm not sure what the best method for exposing the configuration is, but this is a start.

This splits off the actual loading of the JSON file and parsing into a new function. The idea is that eventually I'd
like to make modifying the configuration file available to the code base.
Copy link
Member

@euphi euphi left a comment

Choose a reason for hiding this comment

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

I'm quite sure that this patch corrupts memory.

@@ -49,6 +47,17 @@ bool Config::load() {
JsonObject& parsedJson = jsonBuffer.parseObject(buf);
Copy link
Member

Choose a reason for hiding this comment

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

jsonBuffer is a local variable, so you create a reference to something internal to this local variable.
The local variable gets destroyed when leaving Config::load(), this (hopefully ;-) frees the buffer of parsedJson.

bool Config::load() {
_valid = false;
JsonObject& parsedJson = Config::loadJSONObject();
if (!parsedJson.success()) {
Copy link
Member

Choose a reason for hiding this comment

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

parsedJson is freed memory, so behaviour is undefined. (It will work in most cases anyhow, because no one else has overwritten the memory region yet... but technically this is just good luck....)

Copy link
Author

Choose a reason for hiding this comment

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

It's been a decade or two since I've done any real C++ coding. Thanks for the sanity check.

@timpur
Copy link
Contributor

timpur commented Dec 29, 2017

This has been done in #458 differently and in a safer manner (event though my c++ is basic ... lots of googling) , thus im closing, if you have any comments please add :)

@timpur timpur closed this Dec 29, 2017
@jamesgol
Copy link
Author

Excellent. I'll have to make some time to test it out.

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

Successfully merging this pull request may close these issues.

3 participants