whisperity added subscribers: rsmith, whisperity. whisperity added a comment.
In D142822#4095896 <https://reviews.llvm.org/D142822#4095896>, @DavidSpickett wrote: > > /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:14:18: > error: reference to 'std' is ambiguous > void f(str<char, std::char_traits<char>> &s) { > ^ > > /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:7:11: > note: candidate found by name lookup is 'std' > namespace std { > ^ This looks like an ouchie of the highest order. I've been looking into Modules... 5-ish years ago, when it was still under design. There are two major concerns here, one is that we've been having a feature called //"Modules"// which was very Clang-specific (but a good proof-of-concept spiritual predecessor of what became the Standard Modules), and there are the standard modules. Now the file name explicitly mentions `cxx20` so this is likely about Standard Modules stuff. But to be fair, I'd take this implementation with a pinch of salt. Due to the lack of build-system support for Modules (there are some murmurs these days on the CMake GitLab about very soon adding meaningful support for this...) we do not really seem to have large-scale real-world experience as to how the standard really could be made working, **especially** in such a weird case like touching `namespace std` which //might// not be standards-compliant in the first place. (But yeah, we are test code here in a compiler; we can cut ourselves some slack.) The code itself in these test files is really weird; it seems the test is centred around the idea that there are: // TU "A" module; // Code in the *global module fragment* namespace std { /* ... */ class basic_string; } export module Whatever; // Code in the public module interface unit of module Whatever. export /* ... */ using str = std::basic_string<...>; So we have a symbol called `str`, which is publicly visible (to TUs importing `Whatever`), and the `namespace std` preceding `Whatever` in `TU "A"` is **not** visible but **reachable**? (The standard's way of expressing reachability is a kind of a mess <http://eel.is/c++draft/module.reach>, so I'd **definitely** try finding the people who worked in developing the actual solution that makes Standard Modules //work// in Clang... call the help of someone who's really expert on the Standard...) Perhaps @rsmith could help us clarify this situation. You can't name `std::basic_string<...>` in the client code (because it is not //visible//), but you can depend on the symbol (e.g., `decltype` it to an alias!) because it is //reachable//! And then you have // TU "B" import Whatever; namespace std { /* ... */ struct char_traits {}; } // use Sb mangling, not Ss, as this is not global-module std::char_traits /* ... */ void f(str<..., std::char_traits<...>>) In which there is "another" (?) `namespace std`, which should(??) also be part of the global module <http://eel.is/c++draft/module#unit-6>: > The //global module// is the collection of all //global-module-fragments// > and all translation units that are not module units. Declarations appearing > in such a context are said to be in the //purview// of the global module. I believe that `TU "B"` is **not** a module unit... But the comment (which is somehow misformatted?) in it would like to say that this `std` is, in fact, //not// part of the global module. So somehow, the changes in this patch are making //both// `std`s part of the same //purview// (whatever that means, really...), thus resulting in ambiguity. - Does this only happen on non-X86? @vabridgers, you could try adding a `--target=` flag locally in the test file to the compiler invocation and re-running the tests to verify. (Might need an LLVM build that is capable of generating code of other architectures.) - My other suggestion would be putting some `->dump()` calls in your change, running the code and observing how the ASTs are looking in the states when execution is within the functions you're trying to change. ================ Comment at: clang/test/AST/ast-dump-traits.cpp:55 // CHECK-NEXT: | `-ExpressionTraitExpr {{.*}} <col:10, col:28> 'bool' __is_lvalue_expr -// CHECK-NEXT: `-FunctionDecl {{.*}} <line:30:1, line:35:1> line:30:6{{( imported)?}} test_unary_expr_or_type_trait 'void ()' +// CHECK-NEXT: -FunctionDecl {{.*}} <line:30:1, line:35:1> line:30:6{{( imported)?}} test_unary_expr_or_type_trait 'void ()' // CHECK-NEXT: `-CompoundStmt {{.*}} <col:38, line:35:1> ---------------- How is this change legit? Is this `FunctionDecl` node just "floating"? If so, why is there a `-` preceding in the textual dump? Usually top-level nodes do not have such a prefix. ================ Comment at: clang/test/AST/fixed_point.c:405 -//CHECK-NEXT: `-VarDecl {{.*}} literallast '_Accum' cinit +//CHECK-NEXT: -VarDecl {{.*}} literallast '_Accum' cinit //CHECK-NEXT: `-FixedPointLiteral {{.*}} '_Accum' 1.0 ---------------- Ditto. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142822/new/ https://reviews.llvm.org/D142822 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
