Printable View
Suppose I have a list class and two of its constructors are like this:
The first one throws an exception if the string is not suitable for making a list.Code:list(const std::string& s);
list(const std::string& s, bool& b);
The second one does not throw an exception and just sets the passed boolean variable to false (after doing some cleanup).
I want to use the second one to determine if a string can be made into a list.
Is this a good design wrt to constructors and exceptions ?
O_o
Nope.
It invites a scenario in which the programmer doesn't know if a reference to the object he has references a valid object or an object that on construction "returned false" even though such a notion is impossible.
You have multiple options; many of them presenting a better design. (The ones below are certainly better options.)
You can simply rearrange the notion of construction for your object with "two stage" construction. Though I despise this method, it has a place in the real world. Instead of the constructor "returning false", you setup the constructor to initialize the object to a "ready but invalid" state, provide a second function that does the real work of constructor (such as `bool construct(string &)'), and a third function to query the validity of the object.
You completely create your object normally and doing nothing else or completely fail to create the object normally by throwing an exception, and you rearrange the nature of "determine if a string can be made into a list" by moving the implementation of that question into a function. You only need to create a function (such as `list listfromstring(string &, bool &)') which can catch the exception and take other appropriate steps.
Soma
I already did the second one.... but everything seems identical to doing it in the constructor if the 'non-throwing' constructor makes an empty list (which I'm never going to use..btw)Quote:
Originally Posted by phantomotap
Doesn't that solve the 'broken object' problem you mentioned ?
I disagree on the "two-stage" construction approach.
I suggest only having the constructor that throws an exception if the string is invalid.
If you want to provide an ability to check if a string is valid, then provide a separate non-throwing function (named something pointed like IsValid()) to perform that check. That can be a normal non-member function, or it might be a static member of the list class.
Then you have two implementation choices..... either the constructor uses the IsValid() function in deciding to throw, or the IsValid() function attempts to construct an object and, if the constructor throws, swallows the exception and returns false.
I would also generally suggest a different name for your list class, given the potential confusion (in minds of people using your classes) with std::list.
Yes.. I already did that..Quote:
Originally Posted by grumpy
But I still can't understand the difference between this and doing the same in the constructor.Code:bool is_list(std::string s, list& l)
{
try{l=list(s);}
catch(exception e){if(e.what()==std::string("BAD_LIST: "+s)) return false;}
return true;
}
(As the validity of the list object created doesn't matter if the string is invalid)
As for a different name...I would do so when refactoring...(just list ...in my namespace is a short and sweet name for quickly writing the code.)
Don't catch exceptions by value!Quote:
Code:bool is_list(std::string s, list& l)
{
try{l=list(s);}
catch(exception e){if(e.what()==std::string("BAD_LIST: "+s)) return false;}
return true;
}
O_o
This function says that the list is valid if, for example, allocation from `new' fails.
One is right; the other is wrong, but if you need a reason, look no further than consistency.Quote:
But I still can't understand the difference between this and doing the same in the constructor.
If the constructor fails for any reason other than "this string is not a list" the constructor has to raise an exception. Why do you want a special case to ask a question that can be more truly answered using a different method?
Soma
When it comes to the second solution, the IsValid() function must be named differently, if it is supposed to create any object.
Created object must be valid. If constructor of any object succeeded, that object must be usable.Quote:
But I still can't understand the difference between this and doing the same in the constructor.
(As the validity of the list object created doesn't matter if the string is invalid)
I consider STL naming convensions (like that one) very bad, which is fine, since noone is going to create confusing names ;].Quote:
As for a different name...I would do so when refactoring...(just list ...in my namespace is a short and sweet name for quickly writing the code.)
Oops.. slipped.Quote:
Originally Posted by phantomotap
What new ? ...isn't the memory for 'l' already there?Quote:
This function says that the list is valid if, for example, allocation from `new' fails.
Consider this usage of the checking:Quote:
If the constructor fails for any reason other than "this string is not a list" the constructor has to raise an exception. Why do you want a special case to ask a question that can be more truly answered using a different method?
My point is that, the function itself calls the constructor...so all the potential problems remain same....other exceptions will propagate downwards the same way.Code:std::string setv(list& l)
{
if(l.size()!=2)throw(exception("BAD_SETV"));
std::string name=l.car(),value=l.cdr().car();
list result;
if(is_list(value,result))value=result.eval();
auto v = var_scope.find(name);
if(v!=nullptr)*v = value;
else var_scope.new_global_val(name,value);
return name;
}
//VS
std::string setv(list& l)
{
if(l.size()!=2)throw(exception("BAD_SETV"));
std::string name=l.car(),value=l.cdr().car();
bool test(true);
list result(value,test);
if(test) value=result.eval();
auto v = var_scope.find(name);
if(v!=nullptr)*v = value;
else var_scope.new_global_val(name,value);
return name;
}
I disagree with your comment that validity of the list object created doesn't matter if the string is invalid. Your approach is propagating an invalidly constructed object to the caller. That is an unnecessary invitation for the caller to use an improperly constructed object.
Try implementing your is_list() function like this.
With this approach, as far as the caller is concerned, validity of the string can be checked, but the caller has no access to any invalidly constructed object.Code:bool is_list(const std::string &s)
{
bool retval(true);
try{list s;}
catch(...){retval = false;}
return retval;
}
If the caller wants to create an object regardless, the caller can still use the constructor directly, and then deal with any exception thrown.
Ok..Quote:
Originally Posted by kmdv
Hence it could make a blank list..if the exception is caught.Quote:
Created object must be valid. If constructor of any object succeeded, that object must be usable.
Why construct the same thing twice ?Quote:
Originally Posted by grumpy
(Wouldn't that quickly add up to a lot of slowdown?)
You're overly attached to the notion that an object must be created. If a constructor throws, there is no longer an object.
I am not suggesting constructing anything twice. You are.
If you are checking validity of a string, you (presumably) only subsequently need an object if the string is valid.
But the constructor runs, even if not to completion...Quote:
Originally Posted by grumpy
If the string to be checked is..say 200 lines... that would result in the thing being parsed twice.(..or rather 1.x times).
Surely doing it once is better ~..?
O_o
So, to sum up, you are building a `list', whatever it is, as you parse the contents of a `string' and only want to construct one `list' object while staying true to good design and implementation patterns?
Instead of going further down this path for now, I have to ask, why don't you make construction of an empty `list' very cheap and provide a copy assignment that is destructive in nature?
If constructing the empty `list' on the stack is almost free, constructing it can't be a "hotspot".
If taking one `list' that is already constructed and swapping it for another `list' that is already constructed is almost free, swapping the two can't be a "hotspot".
In other words, your implied reason for not using a free function to build the string is built on misunderstanding and bad design.
Consider this:
and now this:Code:list result; // L1
if(is_list(value,result))value=result.eval();
while maintaining that the constructor must succeed or fail spectacularly with a raised exception.Code:bool trytobuildlistfromstring(std::string s, list& l /*L2*/)
{
try{list newlist(s); /*L3*/
l.swap(newlist);}
catch(...){return false;}
return true;
}
Look at all the advantages this approach has:
We know that construction will succeed or fail spectacularly always; there are no special aspects to remember.
If the creation of 'L1' is almost free, as it certainly will be if it is just initializing a few pointers to null, the cost of that object is trivial.
As construction is all or nothing, we can see that 'L2' (and thus 'L1') will not be modified if the string can't be parsed correctly.
If the members of the `list' objects are swapped, instead of using a potentially expensive copy operation, the construction is "free" in that we will continue using those elements as referenced from 'L2' (and thus 'L1').
The function is clear and concise. (Though the name is verbose here for other purposes.)
Soma
The constructor is only invoked because you are choosing to invoke it in your is_list() function. If there is some realistic option to inexpensively check the string (without creating a list) then THAT is what you need to do in the is_list() function.
If there is no such realistic option, and you care as much as you (apparently) do about parsing the string more than once, or how many times the constructor runs, then you only need the single constructor.
All that then requires is that the user of your class always attempt to create an object, and then handles (or propagates) any exception that is thrown.
The 'list' is a singly linked list which may contain other lists or just string values.Quote:
Originally Posted by phantomotap
So creating is just allocating to sentinel nodes..(It could be made into nullptrs if required though..)
Good idea... I'll do so.Quote:
As construction is all or nothing, we can see that 'L2' (and thus 'L1') will not be modified if the string can't be parsed correctly.
If the members of the `list' objects are swapped, instead of using a potentially expensive copy operation, the construction is "free" in that we will continue using those elements as referenced from 'L2' (and thus 'L1').
The function is clear and concise. (Though the name is verbose here for other purposes.)
The checking can't be less complicated than creating...I think.Quote:
Originally Posted by grumpy
As.. "(1 2 3 4)" and "(()(a b c)((1 2 3 4)(x y z)))" are both valid for constructing the list.
Hence, my advice is that you only have the one throwing constructor, and don't bother with the is_list() function.
All of grumpy's replies are exactly how to implement RAII. It would be good to follow them.
If a constructor can fail without throwing an exception, then the destructor will have to deal with destroying an object that wasn't successfully constructed. This increases complexity and the probability of a bug.
Unless you have a solid design requirement which forbids exceptions ("I don't like them" is not a valid reason), use exceptions to communicate constructor failure.
I have to work in a codebase that forbids exceptions. I'm really not sure how to put into words how much that sucks. At least the reason why is a legitimate reason, though.
Out of curiosity, what is that "legitimate reason"?Quote:
Originally Posted by brewbuck
We develop an SDK which is used in certain kinds of software drivers. Those drivers are subjected to automated testing with a well-known test framework by a well-known company. The particular tests which these drivers are subjected to, will consider the throwing of any exception of any type to be a failed test. The drivers must pass the tests. If our SDK throws exceptions, the drivers fail the tests and they cannot use our SDK.Quote:
Originally Posted by grumpy
Can't you just enforce the rule that exceptions must not be allowed to propagate beyond module boundaries, in this just the module boundary being the public interface of the SDK?Quote:
Originally Posted by brewbuck
Doesn't work. The third party test framework is looking for exceptions being THROWN, not uncaught exceptions.Quote:
Originally Posted by laserlight
Funny story how we initially discovered this unstated requirement. The code was already written to not use exceptions (for dogmatic reasons only). Then we were shown a weird failure on this test bench -- remember, we're just a component in someone else's code -- and it seemed to be caused by an exception thrown by our own code.
Of course, that couldn't be true as we do not use exceptions. Eventually we tracked the problem to a failed memory allocation via no-throw operator new, an allocation we were making. The reason the allocation was failing is also a funny story but can't be repeated here. Okay, what the heck. The test is popping because of an exception generated by no-throw new? Yes. The C++ runtime had implemented no-throw new by wrapping throwing new in a try-catch and returning NULL if an exception was thrown.
The fix? Override all the standard library's new and delete operators to call malloc() directly and simply return NULL on failure, rather than generating, then absorbing, an exception, completely internally.
O_oQuote:
The third party test framework is looking for exceptions being THROWN, not uncaught exceptions.
That's bizarre on so many levels.
I wonder, are you able to explain why this is a condition for passing? (I appreciate that you may not be able to discuss it for whatever reason. I'm just very extremely curious.)
Soma
I think it's not really intentional. A debugger is attached to the process being tested, the debugger is configured to automatically break when any of a large number of weird conditions happens, the throwing of an exception being one of them. I don't think there is an explicit requirement that exceptions be avoided, the rule is just "If the debugger breaks in you fail."Quote:
Originally Posted by phantomotap
We filed a bug with the maker of the test harness. Might as well have filed it with the Pope.