zeromq / zproject

CLASS Project Generator
Mozilla Public License 2.0
149 stars 104 forks source link

Problem: Our default .clang-format complains about code generated by zproject #1070

Open jimklimov opened 6 years ago

jimklimov commented 6 years ago

The .clang-format file initially provided by integration of the style checker in libzmq contains settings that are not compatible with code generated by zproject (e.g. per travis job logs it changes markup to use 2 spaces, does not indent nested C/C++ macros, does not put return type and function name on different lines in the declarations...

Someone more familiar with the tool should amend the configuration file (and the zproject_autotools.gsl script to provide it) so zproject-generated code is style-clean out of the box.

Noticed discrepancies:

Mismatch in style that might be amended on either side (if not dictated by CLASS):

jimklimov commented 6 years ago

CC @sigiesec as our one known resident expert :)

sigiesec commented 6 years ago

Unfortunately, I am a total non-expert for zproject ;)

What is generated by zproject? Does this generate only skeletons that are manually edited, or final files that are not touched manually? In the latter case, perhaps the format checking should just exclude those files.

In the former case... do you suggest changing the .clang-format configuration, or the code generation template? I am not sure if clang-format supports all the formatting options that the generated code currently provides.

Have you got some examples where the generated code does not adhere to the formatting clang-format expects?

jimklimov commented 6 years ago

One example would be here https://travis-ci.org/42ity/fty-prometheus-rest/jobs/351305664 (colourful and slow, or https://api.travis-ci.org/v3/job/351305664/log.txt raw and quick). The suggested 2-space indents surprised me, since the provided config lists "4" in several places...

jimklimov commented 6 years ago

As for zproject - yes, it does generate skeletons which are often edited afterwards (and some files are not) and which try to adhere to a common style. This tool should help keep up that style, not contradict it :)

jimklimov commented 6 years ago

There is also a spec for the zeromq CLASS style (generation of which zproject automates, per advertisement) :

sigiesec commented 6 years ago

Definitely, not everything what is specified by http://zeromq.org/docs:style is supported by clang-format, e.g. inserting spaces before [. I brought this up when we discussed automatic formatting in Brussels. We came to the conclusion that some changes in formatting style are acceptable when this means we can have automatic formatting. Probably, http://zeromq.org/docs:style should be updated to reflect that.

I am pretty sure this also applies to some other points in the diff. In theory, clang-format could be extended to support that, but someone would need to do that (I have no experience in that), and I know that clang-format is somewhat hesitant to add features only relevant for few users, since this increases complexity of the tool permanently. I think, it wouldn't be worth to maintain a fork or a completely separate tool.

On the other hand, some aspects might be solvable by adapting the .clang-format configuration. I could assist with that, but I need to have a closer look, and maybe some more concrete pointers to specific instances of where this would be desirable.

In the end, I think the solution must include running clang-format over the generated code, automatically as part of the generation process. It would be cumbersome to try to incorporate all rules, particularly those involving where line breaks are inserted, into the template or the generator.

jimklimov commented 6 years ago

Yes, I hope the adaptation of configuration file is the way to go (and maybe adaptation of the CLASS spec where unavoidable, and subsequent change of zproject templates to generate according to the new spec - but that path would need a big storm of discussion, so undesirable).

Probably as a first shot goal, a dummy project generated with one class and one main should cleanly pass the check :)

sigiesec commented 6 years ago

To start with, I understood one point from your original comment:

does not put return type and function name on different lines in the declarations

This is controlled by the AlwaysBreakAfterReturnType option, which should be set to All to achieve this.

jimklimov commented 6 years ago

Thanks, per https://travis-ci.org/jimklimov/fty-prometheus-rest/jobs/351334372 this setting seemed to work and even found an issue of that sort in the code :)

A few more to go...

sigiesec commented 6 years ago

Positive:

 FTY_PROMETHEUS_REST_EXPORT ftyprometheusrest_t *
-    ftyprometheusrest_new (void);
+ftyprometheusrest_new (void);

Set IndentWrappedFunctionNames to true.

-        }
-        else {
-            printf ("Unknown option: %s\n", argv [argn]);
+        } else {
+            printf ("Unknown option: %s\n", argv[argn]);

Set BraceWrapping/BeforeElse to false

-struct _ftyprometheusrest_t {
-    int filler;     //  Declare class properties here
+struct _ftyprometheusrest_t
+{
+    int filler; //  Declare class properties here
 };

Set BraceWrapping/AfterStruct to false

Negative:

-        if (strcmp (argv [argn], "--number") == 0
-        ||  strcmp (argv [argn], "-n") == 0) {
+        else if (strcmp (argv[argn], "--number") == 0
+                 || strcmp (argv[argn], "-n") == 0) {

I don't think aligning boolean operators with if is supported by clang-format.

-            puts ("  --continue / -c        continue on exception (on Windows)");
+            puts (
+              "  --continue / -c        continue on exception (on Windows)");

Here, the configured line length was exceeded. If you want longer lines, the line length must be increased.

-static test_item_t
-all_tests [] = {
+static test_item_t all_tests[] = {

I don't think breaking a variable declaration after the type is supported by clang-format.

jimklimov commented 6 years ago

Cool, there are online tools that might help too :