CodingStyle: add .clang-format and the derived changes by pboettch · Pull Request #394 · stephane/libmodbus

pboettch · 2017-08-07T09:53:39Z

This commit adds a .clang-format-file which helps to automate
the code-formatting. This file is as close as it gets to the existing
style. Some things are not supported by clang-format yet
(mainly PreProcessorIndent - currently under code-review).

This commit does not necessarily needs to be merged, but it
helps to show inconsistencies.

I'm using clang-format-5.0 (5.0.0-svn305653-1) on Debian.

mhei · 2017-08-07T18:29:48Z

src/modbus-rtu.c

    0x44, 0x84, 0x85, 0x45, 0x87, 0x47, 0x46, 0x86, 0x82, 0x42,
-    0x43, 0x83, 0x41, 0x81, 0x80, 0x40
-};
+    0x43, 0x83, 0x41, 0x81, 0x80, 0x40};


Personally, at this point I would prefer having the closing bracket on the next line.
We could also reformat the whole block having 8 or 16 bytes on each line...

There is no way (I know of) telling clang-format to put the closing brace on the next line on intitializers.

Agree with @mhei

mhei · 2017-08-07T18:32:52Z

src/modbus.c

        length = 2 + (nb / 8) + ((nb % 8) ? 1 : 0);
-    }
-        break;
+    } break;


Since this is already "special", I find your version with the break on the same line a bit more difficult to read and understand. I'd prefer to leave it as is...

I'm agree with @mhei too

mhei · 2017-08-07T18:35:28Z

tests/unit-test-client.c

+            BUG_REPORT(_cond, _format, ##__args); \
+            goto close;                           \
+        }                                         \
+    };


I know it's only reformatted, but the trailing semicolon should not be part of define, right?

These kind of defines you normally put between do { ... } while (0)-loops, without the semicolon as you pointed out.

mhei · 2017-08-07T18:36:42Z

tests/unit-test-client.c

        MODBUS_FC_READ_HOLDING_REGISTERS,
-        MODBUS_FC_READ_INPUT_REGISTERS
-    };
+        MODBUS_FC_READ_INPUT_REGISTERS};


Here too, the previous formatting looks better IMHO...

pboettch · 2017-08-07T20:58:10Z

The intention of this commit is to propose a way to impose a common code-formatting all contributors have to respect. A perfect tool does not exists for automation. I actually was surprised that most of the code is coherent. Which is a great sign for this library as it means that code-reviews are really done.

In regards to the tool I chose, it's clang-format because it is integrated into my editor and I can use it to format only a part of the file (the selected lines). What I want to say is, that the .clang-format-file can be present even without changing any line of the existing .c and .h files right now. But future contributions could be indented and aligned with this tool. Should save some time reviewing and integrating.

This commit adds a .clang-format-file which helps to automate the code-formatting. This file is as close as it gets to the existing style. Some things are not supported by clang-format yet (mainly PreProcessorIndent - currently under code-review). This commit does not necessarily needs to be merged, but it helps to show inconsistencies.

pboettch · 2017-08-08T13:21:10Z

I updated my branch to restore the trailing }; on a new line (as it was before).

mhei · 2017-08-08T19:23:52Z

Please don't get me wrong - I'm not familiar with clang-format, so I was not sure whether this could be configured or not. See my comments to express my personal preferences. I agree that having a tool to check/ensure a nice formatting is very helpful and your PR is really appreciated and 👍 for merging it. Maybe it's possible to extend the functionality of clang-format to match the desired coding style for the two corner cases? (I know this could be a long-term goal 😄 )

pboettch · 2017-08-09T07:36:43Z

Don't worry, I didn't get you wrong and, just to be sure, neither I felt offended nor do I want to offend.

There is only coherence and the limits of tools trying to maintain this coherence.

pboettch · 2017-11-01T08:26:17Z

@stephane what about merging this one? Or at least the .clang-format-file. Having one commit will simplify the life of reviewers.

@pboettch

Thank you @pboettch for initial proposal.

@peternewman

* Move malloc before starting unit tests * Fix warning issues When compiling with gcc and option -Wconversion it fixes the warning message warning: conversion from ‘X’ {aka ‘x’} to ‘Y’ {aka ‘y’} may change value Signed-off-by: José Bollo <jose.bollo@iot.bzh> * Add modbus_[get|set]_indication_timeout to doc build * docs: fix simple typo, reponse -> response There is a small typo in tests/bandwidth-client.c. Should read `response` rather than `reponse`. * Update README.md Add Embox RTOS to the list of supported OSes * Install the NEWS and AUTHORS files Fix stephane#513 * Include the test LICENSE in tarball Fix for stephane#542 * Add .clabot * typo * Replace .dir-locals.el (Emacs) by .editorconfig * Fix many typos Thanks to @peternewman * cosmetic changes in man page standardizing itemization * Add FUNDING.yml * Add the baud rate of 256k for Windows (closes stephane#603) * Remove duplicate ';' (closes stephane#602) * Fix position of CC flags in documentation (closes stephane#599) * Replace obsolete AC_PROG_CC_STDC by AC_PROG_CC * Fix typo in comment (closes stephane#588) Thank you @qqq89513 * Add SECURITY.md (closes stephane#613) * modbus_reply: fix copy & paste error in sanity check (fixes stephane#614) While handling MODBUS_FC_WRITE_AND_READ_REGISTERS, both address offsets must be checked, i.e. the read and the write address must be within the mapping range. At the moment, only the read address was considered, it looks like a simple copy and paste error, so let's fix it. Signed-off-by: Michael Heimpold <mhei@heimpold.de> * Bump version to 3.1.7 * Minor changes to NEWS * Fix typos (closes stephane#620) * Add many contributors to .clabot * Minor documentation correction Sorry for the two PRs, I did one then realized I should check the other * Minor documentation correction * Fix network library detection on Haiku * Fix typo on connection (closes stephane#586). Thank you @peternewman. * Add rm5248 to .clabot * Fix comment about EMBUNKEXC (closes stephane#566) This native libmodbus error code is defined but not used. * Fix float endianness issue on big endian architecture. It converts float values depending on what order they come in. This patch was modified from rm5248 [1] [1] synexxus@a511768 * Make sub-command more clear * Swap CRC bytes in request data but not at CRC computing (stephane#397) * address check in single register / coil responses added (stephane#463) Address check in single register / coil responses added According to Modbus standard the address of single register / coils request and response must match Co-authored-by: Heinrich Gsponer <heinrich.gsponer@siemens.com> * Add contributors to .clabot * VCLinkerTool version 1.0.0 to 1.0, NO link error Even the newest visual studio use /VERSION:major[.minor] . It means link.exe /VERSION:1.0 just need two version number . If Version = 1.0.0 , when visual studio link the program , vs will produce link error: ``` 1>"/OUT:C:\Users\Lenovo\Desktop\libmodbus-master\src\win32\modbus.dll" "/VERSION:1.0.0" /INCREMENTAL:NO /NOLOGO ws2_32.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib uuid.lib odbc32.lib odbccp32.lib /MANIFEST "/MANIFESTUAC:level='asInvoker' uiAccess='false'" /manifest:embed /DEBUG "/PDB:C:\Users\Lenovo\Desktop\libmodbus-master\src\win32\modbus.pdb" /MAP /SUBSYSTEM:CONSOLE /TLBID:1 "/IMPLIB:C:\Users\Lenovo\Desktop\libmodbus-master\src\win32\modbus.lib" /MACHINE:X86 /SAFESEH /DLL "C:\Users\Lenovo\Desktop\libmodbus-master\src\win32\/modbus.res" 1>"Debug\modbus-data.obj" 1>"Debug\modbus-rtu.obj" 1>"Debug\modbus-tcp.obj" 1>Debug\modbus.obj 1>LINK : fatal error LNK1117: option“VERSION:1.0.0” grammatical error ``` see: https://docs.microsoft.com/en-us/cpp/build/reference/version-version-information?view=msvc-160 * Add jcarrano to CLA * Update .gitignore * Add build check to CI * Add autogen.sh call * Remove check target (duplicate of distcheck) * Replace Travis CI badge by GitHub CI * Merge NEWS with v3.0.x branch * Fixed MODBUS_ERROR_RECOVERY_LINK not working on Windows. * Fixed a typo error in a comment. * Do not change the behavior for non-windows OS. * Reduce memory use of TCP PI backend (closes stephane#621) - allocate exact memory required to store node and service strings instead of around 1kb of static memory. - accept NULL value of service to use default Modbus port number (502) - unit test updated The new documentation will be updated in another commit. * Rewrite documentation with Material for mkdocs - remove doc build from autotools - don't depend anymore on asciidoc - don't provide man pages anymore - new illustrations - provide mkdocs instructions * Bump version to 3.1.8 * Remove travis.yml * Minor adjustments to MD files * Remove year range in Copyright * Move migration content to libmodbus.org * Avoid negative value in FD_SET call * Test socket against positive value instead of -1 * Add 0x34d to .clabot * Add jordanjohnson56 to CLA * Have autogen.sh fail if autoreconf fails If the autoreconf program does not succeed, autogen.sh should not succeed either. The definition of "succeed" being "exit with exit code 0". * Have autoreconf fail if LT_INIT is not defined If the LT_INIT macro is not available in a *.m4 file to be found during the autoreconf run, it is impossible to build a useful configure script. Therefore, we blacklist the LT_INIT macro so when it appears unexpanded in the configure script, autoreconf will report the failure instead of silently creating a broken configure. This is generally a good idea for any macro not shipped with Automake or Autoconf. * Fix doc links * test the protocol id for 0 previous test would fail if one byte was non zero. * Fix double negative in tests * New quirks handler (closes stephane#38 stephane#533) Useful functions when you are confronted with equipment which does not respect the protocol, which behaves strangely or when you wish to move away from the standard. Thank you @mhei for the great initial version. * Fix bitwise OR in documentation * Improve doc about slave ID in RTU * Add .clang-format and format code (closes stephane#394) Thank you @pboettch for initial proposal. * Remove constraints on baud rate values * Accept IP or device in arg of unit test progs * Avoid compilation issue with VS2022 with strdup * Display created files in configure.js * Use strcpy_s under Windows * Replace inet_addr by inet_pton calls * Replace inet_ntoa by inet_ptop - check conversion - display IP in in IPv6 * Update configure.ac/config.h.win32 for new inet functions * Instructions to build libmodbus in a VS project Tested with Visual Studio 2022. * Updated CLA bot file * Fix connection check for Windows RTU (closes stephane#660, stephane#662) * Add CIFuzz Github action * Convert a few int to unsigned int (stephane#402) * Fix wrong close on recent commit 5a6efec * Adjust fuzzing to run on main branch only * Bump version to 3.1.9 * fix modbus quirks function definition * Split POSIX and Windows RTU connect functions Easier to read * Fix setting baud rate for Linux (partial revert of fa20798) * Remove fuzzing action in waiting for a fix * Bump version to 3.1.10 * docs: small re-wordings, fix various small typos and style issues Signed-off-by: Michael Heimpold <mhei@heimpold.de> * Update CLA * Fix doc of modbus_mapping_new_start_address (stephane#615) * Free addrinfo struct on getaddrinfo() errors * Minor to docs * Add polar.sh to FUNDING * Fix errno value on timeout with TCP connect (closes stephane#736, stephane#756) Thank you kyllingstad for the issue and psychon for the fix. * Update CLA * Log error in read input registers if debug (closes stephane#755) * Add checks for netinet/ip.h and gai_strerror Extracted from stephane#745 * Don't use gai_strerror if not available Identical check to stephane#745 * stephane#675 fix wrong generated version for double number * adding modbus_send_raw_request_tid * doc * doc * Don't use loop initial declaration (closes stephane#752) C99 remains too modern :/ * Minor format * Update docs about memory consumption (closes stephane#715) Not true anymore since stephane#621 * Revert TCP checks for recovery (closes stephane#711) Requires more tests on Windows with serial communications... Related to db1cbc5 and 9b679b7. * Link against socket and nsl libs when building on SunOS On OpenIndiana (SunOS) functions such as bind, socket, listen require -lnsl -lsocket to be given to linker. * Format * Fixes float encoding/decoding for both big and little endian (fixes stephane#665, stephane#694) * Removes useless memcpy calls and no longer used swap32 and swap16 macros. * Updated float encoding functions to consider endianess of 16-bit registers (fixes stephane#665) * Deprecated modbus_get_float() and modbus_set_float() are implemented using cdab variants. * Duplicate in .clabot * Revert "Fixes float encoding/decoding for both big and little endian (fixes stephane#665, stephane#694)" This reverts commit 13bd584. * Remove useless cast in setsockopt call (closes stephane#721) * Update CLA * Fix insecure data handling CID 416366: INTEGER_OVERFLOW found with Coverity Scan. * Add Coverity scan status * Remove duplicate warning flag * Check length passed to modbus_reply (write_bit) The modbus_reply function is designed to receive arguments from modbus_receive. This patch avoid a wrong use of memcpy if the user chooses to inject a bad length argument. Thank you Nozomi Networks Labs Advisory for the report. * Check length passed to modbus_reply (write_register) Related to df79a02. * Small cleanups of unit test server * Rename warning flags variable * Do not build with debug flag active by default * Remove useless configure checks * Update .clabot * Fix request length check in modbus_reply in RTU - rename internal *_prepare_response_tid to *_get_response_tid - change signature, don't need req length anymore - remove misleading modification of req_length - check of req length before use in memcpy for mask write register Related to df79a02 * Revert "Fix float endianness issue on big endian architecture." and fix test suite and setter functions. This reverts commit 49af73d. * Prepare NEWS file for next release * Use port 502 instead of 1502 in documentation (closes stephane#771) The default port number of Modbus is 502 but it requires privileges on Linux. * HAVE_NETINET_IN_H as guard around header (stephane#765) Thank you @diplfranzhoepfinger * RTU - Check CRC before filtering on slave ID * Minor * Bump version to 3.1.11 * modbus-tcp.c doesn't include config.h, possible workaround - defining HAVE_NETINET_IP_H directly in modbus-tcp.c for Linux builds to force including <netinet/ip.h> which provides IPTOS_LOWDELAY definition --------- Signed-off-by: José Bollo <jose.bollo@iot.bzh> Signed-off-by: Michael Heimpold <mhei@heimpold.de> Co-authored-by: Stéphane Raimbault <stephane.raimbault@gmail.com> Co-authored-by: José Bollo <jose.bollo@iot.bzh> Co-authored-by: Tim Gates <tim.gates@iress.com> Co-authored-by: Anton Bondarev <anton.bondarev2310@gmail.com> Co-authored-by: Richard Ash <richard.ash@eatechnology.com> Co-authored-by: Stefan Nilsson <stefannilsson@users.noreply.github.com> Co-authored-by: Marc Haber <mh+mr-git@zugschlus.de> Co-authored-by: Michael Heimpold <mhei@heimpold.de> Co-authored-by: Rick M <rmann@latencyzero.com> Co-authored-by: begasus <begasus@gmail.com> Co-authored-by: SZ Lin (林上智) <szlin@debian.org> Co-authored-by: Mochamad Arifin <klampok.child@gmail.com> Co-authored-by: Pascal Bach <pascal.bach@nextrem.ch> Co-authored-by: Heinrich Gsponer <heinrich.gsponer@siemens.com> Co-authored-by: along <alongL@users.noreply.github.com> Co-authored-by: Stéphane Raimbault <stephane@users.noreply.github.com> Co-authored-by: Mohamed Amine Mzoughi <mmzoughi@jeanlutzsa.com> Co-authored-by: Hans Ulrich Niedermann <hun@n-dimensional.de> Co-authored-by: Jordan Johnson <jordan@midtennpc.com> Co-authored-by: Alexander Polleti <metapsychologe@yahoo.de> Co-authored-by: David Korczynski <david@adalogics.com> Co-authored-by: 20162026 <36726858+20162026@users.noreply.github.com> Co-authored-by: Yau Yimsing <874925685@qq.com> Co-authored-by: Cedric Boudinet <boudinpg@gmail.com> Co-authored-by: Lauri Nurmi <lanurmi@iki.fi> Co-authored-by: Andreas Nicolai <andreas.nicolai@gmx.net> Co-authored-by: Andreas Nicolai <A.Nicolai@uit-gmbh.de> Co-authored-by: diplfranzhoepfinger <mail@franzhoepfinger.de>

mhei reviewed Aug 7, 2017

View reviewed changes

pboettch force-pushed the clang-format branch from 457d215 to 663fba9 Compare August 8, 2017 13:19

stephane closed this in dd45f19 Oct 18, 2022

epsilonrt pushed a commit to epsilonrt/libmodbus that referenced this pull request Jun 19, 2023

Add .clang-format and format code (closes stephane#394)

fb3b831

Thank you @pboettch for initial proposal.

Uh oh!

Conversation

pboettch commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pboettch commented Aug 7, 2017

Uh oh!

pboettch commented Aug 8, 2017

Uh oh!

mhei commented Aug 8, 2017

Uh oh!

pboettch commented Aug 9, 2017

Uh oh!

pboettch commented Nov 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants