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

Format json-c with clang-format tool #555

Merged
merged 4 commits into from Apr 3, 2020
Merged

Conversation

dota17
Copy link
Member

@dota17 dota17 commented Mar 14, 2020

  1. Add several kinds of Short Blocks/SingleLine option.

AllowShortBlocksOnASingleLine: true
AllowShortCaseLabelsOnASingleLine: true
AllowShortFunctionsOnASingleLine: Empty
AllowShortIfStatementsOnASingleLine: true

  1. The indent effects of IndentWidth and TabWidth will overlay, so do not add TabWidth.

IndentWidth: 8
UseTab: ForIndentation

  1. BreakBeforeBraces: Custom. This is because we should not wrap the extern blocks. like:
// in arraylist.h
#ifdef __cplusplus
extern "C" {
#endif
  1. Partial comments in functions need to be adjusted manually. like:
{
    /*      (formatted)
     *      (adjusted)
     *      (adjusted)
     */     (adjusted)
}
  1. random_seed.c is not suitable for clang-format. So exclude it from the range of this format temporarily.

In a word, the codes of json-c have been well formatted. If we need to add or modify optimization options, we can format the codes on this basis.

@hawicz hawicz changed the title Format json-c with chang-format tool Format json-c with clang-format tool Mar 16, 2020
@hawicz
Copy link
Member

hawicz commented Mar 16, 2020

Does changing the value used for IndentWidth have an effect? I suspect it doesn't but let's make it consistent with ContinuationIndentWidth (i.e. set it to 4) unless that screws something up.

It's too bad we can't just tweak one of the BraceWrapping presets, but oh well.

Let's turn off AllowShortIfStatementsOnASingleLine, it seems to make a lot of the codeflow look inconsistent and IMO somewhat harder to follow.

There are a number of places where that shouldn't be reformatted, please add /* clang-format off */ and /* clang-format on */ comments around these sections:

  • Arrays of constants (e.g. json_type_name) really shouldn't be merged into a single line.
  • The mix and final macros in linkhash.c shouldn't be re-formatted, neither should hashlittle()
  • errno_list[] in strerror_override.c
  • probably others, I haven't had a chance to read through the full diff yet.

@dota17
Copy link
Member Author

dota17 commented Mar 18, 2020

ok, I will modify these optimazation options and block the partial content . But if the IndentWidth is set as 4 and UseTab is set as ForIndentation, things will get worse and I test it many times. Do you mind I set the IndentWidth and ContinuationIndentWidth both are set as 8?

@hawicz
Copy link
Member

hawicz commented Mar 18, 2020

Oh, I see, it defaults to calculating tabs as 8 characters wide. If you set IndentWidth and TabWidth both to 4, then things end up mostly the same, though it wraps lines a bit less often because it calculates the location of the characters after the tab differently.
So, let's keep IndentWidth:8, but please add an explicit TabWidth:8 line.

ContinuationIndentWidth uses spaces, not tabs, to indent the continuation lines (even when indenting 8 characters or more), so let's leave that as ContinuationIndentWidth:4.

@dota17
Copy link
Member Author

dota17 commented Mar 28, 2020

I think the overall clang-formatting effect of this time is pretty good. You may not have the enough time to read the entire file. I add the following key points:

  1. Some statements in json_tokener.c are folded(indented). The numbers of nesting levels of these statements are too big. But I think the formatted results are acceptable. And if needed, we can also set the ColumnLimit as 120 to alleviate the situation.
  2. The hashlittle() in linkhash.c is adjusted/formatted manully and the disabling formatting comments (/* clang-format off/on */ ) is added. Its original basic format is retained. And the other places where that shouldn't be reformatted get handled as the same way. Manual adjustment details can be seen in commit 2d3fc02
  3. random_seed.c has not been formatted. Because code format affects function implementation. This file should be handled separately. I will deal with it later(maybe not this PR).
  4. Is the new format of macros in debug.h suitable?I think it is well.

@dota17
Copy link
Member Author

dota17 commented Apr 2, 2020

hi @hawicz, Are you satisfied with the above formatting results. I think the overall effect is acceptable and I can adjust the format at any time again.

@hawicz
Copy link
Member

hawicz commented Apr 3, 2020

Yes, that all looks good. Looks like this needs to be updated one more time to address the recent conflicts that appeared, but once that's done let's go ahead and merge it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 83.81% when pulling 8b162c4 on dota17:chang_format_3 into ed54353 on json-c:master.

@hawicz hawicz merged commit 31f1ab2 into json-c:master Apr 3, 2020
@dota17
Copy link
Member Author

dota17 commented Apr 3, 2020

ok. The coveralls error should be related to setting of COVERAGE DECREASE THRESHOLD FOR FAILURE. I have set it as 5.0%.

@dota17 dota17 deleted the chang_format_3 branch April 17, 2020 02:26
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

Successfully merging this pull request may close these issues.

None yet

3 participants