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