@l3u Formatting rework

[speters]

Duplicated from https://github.com/openv/vcontrold/pull/3#issuecomment-343144466 to keep https://github.com/openv/vcontrold/pull/3 clean:

@l3u wrote:

Here is my suggestion for a cleaner base to re-start from: https://github.com/openv/vcontrold/tree/formatting_rework

No code has been changed yet. All just formatting.

@speters wrote:

@l3u :+1: Would be nice to see it merged. Did you also check file encodings and line endings? Do you really see the need for Copyright headers? At least in trivial files like version.h it is ridiculous.

[l3u]

All files should be now UTF-8 encoded and have UNIX line endings.

I simply added copyright headers everywhere, as I think they don't hurt (all bigger project do it in this way, so why not?). This is only my opinon, but I think in a community project with (hopefully) a lot of contributors, a common and meaningful formatting standard is very important to keep it all readable and maintainable.

If it's all neat and tidied up, somebody contributing code will rather try to work in a clean way than on a codebase where in one file it's consequently formatting and in another, it's not.

[hmueller01]

Is this really correct formatted? At least parser.c does not look so good ...

    if (strstr(line, "WAIT") == line)
    { token = WAIT; }
    else if (strstr(line, "SEND BYTES") == line) {
        token = BYTES;
    } else if (strstr(line, "SEND") == line)
    { token = SEND; }

I would expect something like

    if (strstr(line, "WAIT") == line) {
       token = WAIT;
    } else if (strstr(line, "SEND BYTES") == line) {
        token = BYTES;
    } else if (strstr(line, "SEND") == line) {
        token = SEND;
    }

[l3u]

It's not yet. Only what could have been done automatically. I'm not finished yet, please give me some time ;-)

[hmueller01]

Ok, sorry. Pls let me know. I am willing to do some work too. If the above style is the style you expect, I could correct e.g. parser.c and framer.c via pull request. :) Cheers Holger

[l3u]

Okay, now the ones which I haven't reworked yet are:

• vclient.[c,h]
• vcontrold.[c,h]
• vsim.c
• xmlconfig.[c,h]

Would you please be so kind and have a look at my changes? I hope the code still does what it did before ;-)

… and of course, you can do the same with the remaining files!

[hmueller01]

Reworked framer.h, parser.c, vcontrold.c, vsim.c in pull request #7

Still open

• vclient.[c,h]
• xmlconfig.[c,h]

N.B. comments are like // comment and commented out code is like //code And there is such commented out code and we should look into it if we can remove it in the future.

[l3u]

Oh, I just saw your writing about the comments. You're of course right, this is the way code should be commented out and comments should be written.

I also think we should kick out (most of) the commented out code. I'm pretty sure these are mostly remnants of development/testing.

[hmueller01]

@l3u I saw your changes on my rework. You are even more precise than me. :) I was thinking about changing the single line comments to // but left it. You did it. Thanks.

I suggest to remove the commented code and the bad goto's in a next step/branch. I would like to add my non P300 changes in framer.c as well before digging the set_addr bug. Makes debugging easier. Maybe we can tag this release as reworked without functional changes.

[l3u]

That's a good idea imo. I think the multi-line comments are more for commenting out code blocks or really big comments, definitely not for singe-line comments.

I really endorse getting rid of the goto stuff. I almost fell off my chair when I saw it 8-o

[hmueller01]

Yesterday I was really confused. I did not get the formatting_rework branch synced to my hmueller01/vcontrold fork. On my cloned fork hmueller01/vcontrold I did (taken from github docs)

git remote add upstream https://github.com/openv/vcontrold.git
git fetch upstream
git merge upstream/master
git checkout formatting_rework 

Changing and committing the files but git push still wants to push to https://github.com/openv/vcontrold.git and I could not see the branch in the web interface of my fork. After trying more than an hour I deleted my fork and made a new fork. After that I was able to push to my fork and made the pull request as you know.

After asking google more precise I now got an answer on stackoverflow, which I will try next time:

• Make sure you've pulled the new upstream branch into your local repo: First, ensure your working tree is clean (commit/stash/revert any changes) Then, do git fetch upstream to retrieve the new upstream branch
• Create and switch to a local version of the new upstream branch (which is named newbranch in this example): git checkout -b newbranch upstream/newbranch
• When you're ready to push the new branch to origin: git push -u origin newbranch The -u switch sets up tracking to the specified remote (in this example, origin)

[l3u]

Okay, now, I'm done here for now. If we keep on reworking all this, we now should translate all messages. As of yet, no code should have changed. Thus, the other work should probably be done in another branch.

[l3u]

Yeah, "Trabnsated" of course ;-) Well, I thought that after all, still no code has been changed (apart from the print statements for the long help messages). So it's still fine and we still have the very state of the last (non-existing) release of vcontrold.

[hmueller01]

Ok, merged with my fork, compiled and started the daemon. Works fine so far. I see no other/new error messages. So for a quick test I confirm this version.

[l3u]

The very problem with this is that each and every patch against the legacy code base will break because (I think) each single line of code has been changed by this. Thus – assumed I did not make some mistake whilst reworking all this – this should be the code base to work on (meaning being merged to master) as soon as possible.

Please review this! As many people as possible!

But if we want to keep up developing this, this is definitely necessary imho.

[vitoopen]

I support the reformatting of the source code. This is a long overdue task.

In my review I found some issues for the following files The moste imported is located in unit.c - it would break existing applications that are parsing "An" and "Aus" in the result of unit "CycleTime".

byteorder.h

• do not reformat this file - it is from another project (aircrack-ng) and must not be modified.
• after reformatting is done I will propose a solution without this file (it is used only for cygwin builds and it's easy to remove it)

parser.c:

• avoid single line { break; }

semaphore.c

• comments moved before the related lines
• log output renamed "error:: " to "error: "
• typo: prozess renamed to process
• struct addrinfo: single declaration per line (to enable comments on the same line)

unit.c

• getCycleTime: translation reverted: THIS CHANGE WOULD BREAK existing applications. => maybe we could enable english results later with a build option, but not for the default.
• typo Lines -> Times (it was "Zeiten" not "Zeilen")
• log output "4 byte" renamed to "4 bytes"
• do not cleanup owner of comment (FN stands for fnobis)

vclient.c

• obsolete empty line removed

vcontrold.c

• avoid single line { printf(...); }

This is a patch file for all issues (except byteorder.h) vitoopen_format_fixes.zip

[l3u]

Hey, not bad for like 10,000 lines of changed code, is it?! ;-)

I'm glad somebody who is more into the code than me looked at this conscientiously. I'll push your changes (feel free to do this yourself next time!).

I'll also reset byteorder.h to the very svn version. Would definitely be nice if this one could be removed.

[hmueller01]

I propose to make no code changes in the formatting_rework. I am fine with these changes right now. Should we merge this with master and start doing code changes there? I already prepared changes in framer.c and would like to push these asap.

[speters]

I introduced f27d409 in the hope that it would keep formatting_rework in sync with b54cb9b on master. But differences are now so profund for the diffing algorithm, that they show up as merge conflicts.

IMO, let's merge formatting_rework into master. Else it gets unmanageable and becomes a blocker for further development.

Static analysis with Coverity Scan showed no differences in possible defects, so I take that for another hint that no new problems were introduced.

[l3u]

Most probably, reverting f27d409 from master and merging this would be better. Or simply just use the version here ;-) It's definitely a good idea not to do any code changes before this becomes master.

I would also suggest to squash all commits here to a single one, as each and every line of code has been changed anyway. Thus, one can see some "we start again from here" point. And for this to be a clean cut, I would both remove the code change from master and this branch, so that we really only have formatting changes with this (or these) commit(s).

[speters]

Squashed-merged and tagged v0.98.3.