[PATCH] D121375: [clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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)