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

Add formatting infrastructure to the project #2505

Merged
merged 3 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
101 changes: 99 additions & 2 deletions .clang-format
Original file line number Diff line number Diff line change
@@ -1,10 +1,107 @@
Language: Cpp
Standard: c++20
ColumnLimit: 100

WhitespaceSensitiveMacros:
# clang format doesn't understand TypeScript, so make sure it doesn't mangle
# overrides and additional definitions
- JSG_TS_OVERRIDE
- JSG_TS_DEFINE
- JSG_STRUCT_TS_OVERRIDE
- JSG_STRUCT_TS_DEFINE
PointerAlignment: Left
ColumnLimit: 100
AllowShortFunctionsOnASingleLine: Empty

# We should have "true" here but adding an include header would result in too many changed lines.
# Once we turn this on we should use IncludeCategories so that things are consistently sorted.
# For example, to protect against brittle headers (a header not including everything its using
# because existing users happened to include a dependency for it), one could use the following
# order:
# local folder files
# project files
# 3p dependency includes
# standard language headers (put kj/ here?)
# system headers
# While this would be ideal it's also important to note that this isn't the (non-documented) style
# that KJ uses, so it may be worth documenting the style & making it consistent.
SortIncludes: false
danlapid marked this conversation as resolved.
Show resolved Hide resolved

AllowShortIfStatementsOnASingleLine: true
AllowShortLoopsOnASingleLine: true

IndentWidth: 2
IndentCaseBlocks: false
IndentCaseLabels: true
PointerAlignment: Left
DerivePointerAlignment: true

# Really "Attach" but empty braces aren't split.
BreakBeforeBraces: Custom
BraceWrapping:
AfterCaseLabel: false
AfterClass: false
AfterControlStatement: Never
AfterEnum: false
AfterFunction: false
AfterNamespace: false
AfterObjCDeclaration: false
AfterStruct: false
AfterUnion: false
AfterExternBlock: false
BeforeCatch: false
BeforeElse: false
BeforeLambdaBody: false
BeforeWhile: false
IndentBraces: false
SplitEmptyFunction: false
SplitEmptyRecord: false
SplitEmptyNamespace: false

Cpp11BracedListStyle: true

AlignAfterOpenBracket: DontAlign
AlignOperands: DontAlign
AlignTrailingComments:
Kind: Always
OverEmptyLines: 0
AlwaysBreakAfterReturnType: None
AlwaysBreakTemplateDeclarations: Yes
BreakStringLiterals: false
BinPackArguments: true
BinPackParameters: false
BracedInitializerIndentWidth: 2
BreakInheritanceList: BeforeColon
ContinuationIndentWidth: 4
IfMacros:
[
"KJ_SWITCH_ONEOF",
"KJ_CASE_ONEOF",
"KJ_IF_MAYBE",
"KJ_IF_SOME",
"CFJS_RESOURCE_TYPE",
]
LambdaBodyIndentation: OuterScope
Macros:
- "KJ_MAP(x,y)=[y](auto x)"
- "JSG_VISITABLE_LAMBDA(x,y,z)=[x,y](z)"
# The WhitespaceSensitiveMacros solution is flaky, adding the following ensures no formatting:
- "JSG_TS_OVERRIDE(x)=enum class"
- "JSG_TS_DEFINE(x)=enum class"
- "JSG_STRUCT_TS_OVERRIDE(x)=enum class"
- "JSG_STRUCT_TS_DEFINE(x)=enum class"
PenaltyReturnTypeOnItsOwnLine: 1000
PackConstructorInitializers: CurrentLine
ReflowComments: false
SpaceBeforeCtorInitializerColon: false
SpaceBeforeInheritanceColon: false
SpaceBeforeParens: ControlStatementsExceptControlMacros
SpaceBeforeRangeBasedForLoopColon: false
SpacesBeforeTrailingComments: 2
---
# Some files with embedded typescript are incorrectly recognized by clang-format as Objective-C
# This is because the C/C++ macro expansion step happens after the language recognition step, so
# when trying to parse the file, the c++ parser fails with incorrect syntax and a fallback to
# the Objective-C parser is used.
# This is a workaround to hide the warning.
# TODO: Remove this once we have a better solution.
Language: ObjC
DisableFormat: true
3 changes: 3 additions & 0 deletions .git-blame-ignore-revs
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
# Apply prettier to the project
0523bf8b36a937348f1bb79eceda2463a5c220b5

# Apply clang-format to the project.
5e8537a77e760c160ace3dfe23ee8c76ee5aeeb3
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ on:
push:
branches:
- main

jobs:
lint:
runs-on: ubuntu-24.04
Expand Down
8 changes: 8 additions & 0 deletions docs/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,11 @@ just prepare
```sh
just compile-commands
```

## Code Formatting

workerd code is automatically formatted by clang-format. Run `python ./tools/cross/format.py` to reformat the code
or use the appropriate IDE extension.
While workerd generally requires llvm 15, formatting requires clang-format-18.

Code formatting is checked before check-in and during `Linting` CI build.
40 changes: 40 additions & 0 deletions githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,43 @@ then
echo -e "To commit anyway, use --no-verify\n"
exit 1
fi

clang_format_check() {
danlapid marked this conversation as resolved.
Show resolved Hide resolved
source "$(dirname -- $BASH_SOURCE)/../tools/unix/find-python3.sh"
PYTHON_PATH=$(get_python3)
if [[ -z "$PYTHON_PATH" ]]; then
echo
echo "python3 is required for formatting and was not found"
echo
echo "ERROR: you must either install python3 and try pushing again or run `git push` with `--no-verify`"
return 1
fi

set +e
$PYTHON_PATH "$(dirname -- $BASH_SOURCE)/../tools/cross/format.py" --check git --staged
EXIT_CODE=$?
set -e
case $EXIT_CODE in
0)
# No lint.
return 0
;;
1)
echo
echo "ERROR: changes staged for commit have lint. Pass '--no-verify' or '-n' to skip."
echo
echo "To fix lint:"
echo " python3 ./tools/cross/format.py"
echo
return 1
;;
2)
echo
echo "ERROR: failed to run format.py, Pass '--no-verify' or '-n' to skip."
echo
return 1
;;
esac
}

clang_format_check
21 changes: 11 additions & 10 deletions src/workerd/api/actor-state-iocontext-test.c++
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ namespace {
using workerd::TestFixture;

bool contains(kj::StringPtr haystack, kj::StringPtr needle) {
return std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end())
!= haystack.end();
return std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end()) !=
haystack.end();
}

class MockActorId : public ActorIdFactory::ActorId {
class MockActorId: public ActorIdFactory::ActorId {
public:
MockActorId(kj::String id) : id(kj::mv(id)) {}
MockActorId(kj::String id): id(kj::mv(id)) {}
kj::String toString() const override {
return kj::str("MockActorId<", id, ">");
}
Expand All @@ -41,6 +41,7 @@ public:
}

virtual ~MockActorId() {};

private:
kj::String id;
};
Expand All @@ -63,9 +64,9 @@ void runBadDeserialization(jsg::Lock& lock, kj::StringPtr expectedId) {

void runBadDeserializationInIoContext(TestFixture& fixture, kj::StringPtr expectedId) {
fixture.runInIoContext(
[expectedId](const workerd::TestFixture::Environment& env) -> kj::Promise<void> {
runBadDeserialization(env.lock, expectedId);
return kj::READY_NOW;
[expectedId](const workerd::TestFixture::Environment& env) -> kj::Promise<void> {
runBadDeserialization(env.lock, expectedId);
return kj::READY_NOW;
});
}

Expand All @@ -88,10 +89,10 @@ KJ_TEST("actor specified with ActorId object") {
kj::Own<ActorIdFactory::ActorId> mockActorId = kj::heap<MockActorId>(kj::str("testActorId"));
Worker::Actor::Id id = kj::mv(mockActorId);
TestFixture fixture(TestFixture::SetupParams{
.actorId = kj::mv(id),
.actorId = kj::mv(id),
});
runBadDeserializationInIoContext(fixture, "actorId = MockActorId<testActorId>;"_kj);
}

} // namespace
} // namespace workerd::api
} // namespace
} // namespace workerd::api
16 changes: 6 additions & 10 deletions src/workerd/api/actor-state-test.c++
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,23 @@ namespace {
jsg::V8System v8System;

struct ActorStateContext: public jsg::Object, public jsg::ContextGlobal {
JSG_RESOURCE_TYPE(ActorStateContext) {
}
JSG_RESOURCE_TYPE(ActorStateContext) {}
};
JSG_DECLARE_ISOLATE_TYPE(ActorStateIsolate, ActorStateContext);

KJ_TEST("v8 serialization version tag hasn't changed") {
jsg::test::Evaluator<ActorStateContext, ActorStateIsolate> e(v8System);
e.getIsolate().runInLockScope([&](ActorStateIsolate::Lock& isolateLock) {
JSG_WITHIN_CONTEXT_SCOPE(isolateLock,
isolateLock.newContext<ActorStateContext>().getHandle(isolateLock),
[&](jsg::Lock& js) {
isolateLock.newContext<ActorStateContext>().getHandle(isolateLock), [&](jsg::Lock& js) {
auto buf = serializeV8Value(isolateLock, isolateLock.boolean(true));

// Confirm that a version header is appropriately written and that it contains the expected
// current version. When the version increases, we need to write a v8 patch that allows it
// to continue writing data at the old version so that we can do a rolling upgrade without
// any bugs caused by old processes failing to read data written by new ones.
KJ_EXPECT(buf[0] == 0xFF);
KJ_EXPECT(buf[1] == 0x0F); // v8 serializer version
KJ_EXPECT(buf[1] == 0x0F); // v8 serializer version

// And this just confirms that the deserializer agrees on the version.
v8::ValueDeserializer deserializer(isolateLock.v8Isolate, buf.begin(), buf.size());
Expand All @@ -61,15 +59,14 @@ KJ_TEST("we support deserializing up to v15") {
jsg::test::Evaluator<ActorStateContext, ActorStateIsolate> e(v8System);
e.getIsolate().runInLockScope([&](ActorStateIsolate::Lock& isolateLock) {
JSG_WITHIN_CONTEXT_SCOPE(isolateLock,
isolateLock.newContext<ActorStateContext>().getHandle(isolateLock),
[&](jsg::Lock& js) {
isolateLock.newContext<ActorStateContext>().getHandle(isolateLock), [&](jsg::Lock& js) {
kj::Vector<kj::StringPtr> testCases;
testCases.add("54");
testCases.add("FF0D54");
testCases.add("FF0E54");
testCases.add("FF0F54");

for (const auto& hexStr : testCases) {
for (const auto& hexStr: testCases) {
auto dataIn = kj::decodeHex(hexStr.asArray());
KJ_EXPECT(deserializeV8Value(isolateLock, "some-key"_kj, dataIn).isTrue());
}
Expand Down Expand Up @@ -104,8 +101,7 @@ KJ_TEST("wire format version does not change deserialization behavior on real da
jsg::test::Evaluator<ActorStateContext, ActorStateIsolate> e(v8System);
e.getIsolate().runInLockScope([&](ActorStateIsolate::Lock& isolateLock) {
JSG_WITHIN_CONTEXT_SCOPE(isolateLock,
isolateLock.newContext<ActorStateContext>().getHandle(isolateLock),
[&](jsg::Lock& js) {
isolateLock.newContext<ActorStateContext>().getHandle(isolateLock), [&](jsg::Lock& js) {
// Read in data line by line and verify that it round trips (serializes and
// then deserializes) back to the exact same data as the input.
std::string hexStr;
Expand Down
Loading
Loading