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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for all 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:
> > > 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.
> > 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.
> 
> Looking more closely, you're right that initialization is pretty twisty; I don't think it's worth the risk for now.
> 
> In which case, I like the member function approach, even though it makes LangOpts a little less dumb. Something like `LangOpts::setLangDefaults()`, I guess. @hokein, if you'd strongly prefer a free function (what you already have) I'd be fine with that too.
Personally, I don't have a strong opinion, I'm fine with either. Change to a method of LangOpts.


================
Comment at: clang/lib/Basic/LangOptions.cpp:104
+
+  if (LangStd == LangStandard::lang_unspecified) {
+    // Based on the base language, pick one.
----------------
sammccall wrote:
> Pull this out into a separate function `getDefaultLangStandard(Language, const Triple&)`? (No need to expose it unless you want to, though I think it'll be helpful in future).
> 
> It seems bizarre that this depends on the triple, but obviously don't want to change that now
I think this is a good idea, there is a similar `getLangKind` method in `LangStandard`. Moved it to `LangStandard`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121375