[interop] clang name lookup should find declarations in inline namesp…#40127
[interop] clang name lookup should find declarations in inline namesp…#40127hyp merged 1 commit intoswiftlang:mainfrom
Conversation
|
@swift-ci please test |
There was a problem hiding this comment.
Some suggestions for tests to add:
- Extending an inline namespace in Swift.
- A type with the same name in two inline namespaces (this might be an ODR violation...).
- A function inside of an inline namespace.
- A test for
std.innerand a test forstd.__1.__2.inner. - If it's allowed, an inline namespace that is at the top level.
- (Optional) a namespace inside of an inline namespace inside of a namespace.
There was a problem hiding this comment.
I love this new way to write tests :)
There was a problem hiding this comment.
string.h is a header that may already be in our search paths, so maybe another name is better.
There was a problem hiding this comment.
This doesn't seem like a really necessary part of the test. It's sort of complicated. It might be better to just use simple types/functions.
There was a problem hiding this comment.
Generally we use the TitleCase format for types. I know this is a test... but might as well start off right.
There was a problem hiding this comment.
Can you mark these as the end of the namespaces they close?
test/Interop/Cxx/namespace/inline-namespace-ambiguity-error.swift
Outdated
Show resolved
Hide resolved
|
Build failed |
|
Build failed |
hyp
force-pushed
the
eng/cxx-inline-ns-lookup
branch
from
595871f to
1ede3ef
Compare
|
@swift-ci please test |
|
Build failed |
lib/ClangImporter/ClangImporter.cpp
Outdated
There was a problem hiding this comment.
Did you run into problems with this? Macros can't be members so this should be fine.
There was a problem hiding this comment.
yes, this broke the stdlib build.
There was a problem hiding this comment.
Hmm, that's super interesting. Do you know how it broke the build or what decl it was? I might go investigate after this lands..
There was a problem hiding this comment.
which I believe is a macro
There was a problem hiding this comment.
That's super interesting... so it's a macro defined to a member of a struct. I guess this is the correct way to handle this...
There was a problem hiding this comment.
I wish this could just be a ranges::filter(isa-named-decl) | ranges::filter(...)
zoecarver
left a comment
zoecarver
left a comment
There was a problem hiding this comment.
Thank you for overhauling the test. They look great now!
|
Build failed |
hyp
force-pushed
the
eng/cxx-inline-ns-lookup
branch
from
1ede3ef to
34656fd
Compare
|
@swift-ci please test |
|
Build failed |
hyp
force-pushed
the
eng/cxx-inline-ns-lookup
branch
from
34656fd to
71e9119
Compare
|
@swift-ci please test |
lib/AST/NameLookup.cpp
Outdated
There was a problem hiding this comment.
If you're going to duplicate the code anyway, might as well unconditionally take this path.
There was a problem hiding this comment.
The code isn't duplicated, the regular path does something additional - it marks the name as markLazilyComplete in the table.
There was a problem hiding this comment.
OK, so, if we know that there are no lazy members (i.e., all members are in the lookup table for a given name), then we don't have to emit a namespace lookup request? Makes sense.
zoecarver
left a comment
zoecarver
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the diligent updates, Alex!
|
Build failed |
hyp
force-pushed
the
eng/cxx-inline-ns-lookup
branch
from
71e9119 to
d570826
Compare
|
@swift-ci please test |
hyp
force-pushed
the
eng/cxx-inline-ns-lookup
branch
from
d570826 to
1089959
Compare
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@hyp I don't think a source compatibility check is really necessary here. It seems very unlikely this will break source compatibility as we have few projects that use Objective-C interop and none that use C++ interop. I think the failures you're seeing for source compatibility are failing on main and will be resolved by #40143. Anyway, I think you're good to merge if you want. |
I know, I saw that the same failures are in the baseline :) I was just curious to see what the source compatibility test would do. |
|
I know I just approved this patch, but I also just had a thought: this will crash if someone tries to call a function in an inline namespace from the parent namespace, right? If so, that's really not a great failure mode on our part. Ideally we'd have a diagnostic, but it probably doesn't make sense to spend time adding that, as we want to fix this issue pretty soon. How hard would it be to filter out |
It produces a diagnostic , unless I'm misunderstanding your scenario. One of the test cases covers it. |
Ah, no, you're right. It doesn't crash. I was mis remembering. |
3 participants
…aces