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

Fixed clang configuration file #360

Merged

Conversation

Hadatko
Copy link
Member

@Hadatko Hadatko commented May 15, 2023

  • reformatted code.

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request

I noticed that some clang-format config values were changed for version we have. So i adapt them and added one more which was not available.

To Reproduce

Expected behavior

Screenshots

Desktop (please complete the following information):

  • OS:
  • eRPC Version:

Steps you didn't forgot to do

  • I checked if other PR isn't solving this issue.
  • I read Contribution details and did appropriate actions.
  • PR code is tested.
  • PR code is formatted.
  • Allow edits from maintainers pull request option is set (recommended).

Additional context

+ reformatted code.

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko Hadatko self-assigned this May 15, 2023
@Hadatko Hadatko added this to the 1.11.0 milestone May 15, 2023
@Hadatko Hadatko modified the milestones: 1.11.0, 1.12.0 Jun 1, 2023
@MichalPrincNXP
Copy link
Member

Hi Dusan, thank you. Would it be possible to move to LLVM16 to be aligned with MCUXpressoSDK formatting?

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko
Copy link
Member Author

Hadatko commented Jun 2, 2023

Hi @MichalPrincNXP, does last commit matched your preference?

.clang-format Outdated Show resolved Hide resolved
Copy link

@isolansky isolansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings should not have wrapping with quotes.

The following one is questionable:

BreakInheritanceList: "BeforeComma"

I propose to use any from rest of two values

@Hadatko
Copy link
Member Author

Hadatko commented Oct 16, 2023

Hi @isolansky , i don't understand your first line. If we want to be consistent we can take a look how it looks like in initializing contructor. Newline is before comma. Then we need change both.

image

But personally this is cleaner as comma is aligned with ":"

@isolansky
Copy link

From my understanding, goal is to have code, easily mergable with other projects (under MCUX) or using MSDK in other way.
From my experiences I always got commas at the line end from OSS either from in-company code. So thus asking if there is any other kind of strong motivation for this. If it is just personal feeling, or some big related project.

@Hadatko
Copy link
Member Author

Hadatko commented Oct 16, 2023

I think you are mixing apples and pears ;) We are trying to align C++ code style with NXP code style. But NXP doesn't have C++ code style (afaik) so we should discuss what is best for C++ specific cases. My suggestion looks like block so easier to read.

Anyway as mentioned before. If we do your requested change it would lead to inconsistency with constructor declaration. So we need keep current settings or change both.

(it looks this type of inconsistency is present in webkit. Do you still like Mozilla? They are using my proposed change :D)

image
image
image

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
-- includes will not be sorted at all (not even in groups)
-- Constructutor and inheritance will use AfterColon format style

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko Hadatko force-pushed the feature/updateClangConfig branch 2 times, most recently from 54fc38d to 4038a86 Compare October 18, 2023 13:28
@Hadatko Hadatko force-pushed the feature/updateClangConfig branch 2 times, most recently from 1fe593f to b43796f Compare October 18, 2023 13:45
Update clang-format version

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko
Copy link
Member Author

Hadatko commented Oct 18, 2023

@MichalPrincNXP @isolansky Your requests are placed. I noticed that clang check was not call on PR change so i added this option and moved excluded files list to its own file so we can have one source of excluded files for two different usage of clang format.

@isolansky
Copy link

isolansky commented Oct 19, 2023

Thank you, also for pointed out that include sorting based on grouping. This will be subject of future consideration.

@Hadatko
Copy link
Member Author

Hadatko commented Oct 19, 2023

@isolansky That sounds great. Also in that case you would have to use:
IncludeBlocks: Preserve

…updateClangConfig

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko
Copy link
Member Author

Hadatko commented Oct 25, 2023

FYI: @MichalPrincNXP this should be ready too.

@MichalPrincNXP MichalPrincNXP merged commit 685c527 into EmbeddedRPC:develop Nov 2, 2023
8 checks passed
@MichalPrincNXP
Copy link
Member

thank you, Dusan

@Hadatko Hadatko deleted the feature/updateClangConfig branch November 2, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants