urnathan added inline comments.
================
Comment at: clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp:5
// CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global
-// CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
+// CHECK-DAG: @inline_var_exported = available_externally {{(dso_local
)?}}global
// CHECK-DAG: @const_var_exported = available_externally {{(dso_local
)?}}constant i32 3,
----------------
ChuanqiXu wrote:
> urnathan wrote:
> > I don;t think this is correct. That should still be a linkonce odr,
> > otherwise you'll get conflicts with other module implementation units.
> It is still linkonce_odr in the module it get defined. See the new added test
> case: inline-variable-in-module.cpp for example. The attribute
> `available_externally` is equivalent to external from the perspective of
> linker. See https://llvm.org/docs/LangRef.html#linkage-types. According to
> [dcl.inline]p7, inline variable attached to named module should be defined in
> that domain. Note that the variable attached to global module fragment and
> private module fragment shouldn't be accessed outside the module, so it
> implies that all the variable defined in the module could only be defined in
> the module unit itself.
There's a couple of issues with this. module.cppm is emitting a (linkonce)
definition of inlne_var_exported, but only because it itself is ODR-using that
variable. If you take out the ODR-use in noninline_exported, there is no
longer a symbol emitted.
But, even if you forced inline vars to be emitted in their defining-module's
interface unit, that would be an ABI change. inline vars are emitted whereever
ODR-used. They have no fixed home TU. Now, we could alter the ABI and allow
interface units to define a home location for inline vars and similar entities
(eg, vtables for keyless classes). But we'd need buy-in from other compilers
to do that.
FWIW such a discussion did come up early in implementing modules-ts, but we
decided there was enough going on just getting the TS implemented. I'm fine
with revisiting that, but it is a more significant change.
And it wouldn't apply to (eg) templated variables, which of course could be
instantiated anywhere.
================
Comment at: clang/test/CodeGenCXX/static-variable-in-module.cpp:2-8
+// RUN: mkdir %t
+// RUN: echo "struct S { S(); };" >> %t/foo.h
+// RUN: echo "static S s = S();" >> %t/foo.h
+// RUN: %clang -std=c++20 -I%t %s -S -emit-llvm -o - | FileCheck %s
+module;
+#include "foo.h"
+export module m;
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > urnathan wrote:
> > > rather than generate a foo.h file, why not (ab)use the preprocessor with
> > > internal line directives?
> > >
> > > ```
> > > module;
> > > # 3 __FILE__ 1 // use the next physical line number here (and below)
> > > struct S { S(); };
> > > static S s = S();
> > > # 6 "" 2
> > > export module m;
> > > ...
> > > ```
> > Yeah, the form is useful when we need to add expected-* diagnostic message
> > to GMF. But I feel it is a little bit hacker. I guess the form mimics looks
> > like user more wouldn't be worse personally.
> Ok, I admit this is really helpful and time saving : ) (Now all my new test
> cases would be wrote in this form)
The other thing you can do, which I've seen elsewhere, is an Input subdirectory
and put the #include file there. I prefer the direct encoding, 'cos that's
less indirection when trying to understand the testcase :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119409/new/
https://reviews.llvm.org/D119409
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
