Skip to content

fix: use std::move when pass by value#555

Open
SYaoJun wants to merge 1 commit intoapache:mainfrom
SYaoJun:fix_move
Open

fix: use std::move when pass by value#555
SYaoJun wants to merge 1 commit intoapache:mainfrom
SYaoJun:fix_move

Conversation

@SYaoJun
Copy link

@SYaoJun SYaoJun commented Feb 5, 2026

Hi iceberg-cpp team, I found a clang-tidy warning on my environment.

Copy link
Collaborator

@zhjwpku zhjwpku left a comment

Choose a reason for hiding this comment

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

LGTM

ninja==1.13.0
mkdocs==1.6.1
mkdocs-material==9.6.22
pre-commit==4.5.1
Copy link
Member

Choose a reason for hiding this comment

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

Is this related?

@SYaoJun
Copy link
Author

SYaoJun commented Feb 6, 2026 via email

template <typename R, typename... Args>
struct Trait<R (*)(Args...)> {
using ReturnType = R::value_type;
using ReturnType = typename R::value_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a warning from clang-tidy? I guess that typename may be duplicate here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, clang-tidy suggest that. I think it is more straightforward if added.

Copy link
Contributor

@HuaHuaY HuaHuaY Feb 6, 2026

Choose a reason for hiding this comment

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

According to cppreference(https://en.cppreference.com/w/cpp/language/dependent_name.html), a dependent qualified name is assumed to name a type and no typename is required when a qualified name that appears in type-id, where the smallest enclosing type-id is the type-id in an alias declaration since C++20. Can you show the rule name when you meet clang-tidy warning?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants