ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment.
In D131258#3705503 <https://reviews.llvm.org/D131258#3705503>, @ChuanqiXu wrote: > When I run this locally, it emits several unexpected errors: Sorry, my bad. I was testing this with `-std=c++20` and later switched to `-std=c++17` as this does not have any C++20-related features. I thought I reran the tests with `-std=c++17` before sending out the patch, but apparently I did not. The reason why this worked in C++20 mode is that C++20 implies `-fcxx-modules`, which imply `-fmodules-local-submodule-visibility`. Without the latter flag, all declarations inside the same TU are visible, even when not imported. I have added `-fmodules-local-submodule-visibility` to the tests and also included a comment explaining why we need it. While here, also switched to C++14, which seems enough here. ================ Comment at: clang/test/Modules/merge-var-template-spec-cxx-modules.cpp:18-23 +template <class T> constexpr T zero = 0; // expected-error-re {{declaration{{.*}}in the global module follows declaration in module var_def}} + // expected-note@* {{previous}} +template <> constexpr Int zero<Int> = {0}; // expected-error-re {{declaration{{.*}}in the global module follows declaration in module var_def}} + // expected-note@* {{previous}} +template <class T> constexpr T* zero<T*> = nullptr; // expected-error-re {{declaration{{.*}}in the global module follows declaration in module var_def}} + // expected-note@* {{previous}} ---------------- ChuanqiXu wrote: > BTW, these errors are not expected by me since they are not in a named > modules and they are in different TU with var_def.cppm. > I am not sure about this one, but my understanding of the standard was that no two definitions are allowed if any is attached to a named module. Am I reading the standard wrong here? The relevant bits from my perspective are in [[ https://eel.is/c++draft/basic.def.odr#14.3 | {basic.def.odr}p14.3 ]]: ``` For any definable item D with definitions in multiple translation units, ... - if the definitions in different translation units do not satisfy the following requirements, the program is ill-formed; a diagnostic is required only if the definable item is attached to a named module and a prior definition is reachable at the point where a later definition occurs. ... - Each such definition shall not be attached to a named module ([module.unit]). ``` The diagnostic seems to be required by the standard when looking at a declaration in the named module and this example is the other way around here. However, I don't see why we can't provide a diagnostic in that case too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131258/new/ https://reviews.llvm.org/D131258 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
