Skip to content

Refactor ConvertGenericCallExpr#145

Open
lucic71 wants to merge 8 commits into
Cpp2Rust:masterfrom
lucic71:call
Open

Refactor ConvertGenericCallExpr#145
lucic71 wants to merge 8 commits into
Cpp2Rust:masterfrom
lucic71:call

Conversation

@lucic71
Copy link
Copy Markdown
Contributor

@lucic71 lucic71 commented May 23, 2026

ConvertGenericCallExpr is a messy function and it became difficult to add new functoinality on top of it. So I decided to refactor it into: EmitCall(CollectCallInfo(expr)).

In the near future I plan to add 3 new features on top of it:

  1. Don't hoist arguments that are integer literals, they don't alias with any other argument
  2. Don't hoist if all passed arguments are local variables and all have non-reference/non-pointer types, local variables don't alias each other
  3. Translate mapped (variadic) functions that have a direct mapping in libc, for example fcntl -> libc::fcntl

In the new version of ConvertGenericCallExpr, CollectCallInfo is responsible for returning the call information:

  struct CallArg {
    enum class Kind {
      Hoisted,
      Inline,
      Materialized,
    };

    clang::Expr *expr;
    Kind kind;
    std::string param_name;
    clang::QualType param_type;
    bool has_default;
    std::string ref_temp_name;
  };

  struct CallInfo {
    clang::Expr *callee;
    bool is_variadic;
    bool is_fn_ptr_call;
    std::vector<CallArg> args;
    std::vector<clang::Expr *> variadic_args;
  };

And EmitCall is responsible for emitting the hoisted arguments, the callee and the argument list using a CallInfo.

This model makes the addition of new strategies, for example Don't hoist arguments that are integer literals, they don't alias with any other argument, easier because only the CallInfo fields have to be modified.

Comment thread cpp2rust/converter/converter.cpp Outdated
using Kind = CallArg::Kind;

unsigned arg_begin = 0; // skip count for operator()'s implicit object arg
CallInfo info{};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

{} unneded

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dropped

Comment thread cpp2rust/converter/converter.cpp Outdated
}
}

void Converter::EmitCall(CallInfo info) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

passed by value

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed to CalInfo&& info

Comment thread cpp2rust/converter/converter.h Outdated
Materialized,
};

clang::Expr *expr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

usually these are sorted by size (larger to shorter) to reduce struct size.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread cpp2rust/converter/converter.h Outdated
std::optional<TempMaterializationCtx> ConvertCallExpr(clang::CallExpr *expr);

struct CallArg {
enum class Kind {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

: int8_t

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread cpp2rust/converter/converter.cpp Outdated
case Kind::Hoisted:
StrCat("let",
std::format("_{}: {}", ca.param_name, ToString(ca.param_type)),
"=");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

'='

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced with std::format("let {}: {} =")

Comment thread cpp2rust/converter/converter.h Outdated
bool is_variadic;
bool is_fn_ptr_call;
std::vector<CallArg> args;
std::vector<clang::Expr *> variadic_args;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there's only up to one variadic arg. this can be a simple pointer initialized to null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, for example in the following code:

void foo(int a, ...);

foo(1, 2, 3);

=>

args = 1,
variadic_args = 2, 3

And the Rust code is:

fn foo(a: i32, args: &[VaArg])

foo(1, &[2.into(), 3.into()])

std::string param_name;
std::string ref_temp_name;
clang::QualType param_type;
clang::Expr *expr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

switch param_type and expr

Copy link
Copy Markdown
Contributor Author

@lucic71 lucic71 May 25, 2026

Choose a reason for hiding this comment

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

The size of both param_type and expr is 8, I inspected using -fdump-record-layouts. Is there other reason for switching?

Dump of -Xclang -fdump-record-layouts
*** Dumping AST Record Layout
         0 | struct cpp2rust::Converter::CallArg
         0 |   class std::basic_string<char> param_name
         0 |     struct std::basic_string<char>::_Alloc_hider _M_dataplus
         0 |       class std::allocator<char> (base) (empty)
         0 |         class std::__new_allocator<char> (base) (empty)
         0 |       pointer _M_p
         8 |     size_type _M_string_length
        16 |     union std::basic_string<char>::(anonymous at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/basic_string.h:204:7) 
        16 |       char[16] _M_local_buf
        16 |       size_type _M_allocated_capacity
        32 |   class std::basic_string<char> ref_temp_name
        32 |     struct std::basic_string<char>::_Alloc_hider _M_dataplus
        32 |       class std::allocator<char> (base) (empty)
        32 |         class std::__new_allocator<char> (base) (empty)
        32 |       pointer _M_p
        40 |     size_type _M_string_length
        48 |     union std::basic_string<char>::(anonymous at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/basic_string.h:204:7) 
        48 |       char[16] _M_local_buf
        48 |       size_type _M_allocated_capacity
        64 |   class clang::QualType param_type
        64 |     class llvm::PointerIntPair<class llvm::PointerUnion<const class clang::Type *, const class clang::ExtQuals *>, 3> Value
        64 |       struct llvm::detail::PunnedPointer<class llvm::PointerUnion<const class clang::Type *, const class clang::ExtQuals *> > Value
        64 |         unsigned char[8] Data
        72 |   clang::Expr * expr
        80 |   _Bool has_default
        81 |   Kind kind
           | [sizeof=88, dsize=82, align=8,
           |  nvsize=82, nvalign=8]


struct CallInfo {
clang::Expr *callee;
bool is_variadic;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is_variadic isn't equal to !variadic_args.empty()?

Copy link
Copy Markdown
Contributor Author

@lucic71 lucic71 May 25, 2026

Choose a reason for hiding this comment

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

No, a variadic function call be called with 0 variadic arguments, in which case we need to emit an empty &[VaArgs] like this: foo(0, &[])

}
}

void Converter::EmitCall(CallInfo &&info) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

const CallInfo &
The function doesn't consume or take ownership

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It can't be const CallInfo & because EmitHoistedArgs which is called inside EmitCall takes a CallInfo&. But EmitCall can be CallInfo&

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.

2 participants