The function will be moved from clangFrontend to clangBasic, which
allows tools (clang pseudoparser)which don't depend on clangFrontend to use
this function.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This makes sense to me! A few comments inline.
Also, the description doesn't talk about timeline for removing the wrapper. Ideally we wouldn't leave behind the wrapper behind... just long enough that you can migrate the callers and delete the old function in separate incremental commit(s) (or if there are very few callers, all in this commit would be fine, but I'm guessing that's not the case here). Or were you thinking something else?
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
561 | Looks like a copy/paste typo in the first word. | |
569–571 | I think this would be cleaner as: 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. | |
clang/include/clang/Frontend/CompilerInvocation.h | ||
221–222 | I think you should inline the new one-liner implementation into the header, documenting what people should do instead. | |
221–222 | I suggest having this comment phrased to explain what it does before saying what people should do. Something like: /// Forwards to [...]. /// /// TODO: Remove this wrapper once all callers have been updated. |
+1 to Duncan's comments, and a couple of nits while here.
Otherwise LG, will be nice to use this without pulling in the grab-bag that is Frontend.
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.
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
567 | while here: this param is non-obvious and this comment doesn't clarify much. | |
569–571 | 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. | |
clang/lib/Basic/LangOptions.cpp | ||
221 | 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 |
Nice, agreed, no need to split up.
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
569–571 |
@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.)
Maybe it's better to return by value in either case to remove the inout, since it seems unnecessary: 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: namespace clang { LangOpts getLangDefaults(Language Lang, const llvm::Triple &T, ...); } (I'm probably almost indifferent at this point, after thinking about the triple, ...) |
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
569–571 |
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. |
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
569–571 |
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. |
address comments;
move to LangOpts::setLangDefaults;
pull out a getNameKind in LangStandard;
remove the old API in CompilerInvocation;
Thanks for all comments!
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
569–571 | Personally, I don't have a strong opinion, I'm fine with either. Change to a method of LangOpts. | |
clang/lib/Basic/LangOptions.cpp | ||
221 | I think this is a good idea, there is a similar getLangKind method in LangStandard. Moved it to LangStandard. |
clang/include/clang/Basic/LangStandard.h | ||
---|---|---|
141 | this makes it sound like a simple getter/lookup, which it absolutely is not! getDefaultLanguageStandard? And separate it out from the simple getters? (The meaningless distinction in the other function names between "standard" and "kind", and the "lang" abbreviation are both pretty unfortunate here, we don't need to fix them but let's not propagate them further) |
Looks like a copy/paste typo in the first word.