View Single Post
Old 16/02/2018, 12:21 PM   #9
yugecin
Big Clucker
 
Join Date: Oct 2009
Location: Belgium
Posts: 83
Reputation: 82
Default Re: Anyone have some feedback? (C code)

Quote:
Originally Posted by Paglia View Post
That case works, the code handles it in the way that it counts all non-whitespace characters and evaluates the gap based on the tabsize we defined on the top of the file - that number of non-whitespace chars, specifically this line:

Code:
if(ns >= TABSIZE - nc)
The problem is that nc is only incremented, and doesn't go down. As result this condition will always evaluate to true and always place tabs.
But I see you've updated your code so this is not applicable anymore.

I tested your updated code and this is what I got (tabsize 2, replaced tabs by a ->):
Code:
       ->->->->->
 input:  a b b c d
output:->a>bb>cd
So the result is not always correct.

Code:
  a b b c d
   ^ after this point, nchars is 1 and nspaces is 1
      'nspaces >= TABSIZE - nchars' evaluates to true, so it adds a tab
      but then, in the if block you do 'nspaces -= TABSIZE', effectively setting nspaces to -1
      so the space after the b is simply ignored (and again after the c)
Putting nspaces to 0 on line 29 (like the comment above suggests), fixes that.

Now I see this issue:
Code:
       ->->->->->
 input:  a bb b
output:->a>bb>b
             ^ this should not be a tab, but a space
Basically at every iteration of the while loop, nchars is 1 if the previous character was not a tab (except in the beginning if the line begins with (a) space(s)).
So after the space after the 2nd b, nchars is 1 and nspaces is 1, evaluating nspaces >= TABSIZE - nchars to true, so it will insert a tab incorrectly.

(I have only tested with tabsize 2 and no tabs in input so far)
(I think the comment on line 16 is wrong too, the last word should probably be 'tab' instead of 'space')

The rule for inserting a tab would be something like this (example with tabsize 2):
Code:
A tab will move the cursor to the next tab position, which is a multiple of tabsize.
So at char 0, inserting a tab will put cursor at 2.
At char 1, it will also put the cursor at 2 because that is the next tab position
At char 2, the next tab position is 4
At char 3, it will be 4 as well
etc...

->->->->->->->
  a bb c    c
->a>bb c->->c

so this is what I would do:
keep track of how many characters are added since the last tab position
when the next non-space, non-tab char is read, see if the amount
  of spaces is >= the amount of characters missing to proceed
  to the next multiple of TABSIZE. If so, insert tabs
And here's another case that doesn't work, if there are spaces followed by a tab (example tabsize 8 ):
Code:
a    -->b
that can be changed by 1 single tab
a------>b
to do that, check first if c is a tab, and if so,
  add TABSIZE (minus the tab column you're at) to nspaces and continue
I forked your gist and changed so the things I described above works.
(I also added the continue stuff I talked about, see below)

https://gist.github.com/yugecin/7ff4...0ca1e92a70bd9b
The main way of thinking in my code is to remember how many characters are added since the last tab column.

I haven't tested it thoroughly but it seems to be working for all the testcases I tried.
sample testcases (tabsize 8 ):
Code:
  aaa                          b b      b                        bbbbbbbbbbbbbbbbbbbbb          d
  aaa			       b b	b			 bbbbbbbbbbbbbbbbbbbbb		d
    	       	                  a
				  a
                     a					           b
		     a						   b
formatted with ->:
Code:
------->------->------->------->------->------->------->------->------->------->------->------->
  aaa                          b b      b                        bbbbbbbbbbbbbbbbbbbbb          d
  aaa-->------->------->       b b----->b------>------->-------> bbbbbbbbbbbbbbbbbbbbb->------->d
    --->       >                  a
------->------->------->------->  a
                     a->------->------->------->------->           b
------->------->     a->------->------->------->------->------->   b
---

Quote:
Originally Posted by Paglia View Post
For the comment of avoid else and use continue, i assume you mean the else inside the while loop that evaluates whether one or more spaces are still "in buffer". Wouldn't using continue not allow me to put that space and decrease the space count, while the else does? Is it too much to ask if you can rewrite this with your continue method so that I can see how you'd do it?
Basically your code is like this:
Code:
while (..) {
	if (..) {
		...
	} else {
		...
	}
}
and I would do
Code:
while (..) {
	if (..) {
		...
		continue;
	}

	...
}
It reduces the level of indentation in your else branch and if you're checking the code you don't need to scroll down to the point where the else branch ends to see if some other statement will execute or not, by seeing the continue you know that there's nothing more going on so you can read through more quickly (imo).
edit: this is only my opinion, of course. I'd say stick with what you think works best for you.
__________________
robin_be

Last edited by yugecin; 17/02/2018 at 12:08 PM.
yugecin is offline   Reply With Quote