philnik 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" ||
----------------
aaron.ballman wrote:
> philnik wrote:
> > aaron.ballman wrote:
> > > philnik wrote:
> > > > ChuanqiXu wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > ChuanqiXu wrote:
> > > > > > > > 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.
> > > > > > > > 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
> > > > > > >
> > > > > > > +1, that's my thinking as well.
> > > > > > > The main confusion part to me is that why we connect std modules
> > > > > > > with system paths?
> > > > > >
> > > > > > We consider the system paths to be "special" in that they can do
> > > > > > things "user" paths cannot do. I think we want to keep that model
> > > > > > for modules because of how successful it has been for includes.
> > > > > > (e.g., don't suggest fixits in a system module but do suggest them
> > > > > > for user modules).
> > > > > OK, I got it and it won't be a problem we can't workaround.
> > > > IIUC this would prevent the library from handling the `std` module the
> > > > same as a user module, right? AFAIK the actual use of
> > > > `_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER` is to enable warnings in the
> > > > headers for development, which would not work with the modules with
> > > > this patch, or am I misunderstanding something? Is there a reason this
> > > > isn't a warning that's an error by default? That would allow the
> > > > library to disable it and still serve the same purpose.
> > > > AFAIK the actual use of _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is to
> > > > enable warnings in the headers for development, which would not work
> > > > with the modules with this patch, or am I misunderstanding something?
> > >
> > > Why would the library want a diagnostic telling them they're using a
> > > reserved identifier as a module name?
> > >
> > > > Is there a reason this isn't a warning that's an error by default? That
> > > > would allow the library to disable it and still serve the same purpose.
> > >
> > > It also allows users to produce modules with reserved identifiers. It's
> > > an error that can't be downgraded specifically because I don't think we
> > > want our implementation to give arbitrary users that ability.
> > > > AFAIK the actual use of _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is to
> > > > enable warnings in the headers for development, which would not work
> > > > with the modules with this patch, or am I misunderstanding something?
> > >
> > > Why would the library want a diagnostic telling them they're using a
> > > reserved identifier as a module name?
> >
> > I don't mean specifically this error, I mean more generally that other
> > warnings should be generated from std modules. Treating the headers as
> > system headers disables most warnings, which is the reason libc++ treat
> > them as normal headers in the tests.
> >
> > > > Is there a reason this isn't a warning that's an error by default? That
> > > > would allow the library to disable it and still serve the same purpose.
> > >
> > > It also allows users to produce modules with reserved identifiers. It's
> > > an error that can't be downgraded specifically because I don't think we
> > > want our implementation to give arbitrary users that ability.
> >
> > I think there should be some way to enable normal warnings in the special
> > modules, since it makes the life of library developers a lot easier. I
> > don't care whether that's through disabling a warning or some special sauce
> > to enable warnings from the std module, but there should be some way.
> >
> > I don't mean specifically this error, I mean more generally that other
> > warnings should be generated from std modules. Treating the headers as
> > system headers disables most warnings, which is the reason libc++ treat
> > them as normal headers in the tests.
>
> Ahhh, thank you, that makes a lot more sense to me. :-D
>
> > I think there should be some way to enable normal warnings in the special
> > modules, since it makes the life of library developers a lot easier. I
> > don't care whether that's through disabling a warning or some special sauce
> > to enable warnings from the std module, but there should be some way.
>
> Just like we have `-Wsystem-headers`, I would expect we'd have something
> similar for modules (or reuse it, perhaps with a different name, for both
> headers and modules).
`-Wsystem-headers` doesn't work because that enables warnings in all system
headers, but we only want the warnings from the system library that we write,
i.e. libc++. Or can you somehow control in which system headers warnings are
emitted?
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
