fix(format): add clang-format to check all c++ files and fix exist fo… #847

SYaoJun · 2026-02-09T13:38:29Z

Reason for this PR

Currently there are some files(.cpp/.h) not incorperate into the graphar-clformat checklist, such as python/rust directories.

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?

Make the pre-commit check all .cpp/.cc/.h/.hpp directly.
Fix all not format files automatically by pre-commit run clang-format -a

Are these changes tested?

Are there any user-facing changes?

SYaoJun · 2026-02-09T13:39:30Z

issue: #845

codecov-commenter · 2026-02-09T13:51:23Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.21%. Comparing base (6c802a1) to head (3cfe001).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #847   +/-   ##
=========================================
  Coverage     80.21%   80.21%           
  Complexity      615      615           
=========================================
  Files            93       93           
  Lines         10257    10257           
  Branches       1049     1049           
=========================================
  Hits           8228     8228           
  Misses         1789     1789           
  Partials        240      240

Flag	Coverage Δ
cpp	`71.52% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

SYaoJun · 2026-02-10T04:10:18Z

please review the code @yangxk1

Copilot

Pull request overview

Updates the repo’s formatting enforcement so clang-format is applied via pre-commit to a broader set of C/C++ sources (beyond the previous limited globbing), and aligns CI to run the same pre-commit clang-format hook. The PR also includes the resulting clang-format-only reformatting across several C++ files (including bindings, JNI, core C++ code, and vendored thirdparty code).

Changes:

Broaden clang-format pre-commit hook matching to all .(cc|cpp|h|hpp) files.
Replace the prior CI clang-format logic with a pre-commit run clang-format --all-files-style execution.
Apply clang-format updates across affected C++ sources (Python bindings, JNI, filesystem, and thirdparty libraries).

Reviewed changes

Copilot reviewed 8 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`python/src/bindings/types_binding.cc`	Formatting-only adjustments to match clang-format output.
`python/src/bindings/high_level_binding.cc`	Large formatting-only reflow of pybind11 bindings.
`maven-projects/java/src/main/cpp/ffi/jni_org_apache_graphar_arrow_ArrowTableStatic.cc`	Formatting-only include/order and wrapping updates.
`cpp/thirdparty/simple-uri-parser/uri_parser.h`	clang-format reflow of vendored URI parser header.
`cpp/thirdparty/mini-yaml/yaml/Yaml.hpp`	clang-format reflow of vendored YAML header.
`cpp/thirdparty/mini-yaml/yaml/Yaml.cpp`	clang-format reflow of vendored YAML implementation (very large diff).
`cpp/src/graphar/filesystem.h`	Formatting-only signature wrapping.
`cpp/src/graphar/filesystem.cc`	Formatting-only wrapping around template specialization/instantiation lines.
`.pre-commit-config.yaml`	Expands clang-format hook scope to all C++ extensions.
`.github/workflows/ci.yml`	Switch CI clang-format step to run via pre-commit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T07:17:48Z

.pre-commit-config.yaml

    hooks:
    - id: clang-format
-      files: ^python/
+      files: \.(cc|cpp|h|hpp)$


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.

Suggested change

files: \.(cc|cpp|h|hpp)$

files: ^cpp/.*\.(cc|cpp|h|hpp)$

exclude: ^cpp/thirdparty/

I prefer to exclude the thirdparty.

Copilot · 2026-02-10T07:17:48Z

cpp/thirdparty/simple-uri-parser/uri_parser.h


+#include <algorithm>
 #include <string>
 #include <unordered_map>


This header uses std::string_view (when C++17+) and std::tuple/std::tie, but it doesn't include <string_view> or ; it also calls std::isalnum/std::tolower/std::strtol without including /. To keep the header self-contained and portable across standard library implementations, add the missing standard includes near the top.

Suggested change

#include <unordered_map>

#include <unordered_map>

#include <string_view>

#include <tuple>

#include <cctype>

#include <cstdlib>

Copilot · 2026-02-10T07:17:49Z

cpp/thirdparty/simple-uri-parser/uri_parser.h

+  auto auth_string = uri.substr(2, pos);
+  auto rem = uri.substr(pos + 2);


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.

Suggested change

auto auth_string = uri.substr(2, pos);

auto rem = uri.substr(pos + 2);

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);

}

Copilot · 2026-02-10T07:17:49Z

cpp/src/graphar/filesystem.cc

 /// template specialization for std::string
 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;


These explicit template instantiation comments say "template specialization for std::string" but the instantiations shown are for IdType (ReadFileToValue and WriteValueToFile). Updating/removing the misleading comments would avoid confusion when maintaining these explicit instantiations.

Copilot · 2026-02-10T07:17:49Z

.github/workflows/ci.yml

+        pre-commit install
+        pre-commit run clang-format -a



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.

Suggested change

pre-commit install

pre-commit run clang-format -a

pre-commit run clang-format --all-files

SYaoJun · 2026-02-10T08:15:35Z

I will exculde the thirdparty directory and update the PR.

…rmat problem

yangxk1

LGTM~ thank you! @SYaoJun

SYaoJun force-pushed the 845_fix_clang_format branch from e88423c to 33d490f Compare February 9, 2026 13:46

SYaoJun force-pushed the 845_fix_clang_format branch 3 times, most recently from 8371dc0 to 31ec000 Compare February 10, 2026 00:36

yangxk1 requested a review from Copilot February 10, 2026 07:09

Copilot started reviewing on behalf of yangxk1 February 10, 2026 07:10 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

SYaoJun force-pushed the 845_fix_clang_format branch from 31ec000 to b404ea6 Compare February 10, 2026 08:19

fix(format): add clang-format to check all c++ files and fix exist fo…

3cfe001

…rmat problem

SYaoJun force-pushed the 845_fix_clang_format branch from b404ea6 to 3cfe001 Compare February 10, 2026 09:17

SYaoJun mentioned this pull request Feb 10, 2026

fix(C++): add .clang-tidy configuration and fix missing brackets #851

Open

yangxk1 approved these changes Feb 10, 2026

View reviewed changes

yangxk1 merged commit 02bd3fe into apache:main Feb 10, 2026
13 checks passed

	files: \.(cc\|cpp\|h\|hpp)$
	files: ^cpp/.*\.(cc\|cpp\|h\|hpp)$
	exclude: ^cpp/thirdparty/

-#include <unordered_map>
+#include <unordered_map>
+#include <string_view>
+#include <tuple>
+#include <cctype>
+#include <cstdlib>

		auto auth_string = uri.substr(2, pos);
		auto rem = uri.substr(pos + 2);

-  auto auth_string = uri.substr(2, pos);
-  auto rem = uri.substr(pos + 2);
+  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);
+  }

	pre-commit install
	pre-commit run clang-format -a
	pre-commit run clang-format --all-files

fix(format): add clang-format to check all c++ files and fix exist fo… #847

fix(format): add clang-format to check all c++ files and fix exist fo… #847

Uh oh!

Conversation

SYaoJun commented Feb 9, 2026

Reason for this PR

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

SYaoJun commented Feb 9, 2026

Uh oh!

codecov-commenter commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SYaoJun commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

SYaoJun Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

SYaoJun commented Feb 10, 2026

Uh oh!

yangxk1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Feb 9, 2026 •

edited

Loading