[PATCH] D131258: [Sema] Merge variable template specializations

[Ilya Biryukov via Phabricator via cfe-commits]

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