string parsing crashes

All code submission.
MootPoint
Posts: 58
Joined: Fri Sep 24, 2010 7:44 am

string parsing crashes

Post by MootPoint » Fri Oct 01, 2010 11:12 am

These fixes take care of some crashes I hit when loading psp versions of my player options on Win.
Attachments

[The extension zip has been deactivated and can no longer be displayed.]


wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Fri Oct 01, 2010 1:25 pm

Looks good.
A very minor comment:
Please try to keep braces on the same line (unless you have a good reason to not do so, which sometimes happens, but you'd rather be ready to justify it).

please use:

Code: Select all

for (size_t t=0,i=0; t < input.size();t++) { 


please avoid

Code: Select all

for (size_t t=0,i=0; t < input.size();t++)
{
we don't have any explicit rule for that but I'm strongly thinking about it, so just a heads up for your future patches ;)

MootPoint
Posts: 58
Joined: Fri Sep 24, 2010 7:44 am

Re: string parsing crashes

Post by MootPoint » Fri Oct 01, 2010 4:38 pm

I'll give you one strong reason why one should put braces on subsequent lines (and this is what we have for coding standards at work): it becomes very easy to see scope.

Code: Select all

{
	std::string x("foo");
	{
		std::string x("bar");
		{
			std::out << x;
			if (ConditionCheck() == SomethingElse())
			{
				string x("lame example, but you get the point");
			}
		}
	}
}	
In one glance, I can tell exactly where each new scope of x starts & ends. Similarly, when you're dealing with giant nests of if {} blocks, this rapidly becomes more useful, esp if you need to copy paste things around within that nested set of braces.

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Fri Oct 01, 2010 5:04 pm

MootPoint wrote: Similarly, when you're dealing with giant nests of if {} blocks
We should avoid that as much as possible (I know it is not always the case right now in our code)

also, I didn't want you to give me a good "general" reason, but a good reason every time you want to put the bracket on a new line.
Our rule (implicit for now, but explicit soon) is to put opening brackets on the same line. You may disagree because it is more a religious war than anything, but this is our (again, implicit for now) convention.

There are cases where it majorly improves readability (for example, if you init variables in a constructor before the constructor code...) and I am ready to discuss them.

Regarding scope again, I agree with you, but another rule (currently implicit, but soon to become explicit) is to avoid block nesting as much as possible.

Reasons for putting them on the same line:
- reduces the number of lines dramatically, enabling to see more code on the screen in one page
- doesn't screw up IDEs that give you information on the line containing the matching opening bracket when you are looking at a closing one.

In your patch, the "{" changes you made are exactly ones that IMO don't improve readability and just make the function take more space on the screen than it should.
Again, there are good arguments for both things, and I'm not discussing that.

The code conventions in Wagic are not the result of a democratic vote. I ask for people's opinion but I take the decision in the end. I'm evil :twisted:
Note that my point is not to strongly enforce this rule until it actually becomes one.
MootPoint wrote:(and this is what we have for coding standards at work)
My team has the convention of putting brackets at the end of the line, and I admit this majorly influenced my choice. HBL has the same rules as yours, and I keep scr..ing it up, so I'm happy to have rules close to that of my team in my personal project. I know that's a lame excuse.

MootPoint
Posts: 58
Joined: Fri Sep 24, 2010 7:44 am

Re: string parsing crashes

Post by MootPoint » Fri Oct 01, 2010 5:29 pm

definitely a religious argument, I remember the debates we had at work when finalizing our coding style guidelines :lol:

I will argue though that seeing more lines of code isn't necessarily beneficial, it can actually detract from legibility, which I think is the most important goal of maintainable code. Case in point, I find reading this a nightmare:

Code: Select all

  ATransformerFOREVER(int id, MTGCardInstance * source, MTGCardInstance * target, string types, string abilities):InstantAbility(id,source,target){
     ability = NEW AForeverTransformer(id,source,target,types,abilities);}
    int resolve(){
    AForeverTransformer * a = ability->clone();
    GenericInstantAbility * wrapper = NEW GenericInstantAbility(1,source,(Damageable *)(this->target),a);
    wrapper->addToGame();
    return 1;}
    ATransformerFOREVER * clone() const{
    ATransformerFOREVER * a =  NEW ATransformerFOREVER(*this);
    a->ability = this->ability->clone();
    a->isClone = 1;
    return a;}
At a glance, I can't tell where a function starts and where one ends. Here's how I'd format it:

Code: Select all

 
/*
**
*/
ATransformerFOREVER(int id, MTGCardInstance * source, MTGCardInstance * target, string types, string abilities)
	:
	InstantAbility(id,source,target)
{
	ability = NEW AForeverTransformer(id,source,target,types,abilities);
}

/*
**  Also, note here that the construction of GenericInstantAbility() is a little long in the tooth - I might choose here to tab align the
**  params, one per line - depends how much horizontal space you deem is acceptable.
*/
int resolve()
{
	AForeverTransformer * a = ability->clone();
	GenericInstantAbility * wrapper = NEW GenericInstantAbility(1,source,(Damageable *)(this->target),a);
	wrapper->addToGame();
	return 1;
}

/*
**
*/
ATransformerFOREVER * clone() const
{
	ATransformerFOREVER * a =  NEW ATransformerFOREVER(*this);
	a->ability = this->ability->clone();
	a->isClone = 1;
	return a;
}
So yeah, I've chewed up a lot of extra vertical space, but chances are if I'm examining the constructor for a bug, I don't really give a crepes that the clone() function is only a few lines away. I now have clear delineation between my functions at a simple glance, and I understand the scope of my functions without having to squint & pay attention to where the braces are landing.

mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Fri Oct 01, 2010 5:36 pm

I'm on the same side with MootPoint on this one. But as you say, this is your project and only one of many different coding styles around. It would be nice to firm up the coding style guide since there are clearly more people involved now with different coding styles as well as with new devs who are learning how to code. Coding conventions are key to understanding code in an environment like this. It makes the code overall more legible and cohesive.

A couple of other things to think about:
1) spacing between parens and parameters in method signatures and calls
2) variable naming conventions. MootPoint reminded me about this the other day when he noticed I used camel casing for variable naming, when the rest of the code seemed to not use it. :) (i'll have to fix that later)
3) Is there an order to how you would like the methods, variables in the source files to be declared? ie private, protected, public or public, protected, private
4) indentation of code. currently you've stated 2 spaces, but could we discuss the use of 4 spaces?

there are probably more, and I would like for all of us to have a discussion if possible about these conventions at the appropriate time.

Thanks,
Mike

MootPoint
Posts: 58
Joined: Fri Sep 24, 2010 7:44 am

Re: string parsing crashes

Post by MootPoint » Fri Oct 01, 2010 8:14 pm

here's another very good example of an aggravation caused by terse coding practices:
int GameObserver::receiveEvent(WEvent * e){
if (!e) return 0;
eventsQueue.push(e);
if (eventsQueue.size() > 1) return -1; //resolving events can generate more events
What if I want to put a breakpoint on any of these error conditions? Uh, I can't. Not without reformatting & recompiling.

Again, if your coding standards force people to do this, you avert the problem:
int GameObserver::receiveEvent(WEvent * e)
{
if (!e)
return 0;
eventsQueue.push(e);
if (eventsQueue.size() > 1)
return -1; //resolving events can generate more events

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Sat Oct 02, 2010 1:03 am

Ok, let's hijack this topic and discuss coding guidelines, I do want us to have stricter rules after the upcoming release. I'll try to have the other devs have a look at this thread and state their own arguments.

Please answer to the following questions, give examples if you want. I'll update this thread with your choices, you're of course allowed to change your mind as we discuss

1) How should we format opening/closing brackets ?
always on a new line: MootPoint, mnguyen
always on the same line:
On the same line except for functions opening/closing brackets: wololo

2) spacing between parens and parameters in method signatures and calls

int foo(int a, int b) { : wololo

3.a) Should we add brackets after a "if" if there's only one operation inside the if ?
No:
Yes: wololo
3.b) should the unique operation after a "if" be on a separate line?
No:
Yes: wololo, MootPoint

4) Indentation: 2 or 4 spaces ?
2:
4: wololo, mnguyen

5.a) variable naming: camelcase or lowercase+underscore ?
camelcase wololo
lowercase+underscore

5.b) variable naming: how to prefix global variables, class attributes, local variables?

gGlobal, mAttribute, noSpecificRuleForLocalVars : wololo


6) any maximum number of nested blocks depth? (this is of course flexible..it means the coder will be strongly encouraged to refactor when this happens)

No
3: wololo

7) any limit to the size of a line ?
No: wololo

8) What order should we use for variable/methods to be declared in .h files? How about order in .cpp files?
h files
private, protected,public > Attributes then methods > And alphabetical order inside those: wololo

cpp files
No specific order: wololo

9) Anything else? (we can also discuss that later, we don't need all rules at once)

Jeck
Posts: 71
Joined: Mon Aug 17, 2009 11:53 pm
Location: Snow-covered forest

Re: string parsing crashes

Post by Jeck » Sat Oct 02, 2010 4:25 am

1) How should we format opening/closing brackets ? I'm with Wololo, I prefer always on the same line except for opening/closing classes/functions.
2) int foo(int a, int b) { sounds fine.
3.a) No, I don't think it's necessary. I find single-statement if clusters perfectly legible.
3.b) I don't mind moving towards requiring a newline.
4) Four spaces is fine.
5.a) Definitely prefer camelCase.
5.b) gGlobal, mAttribute, noSpecificRuleForLocalVars sounds good.
6) I've a slight preference not to limit, because this'd be on a case-by-case basis anyways. Maybe rather than a limit, we'd make it mandatory to request code reviews when over X blocks? I dunno, it seems like a lot of management overhead would be involved...
7) No hard limit, but be reasonable.
8) I'm fine with ordering based on private/protected/public, and then attributes/methods, but there's no good reason I can see to order beyond that alphabetically. I think it makes much more sense to order by logical groups, having all the file IO in one group, etc, on a class by class basis. Any modern IDE will bring you directly to the function definition from a given call, so I don't see alphabetical ordering as an advantage. On the other hand, logical grouping means that when researching a given function you can immediately see related functions. I'd be all for a requirement that these logical groups be explicitly labeled, though.

9) While we're coming up with rules, I find it immensely helpful to have a single sentence synopsis comment following function member definition. Not as a hard rule, but as a strong suggestion? Really, the more comments the better, in my opinion.

Djardin
Posts: 129
Joined: Sat Sep 04, 2010 11:40 am

Re: string parsing crashes

Post by Djardin » Sat Oct 02, 2010 10:27 am

1) How should we format opening/closing brackets ?
=> always on a new line except when I'm not in a mood for it (see after) !!!

2) spacing between parens and parameters in method signatures and calls
=> int foo(int a, int b) { looks good

3.a) Should we add brackets after a "if" if there's only one operation inside the if ?
=> Yes
3.b) should the unique operation after a "if" be on a separate line?
=> Yes

4) Indentation: 2 or 4 spaces ?
=> 2

5.a) variable naming: camelcase or lowercase+underscore ?
=> camelcase

5.b) variable naming: how to prefix global variables, class attributes, local variables?
=> gGlobal, mAttribute, noSpecificRuleForLocalVars

6) any maximum number of nested blocks depth? (this is of course flexible..it means the coder will be strongly encouraged to refactor when this happens)
=> No : if it's flexible, then it's not a rule, we need to find something else, for example, limit the number of line of a method and limit the number of characters per line

7) any limit to the size of a line ?
=> Yes: my company used to force developers to rewrite code if code lines where longer than 80 characters. It's a bit **** currently, but I would say, more than 100 characters should be avoided

8) What order should we use for variable/methods to be declared in .h files? How about order in .cpp files?
=> private, protected,public > Attributes then methods only for .h. No alphabetic order, everybody uses smart editors anyway

9) Anything else?
=> We all agree on no exception ?
=> Any restriction on the usage of template and macro ? They can make the one writing them very proud but they can also make the ones using or modifying them very unhappy depending of their complexity ...
=> return point in method. My company forces every method to have a single return point. I think it's also a practice applied in the Linux Kernel development. It increases a lot the readability once it's correctly applied correctly and everybody is used to it. It can also be used to remove nested blocks.

For example, without it, you get:

Code: Select all

int foo()
{
  if(somethingHappened)
  {
    return somethingSmart;
  }
  else
  {
    if(reallySure)
    {
      return somethingDumb;
    }
    else
    {
      return somethingNotThatDumb;
    }
  }
  return whatever;
}
With it you get:

Code: Select all

int foo()
{
  int something;

  do  { // I like having those on the same line, but not the others !!
    if(somethingHappened)
    {
      something = somethingSmart;
      break;
    }
    if(reallySure)
    {
      something = somethingDumb;
      break;
    }
    else
    {
      something = somethingNotThatDumb;
      break;
    }
    something = whatever;
  } while(0);
  
  return something;
}

Post Reply