dexonsmith added a comment.
In D121375#3429023 <https://reviews.llvm.org/D121375#3429023>, @sammccall wrote:
> 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.
Nice, agreed, no need to split up.
================
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:
> > 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, ...)
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
