kilo icon indicating copy to clipboard operation
kilo copied to clipboard

Small nit

Open maplant opened this issue 9 years ago • 2 comments
trafficstars

Hey there, I don't really want to make a pull request here so I thought I'd just make an issue. Sorry if that's not proper etiquette! By the way, love the work. The following lines of code appear in the source code:

erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
if (row) {
        for (j = E.coloff; j < (E.cx+E.coloff); j++) {
            if (j < row->size && row->chars[j] == TAB) cx += 7-((cx)%8);
            cx++;
        }
}

Surely this should be simplified slightly to:

if (filerow < E.numrows) {
        erow *row = &E.row[filerow];
        for (j = E.coloff; j < (E.cx+E.coloff); j++) {
            if (j < row->size && row->chars[j] == TAB) cx += 7-((cx)%8);
            cx++;
        }
 }

It seems a little off to make the conditional based off of the result of another conditional that appears literally one line above.

maplant avatar Jul 11 '16 21:07 maplant

@DataAnalysisCosby Your simplification looks better in this case. I'm guessing the reasoning behind this structure it is probably to keep it consistent with the other usages of erow *row = …; in the code base:

$ grep 'erow \*row = (' *.c
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];
    erow *row = (filerow >= E.numrows) ? NULL : &E.row[filerow];

practicalswift avatar Jul 12 '16 20:07 practicalswift

@DataAnalysisCosby @practicalswift I would also agree that this simplification is an improvement. It also makes the purpose of the code much more clear.

SevenBits avatar Jul 13 '16 18:07 SevenBits