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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

+1 to Duncan's comments, and a couple of nits while here.
Otherwise LG, will be nice to use this without pulling in the grab-bag that is Frontend.

In D121375#3428123 <https://reviews.llvm.org/D121375#3428123>, @dexonsmith wrote:

> Also, the description doesn't talk about timeline for removing the wrapper. Ideally we wouldn't leave behind the wrapper behind... just long enough that you can migrate the callers and delete the old function in separate incremental commit(s) (or if there are very few callers, all in this commit would be fine, but I'm guessing that's not the case here). Or were you thinking something else?

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.



================
Comment at: clang/include/clang/Basic/LangOptions.h:517
+/// \param T - The target triple.
+/// \param Includes - The affected list of included files.
+/// \param LangStd - The input language standard.
----------------
while here: this param is non-obvious and this comment doesn't clarify much.
Maybe "If the language requires extra headers to be implicitly included, they will be appended to this list"


================
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:
> 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.


================
Comment at: clang/lib/Basic/LangOptions.cpp:104
+
+  if (LangStd == LangStandard::lang_unspecified) {
+    // Based on the base language, pick one.
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121375