You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/23 17:46:24 UTC
[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #64: [refactor] Apply forward declaration as much as possible
BewareMyPower opened a new pull request, #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64
Fixes https://github.com/apache/pulsar-client-cpp/issues/60
### Motivation
The includes in pulsar-client-cpp is very casual. There are a lot of implicit includes and the forward declaration is not used much. For example, if `lib/ClientConnection.h` was modified, 27 files would be recompiled.
The other problem is the `SortIncludes` attribute in `.clang-format` file is false. It might be okay in early days. However, as the project grows, including many headers without ordering brings a very bad experience. Combining with the very few usages of the forward declaration, it's hard to determine whether a header is still required after a change.
### Modifications
Apply forward declarations as much as possible and change the `SortIncludes` to true in `.clang-format` file.
It's special for the `PulsarApi.pb.h` file because the size of this header is over 1 MiB. For classes we can use forward declaration, but for enumerations we have to include this header.
To solve this problem, `ProtoApiEnums.h` is added to define some constant integers that can be cast implicitly from the enumerations. If we want to use enumerations from `PulsarApi.pb.h`, we can include `ProtoApiEnums.h` instead.
Finally, to unify the include rules, in `lib/*.h`, if we want to include a header (e.g. `xxx.h`) from the same directory, just use `#include "xxx.h"`. Don't use `#include "lib/xxx.h"` or `#include <lib/xxx.h>` in headers of `lib/` directory.
### Improvements
Since forward declaration is applied everywhere now, take `lib/ClientConnection.h` for example. After this patch, only 10 files needs to be recompiled, while 27 files would be recompiled before.
This patch also reduces the binary size and speeds up the compilation time.
Binary size:
- `libpulsar.a`: 319234696 (305 MiB) -> 286716564 (274 MiB)
- `libpulsar.so`: 110162496 (106 MiB) -> 102428456 (98 MiB)
Compilation time with the following commands:
```bash
cmake -B build -DBUILD_TESTS=OFF
cmake --build build -j8
```
From 2m35.640s to 1m44.213s.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64#discussion_r1002947201
##########
.clang-format:
##########
@@ -19,7 +19,7 @@
BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 110
-SortIncludes: false
+SortIncludes: true
Review Comment:
https://releases.llvm.org/3.9.0/tools/clang/docs/ClangFormatStyleOptions.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64#discussion_r1002942331
##########
.clang-format:
##########
@@ -19,7 +19,7 @@
BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 110
-SortIncludes: false
+SortIncludes: true
Review Comment:
No. This option is a boolean since the `clang-format` 3.9. It's a newer version of clang that changed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64#issuecomment-1288787275
@merlimat @shibd @RobertIndie @Demogorgon314 Please take a look at this PR in prior to other PRs since it brings many changes. Other PRs should be rebased to it.
Though it's a huge PR, most changes are only related to the header includes, which is definitely good for code quality. The code changes are mostly related to the `ProtoApiEnums.h`. Without it, there would still be many files that have to include `PulsarApi.pb.h`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64#discussion_r1002896777
##########
.clang-format:
##########
@@ -19,7 +19,7 @@
BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 110
-SortIncludes: false
+SortIncludes: true
Review Comment:
Currently the `clang-format` binds to 11.0.0, this option is a boolean value, see https://releases.llvm.org/11.0.1/tools/clang/docs/ClangFormatStyleOptions.html
##########
.clang-format:
##########
@@ -19,7 +19,7 @@
BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 110
-SortIncludes: false
+SortIncludes: true
Review Comment:
Currently the `clang-format` binds to version 11, this option is a boolean value, see https://releases.llvm.org/11.0.1/tools/clang/docs/ClangFormatStyleOptions.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] Demogorgon314 commented on a diff in pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64#discussion_r1002808672
##########
.clang-format:
##########
@@ -19,7 +19,7 @@
BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 110
-SortIncludes: false
+SortIncludes: true
Review Comment:
The value should be `CaseInsensitive` or `CaseSensitive `, see https://clang.llvm.org/docs/ClangFormatStyleOptions.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] Demogorgon314 commented on a diff in pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64#discussion_r1002905767
##########
.clang-format:
##########
@@ -19,7 +19,7 @@
BasedOnStyle: Google
IndentWidth: 4
ColumnLimit: 110
-SortIncludes: false
+SortIncludes: true
Review Comment:
Interesting. Looks like the `clang-format version 11.1.0` changed the option to boolean, and the `clang-format version 14.0.6` changed back.
```
➜ clang-format -version
clang-format version 14.0.6
➜ clang-format -style=llvm -dump-config | grep SortIncludes
SortIncludes: CaseSensitive
➜ clang-format-11 -version
clang-format version 11.1.0
➜ clang-format-11 -style=llvm -dump-config | grep SortIncludes
SortIncludes: true
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat merged pull request #64: [refactor] Apply forward declaration as much as possible
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #64:
URL: https://github.com/apache/pulsar-client-cpp/pull/64
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org