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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D121375#3429023 <https://reviews.llvm.org/D121375#3429023>, @sammccall wrote:

> I only see one usage in-tree (and one more in clspv). And migration is very easy. I think you should do it all in this commit.

Nice, agreed, no need to split up.



================
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);
----------------
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, ...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121375