hokein added a comment.
Thanks for all 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:
> > > 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.
Personally, I don't have a strong opinion, I'm fine with either. Change to a
method of LangOpts.
================
Comment at: clang/lib/Basic/LangOptions.cpp:104
+
+ if (LangStd == LangStandard::lang_unspecified) {
+ // Based on the base language, pick one.
----------------
sammccall wrote:
> 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
I think this is a good idea, there is a similar `getLangKind` method in
`LangStandard`. Moved it to `LangStandard`.
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
