You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@graphar.apache.org by "SYaoJun (via GitHub)" <gi...@apache.org> on 2026/02/09 13:38:32 UTC
[PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
SYaoJun opened a new pull request, #847:
URL: https://github.com/apache/incubator-graphar/pull/847
<!--
Thanks for contributing to GraphAr.
If this is your first pull request you can find detailed information on [CONTRIBUTING.md](https://github.com/apache/graphar/blob/main/CONTRIBUTING.md)
The Apache GraphAr (incubating) community has restrictions on the naming of pr title. You can find instructions in
[CONTRIBUTING.md](https://github.com/apache/graphar/blob/main/CONTRIBUTING.md#title) too.
-->
### Reason for this PR
<!--
Why are you proposing this change? If this is already tracked in an issue, please link to the issue here.
Explaining clearly why this change is beneficial is important for the reviewers to understand the context.
-->
Currently there are some files(`.cpp/.h`) not incorperate into the graphar-clformat checklist, such as python/rust directories.
```cpp
file(GLOB_RECURSE FILES_NEED_FORMAT "src/graphar/*.h" "src/graphar/*.cc"
"test/*.h" "test/*.cc"
"examples/*.h" "examples/*.cc"
"benchmarks/*.h" "benchmarks/*.cc")
file(GLOB_RECURSE FILES_NEED_LINT "src/graphar/*.h" "src/graphar/*.cc"
"test/*.h" "test/*.cc"
"examples/*.h" "examples/*.cc"
"benchmarks/*.h" "benchmarks/*.cc")
add_custom_target(graphar-clformat
COMMAND clang-format --style=file -i ${FILES_NEED_FORMAT}
COMMENT "Running clang-format."
VERBATIM)
```
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
1. Make the pre-commit check all `.cpp/.cc/.h/.hpp` directly.
2. Fix all not format files automatically by `pre-commit run clang-format -a`
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **BREAKING CHANGE: <description>** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either
(a) a security vulnerability,
(b) a bug that caused incorrect or invalid data to be produced, or
(c) a bug that causes a crash (even when the API contract is upheld).
We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **Critical Fix: <description>** -->
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "SYaoJun (via GitHub)" <gi...@apache.org>.
SYaoJun commented on code in PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#discussion_r2786431376
##########
.pre-commit-config.yaml:
##########
@@ -39,7 +39,7 @@ repos:
rev: v21.1.2
hooks:
- id: clang-format
- files: ^python/
+ files: \.(cc|cpp|h|hpp)$
Review Comment:
I prefer to exclude the thirdparty.
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "SYaoJun (via GitHub)" <gi...@apache.org>.
SYaoJun commented on PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#issuecomment-3876050851
I will exculde the thirdparty directory and update the PR.
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "SYaoJun (via GitHub)" <gi...@apache.org>.
SYaoJun commented on PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#issuecomment-3875208636
please review the code @yangxk1
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#issuecomment-3871849241
## [Codecov](https://app.codecov.io/gh/apache/incubator-graphar/pull/847?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.21%. Comparing base ([`6c802a1`](https://app.codecov.io/gh/apache/incubator-graphar/commit/6c802a1aa64d8a049366638eef4ecf8a1f8ebb19?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`33d490f`](https://app.codecov.io/gh/apache/incubator-graphar/commit/33d490fd133aaabead9991b5f6538f41278b6f6a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #847 +/- ##
============================================
- Coverage 80.21% 80.21% -0.01%
Complexity 615 615
============================================
Files 93 93
Lines 10257 10256 -1
Branches 1049 1049
============================================
- Hits 8228 8227 -1
Misses 1789 1789
Partials 240 240
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-graphar/pull/847?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "SYaoJun (via GitHub)" <gi...@apache.org>.
SYaoJun commented on PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#issuecomment-3871772329
issue: #845
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 merged PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] fix(format): add clang-format to check all c++ files and fix exist fo… [incubator-graphar]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#discussion_r2786208800
##########
cpp/thirdparty/simple-uri-parser/uri_parser.h:
##########
@@ -27,215 +27,222 @@
#ifndef SIMPLE_URI_PARSER_LIBRARY_H
#define SIMPLE_URI_PARSER_LIBRARY_H
+#include <algorithm>
#include <string>
#include <unordered_map>
-#include <algorithm>
#ifndef simple_uri_CPLUSPLUS
-# if defined(_MSVC_LANG ) && !defined(__clang__)
-# define simple_uri_CPLUSPLUS (_MSC_VER == 1900 ? 201103L : _MSVC_LANG )
-# else
-# define simple_uri_CPLUSPLUS __cplusplus
-# endif
+#if defined(_MSVC_LANG) && !defined(__clang__)
+#define simple_uri_CPLUSPLUS (_MSC_VER == 1900 ? 201103L : _MSVC_LANG)
+#else
+#define simple_uri_CPLUSPLUS __cplusplus
+#endif
#endif
-#define simple_uri_CPP17_OR_GREATER ( simple_uri_CPLUSPLUS >= 201703L )
+#define simple_uri_CPP17_OR_GREATER (simple_uri_CPLUSPLUS >= 201703L)
namespace uri {
#if simple_uri_CPP17_OR_GREATER
- using string_view_type = std::string_view;
- using string_arg_type = std::string_view;
- constexpr auto npos = std::string_view::npos;
+using string_view_type = std::string_view;
+using string_arg_type = std::string_view;
+constexpr auto npos = std::string_view::npos;
#else
- using string_view_type = std::string;
- using string_arg_type = const std::string&;
- constexpr auto npos = std::string::npos;
+using string_view_type = std::string;
+using string_arg_type = const std::string&;
+constexpr auto npos = std::string::npos;
#endif
using query_type = std::unordered_map<std::string, std::string>;
enum class Error {
- None,
- InvalidScheme,
- InvalidPort,
+ None,
+ InvalidScheme,
+ InvalidPort,
};
struct Authority {
- std::string authority;
- std::string userinfo;
- std::string host;
- long port = 0;
+ std::string authority;
+ std::string userinfo;
+ std::string host;
+ long port = 0;
};
struct Uri {
- Error error;
- std::string scheme;
- Authority authority = {};
- std::string path;
- query_type query = {};
- std::string query_string;
- std::string fragment;
-
- explicit Uri(Error error) : error(error) {}
- Uri(std::string scheme, Authority authority, std::string path, query_type query, std::string query_string, std::string fragment)
- : error(Error::None)
- , scheme(std::move(scheme))
- , authority(std::move(authority))
- , path(std::move(path))
- , query(std::move(query))
- , query_string(std::move(query_string))
- , fragment(std::move(fragment))
- {}
+ Error error;
+ std::string scheme;
+ Authority authority = {};
+ std::string path;
+ query_type query = {};
+ std::string query_string;
+ std::string fragment;
+
+ explicit Uri(Error error) : error(error) {}
+ Uri(std::string scheme, Authority authority, std::string path,
+ query_type query, std::string query_string, std::string fragment)
+ : error(Error::None),
+ scheme(std::move(scheme)),
+ authority(std::move(authority)),
+ path(std::move(path)),
+ query(std::move(query)),
+ query_string(std::move(query_string)),
+ fragment(std::move(fragment)) {}
};
-}
+} // namespace uri
namespace {
bool valid_scheme(uri::string_arg_type scheme) {
- if (scheme.empty()) {
- return false;
- }
- auto pos = std::find_if_not(scheme.begin(), scheme.end(), [&](char c){
- return std::isalnum(c) || c == '+' || c == '.' || c == '-';
- });
- return pos == scheme.end();
+ if (scheme.empty()) {
+ return false;
+ }
+ auto pos = std::find_if_not(scheme.begin(), scheme.end(), [&](char c) {
+ return std::isalnum(c) || c == '+' || c == '.' || c == '-';
+ });
+ return pos == scheme.end();
}
-std::tuple<std::string, uri::Error, uri::string_view_type> parse_scheme(uri::string_arg_type uri) {
- auto pos = uri.find(':');
- if (pos == uri::npos) {
- return { "", uri::Error::InvalidScheme, uri };
- }
-
- auto scheme = uri.substr(0, pos);
- if (!::valid_scheme(scheme)) {
- return { "", uri::Error::InvalidScheme, uri };
- }
- std::string scheme_string{ scheme };
- std::transform(scheme_string.begin(), scheme_string.end(), scheme_string.begin(),
- [](unsigned char c){ return std::tolower(c); });
-
- return { scheme_string, uri::Error::None, uri.substr(pos + 1) };
+std::tuple<std::string, uri::Error, uri::string_view_type> parse_scheme(
+ uri::string_arg_type uri) {
+ auto pos = uri.find(':');
+ if (pos == uri::npos) {
+ return {"", uri::Error::InvalidScheme, uri};
+ }
+
+ auto scheme = uri.substr(0, pos);
+ if (!::valid_scheme(scheme)) {
+ return {"", uri::Error::InvalidScheme, uri};
+ }
+ std::string scheme_string{scheme};
+ std::transform(scheme_string.begin(), scheme_string.end(),
+ scheme_string.begin(),
+ [](unsigned char c) { return std::tolower(c); });
+
+ return {scheme_string, uri::Error::None, uri.substr(pos + 1)};
}
-std::tuple<uri::Authority, uri::Error, uri::string_view_type> parse_authority(uri::string_arg_type uri) {
- uri::Authority authority;
-
- bool has_authority = uri.length() >= 2 && uri[0] == '/' && uri[1] == '/';
- if (!has_authority) {
- return { authority, uri::Error::None, uri };
- }
-
- auto pos = uri.substr(2).find('/');
- auto auth_string = uri.substr(2, pos);
- auto rem = uri.substr(pos + 2);
- authority.authority = auth_string;
-
- pos = auth_string.find('@');
+std::tuple<uri::Authority, uri::Error, uri::string_view_type> parse_authority(
+ uri::string_arg_type uri) {
+ uri::Authority authority;
+
+ bool has_authority = uri.length() >= 2 && uri[0] == '/' && uri[1] == '/';
+ if (!has_authority) {
+ return {authority, uri::Error::None, uri};
+ }
+
+ auto pos = uri.substr(2).find('/');
+ auto auth_string = uri.substr(2, pos);
+ auto rem = uri.substr(pos + 2);
Review Comment:
parse_authority computes `pos = uri.substr(2).find('/')` and then unconditionally does `rem = uri.substr(pos + 2)`. When no '/' exists (pos == npos), `pos + 2` wraps (size_t) and `rem` becomes an incorrect substring (e.g., `s3://bucket` yields path "/bucket" and later becomes "bucket/bucket"). Handle the npos case explicitly by setting `rem` to an empty string_view/string and taking `auth_string` as the remainder.
```suggestion
uri::string_view_type auth_string;
uri::string_view_type rem;
if (pos == uri::npos) {
auth_string = uri.substr(2);
rem = uri::string_view_type{};
} else {
auth_string = uri.substr(2, pos);
rem = uri.substr(pos + 2);
}
```
##########
.github/workflows/ci.yml:
##########
@@ -87,38 +87,15 @@ jobs:
cd build
cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON -DBUILD_EXAMPLES=ON -DBUILD_BENCHMARKS=ON -DCMAKE_CXX_FLAGS="--coverage" -DCMAKE_C_FLAGS="--coverage"
- - name: Cpp Format and lint
- working-directory: "cpp/build"
+ - name: clang-format
run: |
- # install clang-format
- sudo curl -L https://github.com/muttleyxd/clang-tools-static-binaries/releases/download/master-22538c65/clang-format-8_linux-amd64 --output /usr/bin/clang-format
- sudo chmod +x /usr/bin/clang-format
-
- # validate format
- function prepend() { while read line; do echo "${1}${line}"; done; }
-
- make graphar-clformat
- GIT_DIFF=$(git diff --ignore-submodules)
- if [[ -n $GIT_DIFF ]]; then
- echo "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"
- echo "| clang-format failures found!"
- echo "|"
- echo "$GIT_DIFF" | prepend "| "
- echo "|"
- echo "| Run: "
- echo "|"
- echo "| make graphar-clformat"
- echo "|"
- echo "| to fix this error."
- echo "|"
- echo "| Ensure you are working with clang-format-8, which can be obtained from"
- echo "|"
- echo "| https://github.com/muttleyxd/clang-tools-static-binaries/releases"
- echo "|"
- echo "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"
- exit -1
- fi
+ pip install pre-commit
+ pre-commit install
+ pre-commit run clang-format -a
Review Comment:
`pre-commit install` isn't needed in CI (it only installs git hooks) and can be dropped to reduce side effects and runtime; running `pre-commit run clang-format --all-files` directly is sufficient.
```suggestion
pre-commit run clang-format --all-files
```
##########
cpp/thirdparty/simple-uri-parser/uri_parser.h:
##########
@@ -27,215 +27,222 @@
#ifndef SIMPLE_URI_PARSER_LIBRARY_H
#define SIMPLE_URI_PARSER_LIBRARY_H
+#include <algorithm>
#include <string>
#include <unordered_map>
Review Comment:
This header uses std::string_view (when C++17+) and std::tuple/std::tie, but it doesn't include <string_view> or <tuple>; it also calls std::isalnum/std::tolower/std::strtol without including <cctype>/<cstdlib>. To keep the header self-contained and portable across standard library implementations, add the missing standard includes near the top.
```suggestion
#include <unordered_map>
#include <string_view>
#include <tuple>
#include <cctype>
#include <cstdlib>
```
##########
cpp/src/graphar/filesystem.cc:
##########
@@ -397,7 +397,6 @@ Status FinalizeS3() {
template Result<IdType> FileSystem::ReadFileToValue<IdType>(
const std::string&) const noexcept;
/// template specialization for std::string
-template Status FileSystem::WriteValueToFile<IdType>(const IdType&,
- const std::string&) const
- noexcept;
+template Status FileSystem::WriteValueToFile<IdType>(
+ const IdType&, const std::string&) const noexcept;
Review Comment:
These explicit template instantiation comments say "template specialization for std::string" but the instantiations shown are for IdType (ReadFileToValue<IdType> and WriteValueToFile<IdType>). Updating/removing the misleading comments would avoid confusion when maintaining these explicit instantiations.
##########
.pre-commit-config.yaml:
##########
@@ -39,7 +39,7 @@ repos:
rev: v21.1.2
hooks:
- id: clang-format
- files: ^python/
+ files: \.(cc|cpp|h|hpp)$
Review Comment:
The clang-format hook now matches all C++ extensions repo-wide, which also pulls in vendored code under cpp/thirdparty (this repo appears to intentionally exclude thirdparty from tooling, e.g., the CMake graphar-clformat target and codecov ignore list). Consider excluding cpp/thirdparty (and any other vendored/generated trees) via the hook's `exclude:` or by tightening the `files:` regex, so formatting enforcement stays focused on maintained sources.
```suggestion
files: ^cpp/.*\.(cc|cpp|h|hpp)$
exclude: ^cpp/thirdparty/
```
--
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@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org