Format code of canplayer

[triveria]

Hi,
I use the canplayer daily, and as a result, there are some features I'd like to implement(flag to start candump at desired timestamp; show progress in command-line). I'm currently adding these features to the canplayer. However, while working with the codebase, I noticed areas where the code structure and formatting could benefit from enhancements for improved readability and maintainability. In this pull request, I've introduced some formatting changes and added a .clang-format file, which I've applied to canplayer.c.
My primary goal is to contribute positively and uplift the code quality. In an upcoming pull request, I plan to remove the goto statements and further refactor the code. I believe that with additional adjustments, we can further refine the codebase, and I'm eager to implement these updates.
I look forward to your feedback and hope these changes prove beneficial to the project.

[hartkopp]

Have you checked how these white space changes look like in your own code?

https://github.com/triveria/can-utils/blob/refactor-canplayer/canplayer.c#L256

I'm not against all of those refactoring attempts but this definitely does not add any improvement ...

[triveria]

oh I see what you mean.. Sorry, I only looked at it in my editor and assumed tabwidth equals 4 spaces. Apparently tabwidth should equal 8 spaces. I've fixed it now.

[triveria]

have you had the chance yet to give it a second look?

[hartkopp]

Hi,

generally I like the idea of formatting the code in a common way as you propose with the .clang-format file.

But both the examples below do not fit the current Linux kernel netdev code formatting rules, and I would feel more comfortable if we could wait for @marckleinebudde 's review on such kind of improvements.

#define DEFAULT_GAP 1								   /* ms */
#define DEFAULT_LOOPS 1								   /* only one replay */
#define CHANNELS 20								   /* anyone using more than 20 CAN interfaces at a time? */
#define COMMENTSZ 200
#define BUFSZ (sizeof("(1345212884.318850)") + IFNAMSIZ + 4 + CL_CFSZ + COMMENTSZ) /* for one line in the logfile */
#define STDOUTIDX 65536								   /* interface index for printing on stdout - bigger than max uint16 */


static char		  buf[BUFSZ], device[BUFSZ], ascframe[BUFSZ];
struct sockaddr_can	  addr;
static struct canfd_frame frame;
static struct timeval	  today_tv, log_tv, last_log_tv, diff_tv;
struct timespec		  sleep_ts;
int			  s; /* CAN_RAW socket */
FILE			 *infile	 = stdin;
unsigned long		  gap		 = DEFAULT_GAP;
int			  use_timestamps = 1;
int			  interactive	 = 0; /* wait for ENTER keypress to process next frame */
int			  count		 = 0; /* end replay after sending count frames. 0 = disabled */
static int		  verbose, opt, delay_loops;
static unsigned long	  skipgap;
static int		  loopback_disable = 0;
static int		  infinite_loops   = 0;
static int		  loops		   = DEFAULT_LOOPS;
int			  assignments; /* assignments defined on the commandline */
int			  txidx;       /* sendto() interface index */
int			  eof, txmtu, i, j;
char			 *fret;

[triveria]

Hi,
thanks for the feedback. I've read up on the Linux kernel netdev code formatting rules here and I'm fine with most style recommendations, since I also use them daily.
There are, however, 2 rules which I object / would like to discuss:

Column width: The Linux kernel netdev code formatting rules set the width to 80 chars. This rule originates from punch cards and leads IMHO to unnecessary many line breaks where a long line is more readable. Especially, now that everyone has a widescreen monitor (16:9). Even Linus Torvalds himself strongly objects the 80 character limit here. I therefore propose a modern column width of 200 or something.

Braces after if-clauses with single-line statements: I'm coding for a living in a software company and I've seen way too many bugs happen by this sloppy way of writing if-blocks. It all goes well when people are fully aware of what they are doing and remember that a second indented line will not be guarded by the if-clause. But unfortunately, this is rarely the case. Too often people make improvements to code, add another line to the "if-block" and forget about the missing braces and thus introduce a bug that's also hard to spot. It's simply just asking for trouble. Therefore, I strongly advise against this rule.

On a sidenote, I'm not sure whether the Linux kernel netdev code formatting rules are still up to date, since they advertise the use of gotos although this has beed considered bad practise since forever basically. See Dijkstra's essay A Case against the GO TO Statement.
Furthermore and as noted above, the rules still work with 80 char limit, yet Linus himself is against it. This leaves the impression as if they are only dragged around without being updated.

[marckleinebudde]

I'd like to stick with the current Linux coding style.

The 80 char rule has been deprecated since v5.7-rc7-212-gbdc48fa11e46, which is more than 3 years old, and the kernel comes with a .clang-format file.

[hartkopp]

I'd like to stick with the current Linux coding style.

The 80 char rule has been deprecated since v5.7-rc7-212-gbdc48fa11e46, which is more than 3 years old. The kernel comes with a .clang-format file

Oh, I wasn't aware of the fact that the kernel already has a .clang-format file.

So it was definitely good to wait for your feedback ;-)

[triveria]

Could you please elaborate on what you mean by "current Linux coding style"? Are you referring to the style in canplayer.c on the master branch, or the style dictated by the kernel's .clang-format file? That file still enforces an 80-character line limit, which, when applied to canplayer.c, adds numerous line breaks and, in my view, reduces readability. What are your thoughts on the 80-character limit, braces around single-line if clauses, and the use of gotos? I'm asking because I'm uncertain about the current style guidelines; should I make adjustments to my pull request?

Furthermore, since the bulk of the commits are from you two, you might want to consider crafting a .clang-format file that aligns more with your personal style and the specific needs of this project, rather than strictly adhering to the kernel developers' formatting, who are not directly participating in this endeavor anyway.

[marckleinebudde]

Could you please elaborate on what you mean by "current Linux coding style"?

Basically this document: https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst

The clang-format is not yet enforced by the kernel yet.

What are your thoughts on the 80-character limit, braces around single-line if clauses, and the use of gotos? I'm asking because I'm uncertain about the current style guidelines; should I make adjustments to my pull request?

Basically what the coding-style.rst says, maybe a bit more relaxed with respect to the chars per line limit.

Furthermore, since the bulk of the commits are from you two, you might want to consider crafting a .clang-format file that aligns more with your personal style and the specific needs of this project, rather than strictly adhering to the kernel developers' formatting, who are not directly participating in this endeavor anyway.

I don't see the need to invent a new coding style, I'm pretty happy with the kernel.

[triveria]

I've modified the .clang-format file to reflect the linked style recommendation. Please have a look at it and at canplayer.c which is now formatted according to the .clang-fomat.
Looking forward to your feedback.

[marckleinebudde]

Look good to me. Please squash all changes on the .clang-format in one patch the changes on canplayer.c in another.

[triveria]

Great :)
I've now squashed the changes on .clang-format and canplayer.c into 2 separate commits, one for each.