Martin Vermeer wrote:
> On Mon, Jul 21, 2003 at 12:03:13PM +0000, Angus Leeming spake thusly:
>
> ...
>
>> > > If not you should have a 'break' in
>> > > there. Your Assert isn't doing much at the moment ;-)
>> >
>> > It should catch the 'branch not existing' condition and return an
>> > empty string. I believe Assert(false) will do that.
>
> Of course the statement following Assert(false) doesn't do much
> either... but gcc doesn't notice that and bleats. That's why returning
> an empty string :-)
>
> ...
>
>> > I don't think it's wrong, but yours looks better. Compiling now.
>>
>> Well and good. Nonetheless, it is wrong:
>> while (j < string::npos) {
>> j = s.find_first_of(separator(), i);
>> // j can now be string::npos
>> // j-i makes no sense
>> name = s.substr(i, j - i);
>
> Good point :-) and Alfredo's point that '<' doesn't necessarily work
> right with npos.
>
>> Angus
>
> See the fresh patch attached. Thanks for all the good help!
This is going to bomb unless you add a return statement
+void BranchList::setColor(string const & s, string const & val)
+{
+ List::iterator it = list.begin();
+ List::iterator end = list.end();
+ for (; it != end; it++) {
+ if (s.find(it->getBranch(), 0) != string::npos) {
+ it->setColor(val);
return;
+ }
+ }
+ Assert(false);
+}
Presumably, this one would benefit from either a 'return' or a 'break'
+void BranchList::setSelected(string const & s, bool val)
+{
+ List::iterator it = list.begin();
+ List::iterator end = list.end();
+ for (; it != end; it++) {
+ if (s.find(it->getBranch(), 0) != string::npos) {
+ it->setSelected(val);
return
}
+ }
+}
Here you should move the (j == string::npos) test to immediately after the j
= s.find_first_of line, else the very next line is going to be nasty.
+void BranchList::add(string const & s)
+{
+ string name;
+ string::size_type i = 0;
+ Branch br;
+ for (;;) {
+ string::size_type const j = s.find_first_of(separator(), i);
+ name = s.substr(i, j - i);
+ br.setBranch(name);
+ br.setSelected(false);
+ br.setColor("none");
+ list.push_back(br);
+ if (j == string::npos)
+ break;
+ i = j;
+ i++;
+ }
+}
Does it make sense to setColor here if j == npos?
+void BranchList::setColors(string const & s)
+{
+ string name;
+ string::size_type i = 0;
+ List::iterator it = list.begin();
+ for (;;) {
+ string::size_type const j = s.find_first_of(separator(), i);
+ name = s.substr(i, j - i);
+ it->setColor(name);
+ if (j == string::npos)
+ break;
+ it++;
+ Assert(it != list.end());
+ i = j;
+ i++;
+ }
+}
Trivia:
indent with a tab:
+class InsetBranch : public InsetCollapsable {
+ InsetBranchParams params_;
+};
--
Angus
