dexonsmith 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); ---------------- 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. 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