[PATCH] D121375: [clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.h:519-521
+void setLangDefaults(LangOptions &Opts, Language Lang, const llvm::Triple &T,
+                     std::vector<std::string> &Includes,
+                     LangStandard::Kind LangStd);
----------------
dexonsmith wrote:
> sammccall wrote:
> > dexonsmith wrote:
> > > I think this would be cleaner as:
> > > ```
> > > lang=c++
> > > class LangOpts {
> > > // ...
> > >   void setDefaults(Language Lang, const llvm::Triple &T, ...);
> > > };
> > > ```
> > > Or `setLangDefaults` or `setDefaultsFor` (I don't care about the name, just feel like it makes more sense as a member function if we're updating all the callers anyway).
> > > 
> > > Also, you should include a default for `LangStd` or it'll be hard to migrate over callers.
> > I kind of like the idea that this logic is "layered above" the langopts struct itself. On the other hand making it a member makes it more discoverable and less surprising that LangOptions is actually an inout param (e.g. IncludeDefaultHeader). Either way is fine with me.
> > I kind of like the idea that this logic is "layered above" the langopts struct itself. 
> 
> @sammccall, I'm curious if you have reasoning for the preference to layer it above; is it because it takes the `Triple`, or is it something more general? (If it's because of the triple, I agree that makes the layering a bit odd.)
> 
> > On the other hand making it a member makes it more discoverable and less surprising that LangOptions is actually an inout param (e.g. IncludeDefaultHeader).
> 
> Maybe it's better to return by value in either case to remove the inout, since it seems unnecessary:
> ```
> lang=c++
> class LangOpts {
> // ...
>   static LangOpts getDefaults(Language Lang, const llvm::Triple &T, ...);
> };
> ```
> 
> If you still prefer a free function, I'd be happy enough with something like this:
> ```
> lang=c++
> namespace clang {
> LangOpts getLangDefaults(Language Lang, const llvm::Triple &T, ...);
> }
> ```
> (I'm probably almost indifferent at this point, after thinking about the triple, ...)
> @sammccall, I'm curious if you have reasoning for the preference to layer it above; is it because it takes the Triple, or is it something more general?

It's more about compiler defaults being an application-level concern where LangOptions is more of a dumb struct. But that's also an argument for keeping it in Frontend, and we don't want that for practical reasons (it's nice to use the lexer on real code without Frontend!). So I'm not sure I have a coherent argument here, I'm happy with any option.

Return by value sounds great, unfortunately the existing code in Frontend calls this in the *middle* of initializing LangOpts from various sources, so it would imply some bigger/riskier changes I guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121375