ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
LGTM if we add a test for `foo.std`.
================
Comment at: clang/docs/ReleaseNotes.rst:351
+ export declaration. Both are diagnosed as an error, but the diagnostic is
+ suppressed for use of reserved names in a system header.
----------------
It reads odd about `system header`. But I can't get better name now (I guess we
don't have one now). I think we need to update the
StandardCPlusCplusModules.rst documents in a new section. I'll take it.
================
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:
> 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.
================
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
----------------
aaron.ballman wrote:
> ChuanqiXu wrote:
> > We lack a test for `foo.std`;
> reserved-named-2.cpp has that test (it uses `std0` instead of `std`). Is that
> sufficient?
I feel better with `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
