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

Remove the 'Mandatory WML Child missing but untested for' error #3672

Open
wants to merge 1 commit into
base: master
from

Conversation

@CelticMinstrel
Copy link
Member

commented Oct 28, 2018

Pushing for preliminary review, not quite done yet. Basically I'd like everyone to look it over and see if I need to change how the missing child is handled in some of the cases (particularly those labelled by TODO).

This addresses #3647.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

hmm not sure what to think about this, on the one hand it clearly does make the code easier to understnd if there are differnt types for 'config' and 'optional config', on the other hand while forgetting the result of child() was previously 'just' a 'Mandatory WML Child ..' error it is now a segfault/nullptr dereference which is actually worse than that undescriptive error. Also all those (*cfg)["attr"] are not exactly pretty.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

Also all those (*cfg)["attr"] are not exactly pretty.

Yes indeed, I was thinking that too. That's why I'm planning to add a function so I can write cfg->at("attr") instead.

on the other hand while forgetting the result of child() was previously 'just' a 'Mandatory WML Child ..' error it is now a segfault/nullptr dereference which is actually worse than that undescriptive error.

I'm not so convinced that it's worse. It's bad, yes, but the "Mandatory WML Child..." error is also quite bad. This forces us to raise the errors earlier, when we have more information about what has gone wrong.

Of course we do need to ensure we don't dereference a null pointer. However, most of the uses of .child() were already within an if() statement, so I don't think this will end up being a big problem. Still, that's why I've opened it for review instead of just pushing it, because ideally we'd have ten pairs of eyes looking at all the places where .child() or .find_child() was previously called.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

maybe it'd be better if that function returned a custom pointer type instead of just config* , that'd have the advantage that we could easily add new features to it in particular nullptr checks and other debugging tools.

...

hmm yes I was not really worried that this patch induces crashes on it's own, after all it forced is somehow to take a took all all current usecases, more about future code in that past people did accidently write code like child("tag")["atten"], and when they now make the similar mistake of child("tag")->attr("atten") it might cause crashes.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2018

I'll think about your idea of adding a new type of "smart pointer", but if possible I'd like to see people review all the code changes I've made before that, to see if anything sticks out as being wrong. And maybe comment on some of my added TODO comments if they have an idea of what should be done there. If I add a config pointer it would likely be as a second, separate commit.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Nov 10, 2018

@gfgtdf - What about using boost::optional<config&>? Does that help at all to alleviate your concerns? Or do you require a true custom class of some kind?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2018

my idea was basicially to be able to check whether there exists a (c++) code that uses find/get_child functiosn without checing the result first by writing

//config.cpp
//todo handle const correctly
#ifdef DEBUG_CONFIG

class config_ptr
{
	config_ptr(config* cfg)
		: ptr_(cfg), checked_(false)
	{
	
	}
	explicit operator bool() { checked_ = true; return ptr_ != nullptr}
	config* operator*() const{ if(!checked_) { throw error("conifg_ptr used before checked"); return ptr_ }
	...
private:
	config* ptr_;
	mutable bool checked_;
}
#else
//std::observer_ptr<config> is basicially 'config*' as a class.
class config_ptr : public std::observer_ptr<config>
{
	config_ptr(config* cfg) : std::observer_ptr(cfg) {}
}
#endif

then there would be of course be two get_child functions: one like get_optional_child that returns a config_ptr and one .child() function that returns a config& and throws when when no sihc child exists.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2018

Your config_ptr doesn't really solve the issue as far as I can tell - it has the exact same problem that the existing implementation does. Can you explain how it would actually improve the situation?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2018

Your config_ptr doesn't really solve the issue as far as I can tell - it has the exact same problem that the existing implementation does

It working quite differntly, it does not give an error when an invalid config is accecced, it gives an error when a config is accecced without being checked first, whether it is actually valid or not. As i said before it s a debuggin feature to detect unchecked config acccess.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2018

But the error still occurs at the same point - when accessing the config, not when fetching it. The point of this PR is to remove this possibility in order to force coders to check and print an error message, rather than deferring the error to a point where it's too late to add any useful information.

Another possibility would be returning an iterator, though that might make it a touch harder to do the check since you need to compare it with .end() rather than just converting it to bool.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2018

But the error still occurs at the same point - when accessing the config, not when fetching it

just like in the config* case,.

The point of this PR is to remove this possibility in order to force coders to check and print an error message, rather than deferring the error to a point where it's too late to add any useful information

Yes and this is exactly the purpose of the custom ptr class aswell: to detect such errors easiler and not only error when in the bad case (the child was not found) but happens, but always when there is no check for it even when the tag is actualyl there.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2018

That still doesn't solve the underlying problem though, which is that they should be checking if the tag exists before fetching it. The error should be printed when the tag is fetched, not when it's accessed.

Maybe I should remove the "get optional child" function altogether and require that you explicitly check with has_child first if you want it to be optional? That doesn't work too well in the case of find_child mind you, but that's far less frequently used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.