Skip to content

Conversation

bricknerb
Copy link
Contributor

@bricknerb bricknerb commented Sep 8, 2025

C++ Interop Demo:

// my_number.h

namespace MyNamespace {

class MyNumber {
 public:
  explicit MyNumber(int value) : value_(value) {}
  auto value() const -> int { return value_; }

 private:
  int value_;
};

auto operator+(MyNumber lhs, MyNumber rhs) -> MyNumber;

}  // namespace MyNamespace
// my_number.cpp

#include "my_number.h"

namespace MyNamespace {

auto operator+(MyNumber lhs, MyNumber rhs) -> MyNumber {
  return MyNumber(lhs.value() + rhs.value());
}

}  // namespace MyNamespace
// main.carbon

library "Main";

import Core library "io";
import Cpp library "my_number.h";

fn Run() -> i32 {
  let n1: Cpp.MyNamespace.MyNumber = Cpp.MyNamespace.MyNumber.MyNumber(5);
  Core.Print(n1.value());
  let n2: Cpp.MyNamespace.MyNumber = Cpp.MyNamespace.MyNumber.MyNumber(7);
  Core.Print(n2.value());
  let n3: Cpp.MyNamespace.MyNumber = n1 + n2;
  Core.Print(n3.value());
  return 0;
}

Before this change:

$ bazel-bin/toolchain/carbon compile main.carbon
main.carbon:13:38: error: cannot access member of interface `Core.AddWith(Cpp.MyNamespace.MyNumber)` in type `Cpp.MyNamespace.MyNumber` that does not implement that interface
  let n3: Cpp.MyNamespace.MyNumber = n1 + n2;
                                     ^~~~~~~

With this change:

$ clang -c my_number.cpp
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link my_number.o main.o --output=demo
$ ./demo
5
7
12

Part of #5995.

@bricknerb bricknerb marked this pull request as ready for review September 8, 2025 12:44
@github-actions github-actions bot requested a review from chandlerc September 8, 2025 12:44
@bricknerb bricknerb changed the title C++ interop: Support importing operators defined in namespaces. C++ interop: Support importing operators defined in namespaces Sep 8, 2025
@ivanaivanovska
Copy link
Contributor

LGTM.
A nit suggestion: rewrite the demo example to show the Before state at the top, or make it better visible. I got confused as I thought it was the result of the current compilation.

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Looks good to me too, but I think we should at least add a TODO to use ADL once we can.

@@ -60,9 +78,10 @@ auto BuildUnaryOperator(Context& context, SemIR::LocId loc_id, Operator op,
// the C++ operator.
// TODO: Change impl lookup instead. See
// https://github.com/carbon-language/carbon-lang/blob/db0a00d713015436844c55e7ac190a0f95556499/toolchain/check/operator.cpp#L76
if (IsOfCppClassType(context, operand_id)) {
auto cpp_parent_scope_id = GetCppClassTypeParentScope(context, operand_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call Sema::ArgumentDependentLookup here instead of reimplementing something similar? I think we'd need #5891 to land first, so that we have expressions corresponding to the arguments at the point where we do the lookup.

If you'd prefer to do it this way for the time being, I think that's fine, but please add a TODO to replace this with a call to ArgumentDependentLookup when we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, waiting for #5891.
Added TODOs for now (similar to the one in import_cpp.cpp.

@bricknerb
Copy link
Contributor Author

LGTM. A nit suggestion: rewrite the demo example to show the Before state at the top, or make it better visible. I got confused as I thought it was the result of the current compilation.

Done.

@bricknerb bricknerb enabled auto-merge September 10, 2025 07:15
@bricknerb bricknerb added this pull request to the merge queue Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants