Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 of 7 tasks
jimklimov opened this issue Mar 9, 2018 · 11 comments
Open
1 of 7 tasks

Comments

@jimklimov
Copy link
Member

jimklimov commented Mar 9, 2018

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:

  • Return type on one line, function name on another
  • Indentation of macro-code (nested ifdef logic, etc.)
  • Style check insists on indenting 2 spaces rather than 4 in classes, structs, etc.
  • Indentation of a second line when declaring a function in headers (but not source files) per CLASS is like this:
PROJECTNAME_EXPORT void
    funcname(...);

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

  • Pointer characters & * glued to data type (zproject) or var name (clang-format)?
  • Spaces between funcname and parentheses for its arguments (clang-format adds some)
  • Indentation and line breakup about else followed by braces, or by if without braces, or other code without braces...
@jimklimov
Copy link
Member Author

CC @sigiesec as our one known resident expert :)

@sigiesec
Copy link
Member

sigiesec commented Mar 9, 2018

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@sigiesec
Copy link
Member

sigiesec commented Mar 9, 2018

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
Copy link
Member Author

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
Copy link
Member

sigiesec commented Mar 9, 2018

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
Copy link
Member Author

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
Copy link
Member

sigiesec commented Mar 9, 2018

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
Copy link
Member Author

Cool, there are online tools that might help too :

jimklimov added a commit to jimklimov/zproject that referenced this issue Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants