Results of cppcheck
alzeha opened this issue · 2 comments
Hi,
I am currently analyzing the code using cppcheck. I've found a few lines of code that might look like bugs, but I am not that sure. Before solving them by myself I wanted to ask whether you are interested. If not, just close this issue ;)
After some analysis and removing of false positive, I believe that the following main points, which result into warnings of cppcheck, are relevant:
the destructor is not virtual. This is surprising since the class contains a virtual method.
tchecker/include/tchecker/waiting/waiting.hh
Line 194 in 9b36a60
Assert using empty(). As far as I see, empty is not side-effect free. It calls remove_non_waiting_first(). I am not sure, but this looks like the code behaves differently depending on whether assertions are enabled or they are not.
tchecker/include/tchecker/waiting/waiting.hh
Line 207 in 9b36a60
Same.
throw; outside of a catch statement. I think you want to get into the catch part (using exception throwing as part of the control flow), but rethrowing a non-existing exception actually terminates the program (or is undefined, I am not sure about this but it looks weird).
tchecker/src/algorithms/covreach/stats.cc
Line 17 in 9b36a60
looks like _stored_states is not initialized
in comparison with
tchecker/src/clockbounds/solver.cc
Line 162 in 9b36a60
the order of x and y is inversed
tchecker/include/tchecker/zg/state.hh
Line 91 in 9b36a60
the doc says the assignment operator is deleted, but actually it is set to default
tchecker/include/tchecker/zg/state.hh
Line 96 in 9b36a60
Same.
The final point is a little bit complicated. It regards the files include/tchecker/utils/shared_objects.hh and include/tchecker/zg/transition.hh.
Due to the fact that assignment and copy operator for transitions are deleted (btw: why?), the copy and assignment operator defined in the shared_objects will behave quite strange.
Of course, they are never used, therefore it won’t have any impact right now. Nevertheless, a new person working with this code might be mislead here, so it might be a good idea to employ some ifs here (or to set the operators to default, as already happened at state.hh).
In case that you fixed (some of) them, please inform me :) In case you don't want to fix any of them, please just close this issue. In case you have any questions, feel free to ask.
Best,
Alexander
Dear Alexander,
Thanks a lot for your remarks. I have fixed the issues. Here are some details:
the destructor is not virtual. This is surprising since the class contains a virtual method.
Sure, there was a potential error here. The destructor is now virtual.
tchecker/include/tchecker/waiting/waiting.hh
Line 194 in 9b36a60
Assert using empty(). As far as I see, empty is not side-effect free. It calls remove_non_waiting_first(). I am not sure, but this looks like the code behaves differently depending on whether assertions are enabled or they are not.
tchecker/include/tchecker/waiting/waiting.hh
Line 207 in 9b36a60
Same.
There was no bug since remove_non_waiting_first() was called immediately before the assertions. Anyway, as you poiinted out, that was not clear. So I have replaced the assertion to a call to the empty() method of the underlying container.
throw; outside of a catch statement. I think you want to get into the catch part (using exception throwing as part of the control flow), but rethrowing a non-existing exception actually terminates the program (or is undefined, I am not sure about this but it looks weird).
True, it is now throwing an exception.
tchecker/src/algorithms/covreach/stats.cc
Line 17 in 9b36a60
looks like _stored_states is not initialized
Yes, this was missing.
in comparison with
tchecker/src/clockbounds/solver.cc
Line 162 in 9b36a60
the order of x and y is inversed
Rght, the code was correct, but the documentation of the method was not up-to-date.
tchecker/include/tchecker/zg/state.hh
Line 91 in 9b36a60
the doc says the assignment operator is deleted, but actually it is set to default
tchecker/include/tchecker/zg/state.hh
Line 96 in 9b36a60
Same.
There was a error, see below.
The final point is a little bit complicated. It regards the files include/tchecker/utils/shared_objects.hh and include/tchecker/zg/transition.hh.
Due to the fact that assignment and copy operator for transitions are deleted (btw: why?), the copy and assignment operator defined in the shared_objects will behave quite strange. Of course, they are never used, therefore it won’t have any impact right now. Nevertheless, a new person working with this code might be mislead here, so it might be a good idea to employ some ifs here (or to set the operators to default, as already happened at state.hh).
In case that you fixed (some of) them, please inform me :) In case you don't want to fix any of them, please just close this issue. In case you have any questions, feel free to ask.
States and transitions store (intrusive) shared pointers to pool allocated objects. However, states and transitions have no access to the pool. So they cannot make a copy of the objects they point to. So copy and assignment are not safe since:
state_t s1;
state_t s2{s1};
s2.x = ...;would result in s1 and s2 sharing x, and any modification to x in s2 would modify s1 as well. So the only good solution that I have is to delete copy constructor and assignment. Move constructor and move assignment are ok since they do not result in shared memory.
Notice that the copy constructor, assignment operator, move constructor and move-assignment operator in tchecker::shared_object_t explicitely call the corresponding methods of the type T. So, if the method is deleted the code will not compile. Hence, I do not expect any strange behavior here. Tell me if i am missing something.
These issues have been fixed in the latest commit. Are you ok with closing this ticket?
Thanks again for your very useful feedback.
Best regards,
Fr´´déric
yes, looks great. thx!