fix(format): add clang-format to check all c++ files and fix exist fo… by SYaoJun · Pull Request #847 · apache/incubator-graphar

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

Merged

yangxk1 approved these changes Feb 10, 2026

View reviewed changes

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

SYaoJun deleted the 845_fix_clang_format branch February 14, 2026 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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
yangxk1 merged 1 commit intoapache:mainfrom
SYaoJun:845_fix_clang_format

SYaoJun commented Feb 9, 2026

Uh oh!

SYaoJun commented Feb 9, 2026

Uh oh!

codecov-commenter commented Feb 9, 2026 •

edited

Loading

Uh oh!

SYaoJun commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

SYaoJun Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

SYaoJun commented Feb 10, 2026

Uh oh!

yangxk1 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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

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!

4 participants

codecov-commenter commented Feb 9, 2026 •

edited

Loading