ChuanqiXu added inline comments.
================
Comment at: clang/lib/Sema/SemaModule.cpp:282
+ StringRef FirstComponentName = Path[0].first->getName();
+ if (!getSourceManager().isInSystemHeader(Path[0].second) &&
+ (FirstComponentName == "std" ||
----------------
erichkeane wrote:
> ChuanqiXu wrote:
> > aaron.ballman wrote:
> > > ChuanqiXu wrote:
> > > > cor3ntin wrote:
> > > > > ChuanqiXu wrote:
> > > > > > std modules should be irreverent with system headers; The intuition
> > > > > > of the wording should be that the users can't declare modules like
> > > > > > `std` or `std.compat` to avoid possible conflicting. The approach I
> > > > > > imaged may be add a new compilation flags (call it `-fstd-modules`)
> > > > > > now. And if the compiler found a `std` module declaration without
> > > > > > `-fstd-modules`, emit an error.
> > > > > >
> > > > > > For now, I think we can skip the check for `-fstd-modules` and add
> > > > > > it back when we starts to support std modules actually.
> > > > > The idea is that standard modules are built from system
> > > > > directories... it seems a better heuristic than adding a flag for the
> > > > > purpose of 1 diagnostics ( maybe some other system library could in
> > > > > theory export std with no warning, but I'm not super worried about
> > > > > that being a concern in practice)
> > > > > The idea is that standard modules are built from system directories...
> > > >
> > > > This is not true. For example, if someday libc++ supports std modules,
> > > > then we need to build the std modules in our working directory, which
> > > > is not system directories. And **ideally**, we would distribute and
> > > > install module file in the system directories. But it is irreverent
> > > > with the path of the source file.
> > > > then we need to build the std modules in our working directory, which
> > > > is not system directories.
> > >
> > > `-isystem`, pragmas, and linemarkers are all ways around that -- I don't
> > > think we need a feature flag for this, unless I'm misunderstanding
> > > something.
> > Although it may be a little bit nit picker, the module unit which declares
> > the std modules won't be header. It is a module unit. So it requires we
> > implement std modules by wrapping linemarkers around the std modules
> > declaration, which looks a little bit overkill.
> >
> > And another point is that maybe we need to introduce another feature flags
> > to implement std modules. Although I tried to implement std modules within
> > the current features, I can't prove we can implement std modules in that
> > way in the end of the day.
> >
> > Let me add some more words. The standards require us to implement std
> > modules without deprecating the system headers. This strategy leads the
> > direction to "implement the components in the original headers and control
> > their visibility in the std module unit".
> > It may look like:
> >
> > ```
> > //--- std.cppm
> > module;
> > #include <algorithm>
> > ...
> > export module std;
> > ```
> >
> > Then how can control the visibility? In my original experiments, I use the
> > style:
> >
> > ```
> > //--- std.cppm
> > module;
> > #include <algorithm>
> > ...
> > export module std;
> > export namespace std {
> > using std::sort;
> > }
> > ```
> >
> > but this doesn't always work. For example, we can't `using` a in-class
> > friend definition. And there are more reasons, the unreachable declarations
> > in the global module fragment (the section from `module;` to `export module
> > [module_name]`) can be discarded to reduce the size of the module file. And
> > the reachable rules are complex. But the simple story is that it is highly
> > possible the a lot of necessary declarations in global module fragment in
> > the above snippet would be discarded so that the user can't use std modules
> > correctly. I mean, we **may** need a special feature flag for it. And the
> > method with `system headers` looks not good and semantics are not so
> > consistency.
> >
> >
> IMO, any such additional flag (say `-isystem-module`) should ALSO use the
> `isInSystemHeader` infrastructure. I suspect nearly every place we use
> `isInSystemHeader` we also mean to exclude a system-module as well.
>
> I think that any such flag can/should be added later as you figure out how it
> should be specified/work. That said, when you do so, it should either also
> feed `isInSystemHeader`, or basically every use of `isInSystemHeader` should
> ALSO changed to use the new flag as well
The main confusion part to me is that why we connect `std modules` with system
paths? I know implementors can work around the check like the tests did. But
what's the point? I know every header of libcxx contains:
```
#ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
# pragma GCC system_header
#endif
```
but it is for the compatibility with GCC. And it looks not so meaningful to
force the implementation of modules to keep such contraints.
================
Comment at: clang/test/Modules/reserved-names-1.cpp:33
+ expected-error {{module declaration must
occur at the start of the translation unit}}
+
+// Show that being in a system header doesn't save you from diagnostics about
----------------
We lack a test for `foo.std`;
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136953/new/
https://reviews.llvm.org/D136953
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
