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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
