[PATCH] D121589: [C++20][Modules][Driver][HU 2/N] Add fmodule-header, fmodule-header=

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.


================
Comment at: clang/docs/ClangCommandLineReference.rst:1183-1185
+.. option:: -fmodule-header=\[user,system\]
+
+Build a C++20 header unit, but search for the header in the user or system header search paths respectively.
----------------
iains wrote:
> iains wrote:
> > tahonermann wrote:
> > > Are "user" and "system" the right terms to use here? Existing options aren't all that consistent. Some examples from `Options.td`:
> > >   def MD : Flag<["-"], "MD">, Group<M_Group>,
> > >       HelpText<"Write a depfile containing user and system headers">;
> > >   def MMD : Flag<["-"], "MMD">, Group<M_Group>,
> > >       HelpText<"Write a depfile containing user headers">;
> > >   ...
> > >   def iquote : JoinedOrSeparate<["-"], "iquote">, Group<clang_i_Group>, Flags<[CC1Option]>,
> > >     HelpText<"Add directory to QUOTE include search path">, MetaVarName<"<directory>">;
> > >   def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>,
> > >     Flags<[CC1Option]>,
> > >     HelpText<"Add directory to SYSTEM include search path">, MetaVarName<"<directory>">;
> > > 
> > > For comparison, the related MSVC options (also present in `Options.td`) are:
> > >   def _SLASH_headerUnitAngle : CLJoinedOrSeparate<"headerUnit:angle">;
> > >   def _SLASH_headerUnitQuote : CLJoinedOrSeparate<"headerUnit:quote">;
> > > 
> > > Should there be a "both" or "any" option so that the search considers all include paths as opposed to just user OR system?
> > > Are "user" and "system" the right terms to use here? Existing options aren't all that consistent. Some examples from `Options.td`:
> > >   def MD : Flag<["-"], "MD">, Group<M_Group>,
> > >       HelpText<"Write a depfile containing user and system headers">;
> > >   def MMD : Flag<["-"], "MMD">, Group<M_Group>,
> > >       HelpText<"Write a depfile containing user headers">;
> > >   ...
> > >   def iquote : JoinedOrSeparate<["-"], "iquote">, Group<clang_i_Group>, Flags<[CC1Option]>,
> > >     HelpText<"Add directory to QUOTE include search path">, MetaVarName<"<directory>">;
> > >   def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>,
> > >     Flags<[CC1Option]>,
> > >     HelpText<"Add directory to SYSTEM include search path">, MetaVarName<"<directory>">;
> > > 
> > > For comparison, the related MSVC options (also present in `Options.td`) are:
> > >   def _SLASH_headerUnitAngle : CLJoinedOrSeparate<"headerUnit:angle">;
> > >   def _SLASH_headerUnitQuote : CLJoinedOrSeparate<"headerUnit:quote">;
> > > 
> > > Should there be a "both" or "any" option so that the search considers all include paths as opposed to just user OR system?
> > 
> > 
> At this time, the objective was to be command-line-option compatible with the existing GCC implementation.  There is a goal to minimise difference in interfaces presented to the user (and build systems).
> 
> Given that the latter [GCC] is already "in the wild", if we wanted terminology-compatibility with MSVC, we could add aliases (to both), I suppose.  I have no axe to grind here - if folks form a consensus that we could have better terminology, I'm happy to add the aliases etc. 
> 
> There is also -fmodule-header ... which means "find the header at the path specified" which can either be 'absolute' (i.e. /xxxx on unix-y systems) or relative to CWD,
> 
> I'm not sure about 'both' or 'any' in the sense of how that would relate to the existing behaviour of header searches (the mechanisms for substituting HUs for textual headers assume that the searches are done with the same process as for the textual).  
> 
> Hopefully, that expresses the intent of the current landed work - that is not to say that the story ends here, but that probably there are other features we want to complete before fine-tuning this.
Ah, I didn't realize gcc already implemented these options. I agree compatibility with it is the more important goal. Looks good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121589/new/

https://reviews.llvm.org/D121589