dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.
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?
================
Comment at: clang/include/clang/Basic/LangOptions.h:511
+/// s language defaults for the given input language and language standard in
+/// the given LangOptions object.
----------------
Looks like a copy/paste typo in the first word.
================
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);
----------------
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.
================
Comment at: clang/include/clang/Frontend/CompilerInvocation.h:222
- /// Set language defaults for the given input language and
- /// language standard in the given LangOptions object.
- ///
- /// \param Opts - The LangOptions object to set up.
- /// \param IK - The input language.
- /// \param T - The target triple.
- /// \param Includes - The affected list of included files.
- /// \param LangStd - The input language standard.
+ /// This is deprecated. Please use `clang::setLangDefaults` instead.
static void
----------------
I suggest having this comment phrased to explain what it does before saying
what people should do. Something like:
```
lang=c++
/// Forwards to [...].
///
/// TODO: Remove this wrapper once all callers have been updated.
```
================
Comment at: clang/include/clang/Frontend/CompilerInvocation.h:223-226
static void
setLangDefaults(LangOptions &Opts, InputKind IK, const llvm::Triple &T,
std::vector<std::string> &Includes,
LangStandard::Kind LangStd = LangStandard::lang_unspecified);
----------------
I think you should inline the new one-liner implementation into the header,
documenting what people should do instead.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121375/new/
https://reviews.llvm.org/D121375
STAMPS
actor(@dexonsmith) application(Differential) author(@hokein) herald(H75)
herald(H337) herald(H423) herald(H438) herald(H576) herald(H628) herald(H688)
herald(H864) monogram(D121375) object-type(DREV)
phid(PHID-DREV-3totfj2qha3xuifsvz6o) reviewer(@dexonsmith) reviewer(@jdoerfert)
reviewer(@sammccall) revision-repository(rG) revision-status(needs-revision)
subscriber(@cfe-commits) subscriber(@dexonsmith) subscriber(@sstefan1)
tag(#all) tag(#clang) via(web)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
