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

apply updated code style #1196

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Conversation

apolyakov
Copy link
Contributor

No description provided.

@apolyakov apolyakov added the refactoring Logic and code style improvements label Dec 20, 2024
@apolyakov apolyakov added this to the next milestone Dec 20, 2024
@apolyakov apolyakov self-assigned this Dec 20, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

after squash, there will be another sha commit. .git-blame-ignore-revs should be added after the merge of this MR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's added here so you can test it and make sure that git blame continues working

@@ -3,12 +3,12 @@
#include "common/smart_ptrs/intrusive_ptr.h"

#ifndef INCLUDED_FROM_KPHP_CORE
#error "this file must be included only from runtime-core.h"
#error "this file must be included only from runtime-core.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

don't want to enable `IndentPPDirectives'?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for macro indentation

AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: Empty
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should keep it?

ReflowComments: true
SortIncludes: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep it. IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

Includes are sorted implicitly by setting base variant on LLVM, I suppose.

IncludeCategories:
- Regex: '^<'
Priority: 1
IndentCaseLabels: true
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should keep it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mb will we add the formatting script first, and then the new rules?

@@ -3,12 +3,12 @@
#include "common/smart_ptrs/intrusive_ptr.h"

#ifndef INCLUDED_FROM_KPHP_CORE
#error "this file must be included only from runtime-core.h"
#error "this file must be included only from runtime-core.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for macro indentation

ReflowComments: true
SortIncludes: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Includes are sorted implicitly by setting base variant on LLVM, I suppose.

@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch from 3736a14 to 6fcdf6f Compare February 4, 2025 10:44
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch 2 times, most recently from 3418219 to fa623c6 Compare February 4, 2025 14:03
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch from fa623c6 to e2ff8dc Compare February 4, 2025 14:08
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch from 03adca9 to d113357 Compare February 4, 2025 15:16
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch 2 times, most recently from eb36bfa to 4e20e4f Compare February 5, 2025 09:50
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch 9 times, most recently from f17d303 to 948eec1 Compare February 5, 2025 11:56
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch from 948eec1 to 9d63b68 Compare February 5, 2025 12:05
@apolyakov apolyakov force-pushed the apolyakov/update-clang-format branch from 1543212 to 3286da4 Compare February 5, 2025 12:33
contents: read

on:
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a paths section and specify the paths to the folders where we want to check the new code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case.
This will need to be put in another MR so that after adding the MR to .git-blame-ignore-revs we don't lose its history.

@@ -0,0 +1,41 @@
name: "Check code formatting"

permissions:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's that for?

- name: Check for formatting changes
run: |
# Check if any files were modified by clang-format
git diff --exit-code
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be shortened to:
git diff --exit-code || ( echo "Code is not formatted. Please run clang-format." && exit 1 )

:)

@DrDet DrDet modified the milestones: 06.02.2025, next Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Logic and code style improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants