Skip to content

[GLUTEN-11287][VL] Use IWYU tool to check code format#11287

Open
xinghuayu007 wants to merge 1 commit intoapache:mainfrom
xinghuayu007:my/iwyu_format
Open

[GLUTEN-11287][VL] Use IWYU tool to check code format#11287
xinghuayu007 wants to merge 1 commit intoapache:mainfrom
xinghuayu007:my/iwyu_format

Conversation

@xinghuayu007
Copy link
Copy Markdown
Contributor

@xinghuayu007 xinghuayu007 commented Dec 12, 2025 •
edited by github-actions bot
Loading

What changes are proposed in this pull request?

Use IWYU tool to check code format

How was this patch tested?

Related issue: #11287

@xinghuayu007 xinghuayu007 changed the title [GLUTEN-][Velox] Use IWYU tool to check code format [GLUTEN-11287][Velox] Use IWYU tool to check code format Dec 12, 2025
@xinghuayu007 xinghuayu007 force-pushed the my/iwyu_format branch 8 times, most recently from d4c97f3 to 6e2680a Compare December 12, 2025 09:35
@philo-he philo-he changed the title [GLUTEN-11287][Velox] Use IWYU tool to check code format [GLUTEN-11287][VL] Use IWYU tool to check code format Dec 12, 2025
philo-he
philo-he reviewed Dec 12, 2025
View reviewed changes
@xinghuayu007 xinghuayu007 force-pushed the my/iwyu_format branch 4 times, most recently from a51b2eb to 4ac46bc Compare December 12, 2025 10:26
FelixYBW
FelixYBW reviewed Dec 14, 2025
View reviewed changes
@xinghuayu007 xinghuayu007 force-pushed the my/iwyu_format branch 11 times, most recently from 1884480 to ba200ee Compare December 15, 2025 07:29
@xinghuayu007 xinghuayu007 force-pushed the my/iwyu_format branch 23 times, most recently from bbef1fc to 6aaab1a Compare December 17, 2025 09:46
philo-he
philo-he reviewed Dec 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@philo-he philo-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we enable IWYU in a way embed in cmake? We can enable the check only when code changes are made under cpp/ path.

if(ENABLE_IWYU)
    find_program(IWYU_PATH NAMES include-what-you-use iwyu)
    if(IWYU_PATH)
        set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
    endif()
endif()

@xinghuayu007
Copy link
Copy Markdown
Contributor Author

xinghuayu007 commented Jan 4, 2026

Can we enable IWYU in a way embed in cmake? We can enable the check only when code changes are made under cpp/ path.

if(ENABLE_IWYU)
    find_program(IWYU_PATH NAMES include-what-you-use iwyu)
    if(IWYU_PATH)
        set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
    endif()
endif()

Compiling include-what-you-use needs clang and source code. Currently design can ensure only check modified cpp files.

philo-he
philo-he reviewed Jan 20, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@philo-he philo-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. The current implementation looks a bit complex to me. Do you know how IWYU is enabled in Velox?

philo-he
philo-he reviewed Jan 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@philo-he philo-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments. Thanks.

dev/builddeps-veloxbe.sh Outdated
mkdir build
cd build

if [ $USE_CLANG = "ON" ]; then
Copy link
Copy Markdown
Member

@philo-he philo-he Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems no need to add a new build option. Just let the compilation depend on the setting for CC and CXX in the env.

Copy link
Copy Markdown
Member

@philo-he philo-he Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To separate the builds, we can run ./dev/builddeps-veloxbe.sh build_velox xxx with gcc to utilize vcpkg cache in docker. Then, run ./dev/builddeps-veloxbe.sh build_gluten_cpp with clang to build Gluten c++ code.

@weiting-chen weiting-chen mentioned this pull request Mar 25, 2026
Open
60 tasks
@philo-he
Copy link
Copy Markdown
Member

philo-he commented Mar 27, 2026

Errors reported by CI.

/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
/__w/gluten/gluten/cpp/velox/jni/VeloxJniWrapper.cc:1042:24: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
 1042 |       otherTables[t] = std::move(hashTableBuilders[t]->uniqueTable());
      |                        ^
/__w/gluten/gluten/cpp/velox/jni/VeloxJniWrapper.cc:1042:24: note: remove std::move call here
 1042 |       otherTables[t] = std::move(hashTableBuilders[t]->uniqueTable());
      |                        ^~~~~~~~~~                                   ~
1 error generated.
[57/94] Building CXX object velox/CMakeFiles/velox.dir/memory/VeloxMemoryManager.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
[58/94] Building CXX object velox/CMakeFiles/velox.dir/memory/VeloxColumnarBatch.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
[59/94] Building CXX object velox/CMakeFiles/velox.dir/operators/functions/RegistrationAllFunctions.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
ninja: build stopped: subcommand failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhouyuan zhouyuan zhouyuan left review comments

@philo-he philo-he philo-he left review comments

@FelixYBW FelixYBW FelixYBW left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

BUILD INFRA VELOX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xinghuayu007 @philo-he @zhouyuan @FelixYBW