When using a C++ struct as a parameter, map its type to a Carbon class type

[bricknerb]

This doesn't support actually passing the value of the struct, which is planned to be implemented using thunks.

ClangDeclId value is now ClangDecl which includes the mapped Carbon instruction in addition to the Clang declaration. This allows finding the Carbon instruction for a given Clang declaration, which is necessary for mapping a Clang struct parameter type to the Carbon class without doing name lookup. We don't take the instruction as part of the hash key, as discussed in Discord.

To map the type, we also need to map namespaces. To avoid recursion for inner namespaces, we use a vector.

Moved the logic that adds the name to the name scope into ImportNameDeclIntoScope() so it can be used both when importing by name and by decl. However, this technically creates a recursive call since when we import a function, we import its parameter and return types, so while importing the function named declaration we could also call back to import the types named declaration.

Note that the first commit just changes the order of functions in the file to make review easier.

C++ Interop Demo (that shows missing behavior):

// hello_world.h

struct S {
  S(const S&) { x = 1; }
  int x;
};

void hello_world(S s);

// hello_world.cpp

#include "hello_world.h"

#include <cstdio>

void hello_world2(S s) { printf("hello_world2: %d\n", s.x); }

void hello_world(S s) {
  printf("hello_world: %d\n", s.x);
  hello_world2(s);
}

// main.carbon

library "Main";

import Cpp library "hello_world.h";

fn Run() -> i32 {
  var s : Cpp.S;
  Cpp.hello_world(s);
  return 0;
}

$ clang -c hello_world.cpp
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link hello_world.o main.o --output=demo
$ ./demo
hello_world: -1108224096
hello_world2: 1

Part of #5533.

[ivanaivanovska]

remove todo_?

[bricknerb]

Done.

[ivanaivanovska]

Can we add a test for the scope? (e.g. the name not found in the scope)

[bricknerb]

We no longer do name lookup.

[geoffromer]

Can this be a "hidden friend", like CarbonHashtableEq?

[bricknerb]

Done.
FWIW, I was following the example of EntityName.
https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/sem_ir/entity_name.h#L22-L50

[geoffromer]

I think that's just a historical accident -- equality and hashing were added in separate PRs by separate authors.

[geoffromer]

Hashing and equality comparison should operate on the full logical state of the object, and inst_id really seems like it's part of the logical state of ClangDecl. See also style guide, which says:

... don't define operator overloads just because other libraries expect them. For example, if your type doesn't have a natural ordering, but you want to store it in a std::set, use a custom comparator rather than overloading <."

I think the more general principle it's getting at is that fundamental operations like equality and hashing should be defined based on the meaning of the type as a whole, and not contorted to fit one particular use case. Excluding inst_id from hashing and equality feels like such a contortion to me.

Would it work to have a separate Map<clang::Decl*, ClangDeclId> for performing the lookup that you're trying to support here?

[bricknerb]

The general principal makes sense to me, even though I'm not sure the style guide for operator overloads applies here.
I'm fine with a separate mapping, using a custom hashing was proposed here, which also makes sense to me if it's considered an ok practice in Carbon toolchain. Sorry for not providing this context before. Feel free to continue the discussion in Discord or here. I'm fine with both approaches.

[geoffromer]

The general principal makes sense to me, even though I'm not sure the style guide for operator overloads applies here.

I agree that the style rule doesn't directly apply here; I was just citing it for the general principle.

I'm fine with a separate mapping, using a custom hashing was proposed here, which also makes sense to me if it's considered an ok practice in Carbon toolchain. Sorry for not providing this context before. Feel free to continue the discussion in Discord or here. I'm fine with both approaches.

As far as I know, this way of using a custom hash isn't an established practice in our codebase (at least, I can't find any existing examples of it). I've moved the discussion to Discord.