Page MenuHomePhabricator

[Sema] Merge variable template specializations
ClosedPublic

Authored by ilya-biryukov on Aug 5 2022, 6:44 AM.

Details

Summary

Clang used to produce redefinition errors, see tests for examples.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Aug 5 2022, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 6:44 AM
ilya-biryukov requested review of this revision.Aug 5 2022, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 6:44 AM
inclyc added inline comments.
clang/test/Modules/merge-var-template-spec-cxx-modules.cpp
45 ↗(On Diff #450283)

Missing newline

clang/test/Modules/merge-var-template-spec.cpp
71

Missing newline

inclyc removed a reviewer: inclyc.Aug 6 2022, 12:06 PM

When I run this locally, it emits several unexpected errors:

While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:4:32: error: redefinition of 'zero'
template <class T> constexpr T zero = 0;
                               ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:4:32: note: previous definition is here
template <class T> constexpr T zero = 0;
                               ^
While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:5:8: error: redefinition of 'Int'
struct Int {
       ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:5:8: note: previous definition is here
struct Int {
       ^
While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:8:27: error: redefinition of 'zero<Int>'
template <> constexpr int zero<Int> = {0};
                          ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:8:27: note: previous definition is here
template <> constexpr int zero<Int> = {0};
                          ^
While building module 'library':
In file included from <module-includes>:2:
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var2.h:9:33: error: redefinition of 'zero<T *>'
template <class T> constexpr T* zero<T*> = nullptr;
                                ^
/disk2/workspace.xuchuanqi/llvm-project-for-work/build/tools/clang/test/Modules/Output/merge-var-template-spec.cpp.tmp/var1.h:9:33: note: previous definition is here
template <class T> constexpr T* zero<T*> = nullptr;
                                ^
4 errors generated.
clang/test/Modules/merge-var-template-spec-cxx-modules.cpp
1 ↗(On Diff #450283)

Maybe it is better to rename this as merge-var-template-spec.cppm since we uses the suffix .cppm as a note for c++20 modules.

ChuanqiXu added inline comments.Aug 7 2022, 8:55 PM
clang/test/Modules/merge-var-template-spec-cxx-modules.cpp
18–23 ↗(On Diff #450283)

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.

inclyc added a subscriber: inclyc.Aug 7 2022, 8:57 PM
  • Add the forgotten -fmodules-local-submodule-visibility flag to the test
  • Add newlines to the test files
  • Switch to C++14 mode for tests, they don't use C++17.
ilya-biryukov marked 2 inline comments as done.Aug 8 2022, 5:27 AM

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.

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp
18–23 ↗(On Diff #450283)

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 {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.

ChuanqiXu accepted this revision.Aug 8 2022, 7:08 PM

LGTM then.

clang/test/Modules/merge-var-template-spec-cxx-modules.cpp
18–23 ↗(On Diff #450283)

Oh, you are right. My bad. Sorry for wasting time.

This revision is now accepted and ready to land.Aug 8 2022, 7:08 PM
This revision was landed with ongoing or failed builds.Aug 9 2022, 3:18 AM
This revision was automatically updated to reflect the committed changes.
ilya-biryukov marked 2 inline comments as done.