erichkeane 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" ||
----------------
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
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
