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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
