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

2022-04-13 Thread Haojian Wu via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG93471e65df48: [clang] NFC, move 
CompilerInvocation::setLangDefaults to LangOptions.h (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D121375?vs=422412&id=422423#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121375/new/

https://reviews.llvm.org/D121375

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/LangStandard.h
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Basic/LangOptions.cpp
  clang/lib/Basic/LangStandards.cpp
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3173,178 +3173,6 @@
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
-void CompilerInvocation::setLangDefaults(LangOptions &Opts, InputKind IK,
- const llvm::Triple &T,
- std::vector &Includes,
- LangStandard::Kind LangStd) {
-  // Set some properties which depend solely on the input kind; it would be nice
-  // to move these to the language standard, and have the driver resolve the
-  // input kind + language standard.
-  //
-  // FIXME: Perhaps a better model would be for a single source file to have
-  // multiple language standards (C / C++ std, ObjC std, OpenCL std, OpenMP std)
-  // simultaneously active?
-  if (IK.getLanguage() == Language::Asm) {
-Opts.AsmPreprocessor = 1;
-  } else if (IK.isObjectiveC()) {
-Opts.ObjC = 1;
-  }
-
-  if (LangStd == LangStandard::lang_unspecified) {
-// Based on the base language, pick one.
-switch (IK.getLanguage()) {
-case Language::Unknown:
-case Language::LLVM_IR:
-  llvm_unreachable("Invalid input kind!");
-case Language::OpenCL:
-  LangStd = LangStandard::lang_opencl12;
-  break;
-case Language::OpenCLCXX:
-  LangStd = LangStandard::lang_openclcpp10;
-  break;
-case Language::CUDA:
-  LangStd = LangStandard::lang_cuda;
-  break;
-case Language::Asm:
-case Language::C:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  // The PS4 uses C99 as the default C standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnu99;
-  else
-LangStd = LangStandard::lang_gnu17;
-#endif
-  break;
-case Language::ObjC:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  LangStd = LangStandard::lang_gnu11;
-#endif
-  break;
-case Language::CXX:
-case Language::ObjCXX:
-#if defined(CLANG_DEFAULT_STD_CXX)
-  LangStd = CLANG_DEFAULT_STD_CXX;
-#else
-  LangStd = LangStandard::lang_gnucxx14;
-#endif
-  break;
-case Language::RenderScript:
-  LangStd = LangStandard::lang_c99;
-  break;
-case Language::HIP:
-  LangStd = LangStandard::lang_hip;
-  break;
-case Language::HLSL:
-  LangStd = LangStandard::lang_hlsl2021;
-  break;
-}
-  }
-
-  const LangStandard &Std = LangStandard::getLangStandardForKind(LangStd);
-  Opts.LangStd = LangStd;
-  Opts.LineComment = Std.hasLineComments();
-  Opts.C99 = Std.isC99();
-  Opts.C11 = Std.isC11();
-  Opts.C17 = Std.isC17();
-  Opts.C2x = Std.isC2x();
-  Opts.CPlusPlus = Std.isCPlusPlus();
-  Opts.CPlusPlus11 = Std.isCPlusPlus11();
-  Opts.CPlusPlus14 = Std.isCPlusPlus14();
-  Opts.CPlusPlus17 = Std.isCPlusPlus17();
-  Opts.CPlusPlus20 = Std.isCPlusPlus20();
-  Opts.CPlusPlus2b = Std.isCPlusPlus2b();
-  Opts.GNUMode = Std.isGNUMode();
-  Opts.GNUCVersion = 0;
-  Opts.HexFloats = Std.hasHexFloats();
-  Opts.ImplicitInt = Std.hasImplicitInt();
-  Opts.WChar = Std.isCPlusPlus();
-  Opts.Digraphs = Std.hasDigraphs();
-
-  Opts.HLSL = IK.getLanguage() == Language::HLSL;
-
-  // Set OpenCL Version.
-  Opts.OpenCL = Std.isOpenCL();
-  if (LangStd == LangStandard::lang_opencl10)
-Opts.OpenCLVersion = 100;
-  else if (LangStd == LangStandard::lang_opencl11)
-Opts.OpenCLVersion = 110;
-  else if (LangStd == LangStandard::lang_opencl12)
-Opts.OpenCLVersion = 120;
-  else if (LangStd == LangStandard::lang_opencl20)
-Opts.OpenCLVersion = 200;
-  else if (LangStd == LangStandard::lang_opencl30)
-Opts.OpenCLVersion = 300;
-  else if (LangStd == LangStandard::lang_openclcpp10)
-Opts.OpenCLCPlusPlusVersion = 100;
-  else if (LangStd == LangStandard::lang_openclcpp2021)
-Opts.OpenCLCPlusPlusVersion = 202100;
-  else if (LangStd == LangStandard::lang_hlsl2015)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2015;
-  else if (LangStd == LangStandard::lang_hlsl2016)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2016;
-  else if (LangStd == L

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

2022-04-13 Thread Haojian Wu via Phabricator via cfe-commits
hokein updated this revision to Diff 422412.
hokein marked an inline comment as done.
hokein added a comment.

getDefaultLanguageStandard.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121375/new/

https://reviews.llvm.org/D121375

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/LangStandard.h
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Basic/LangOptions.cpp
  clang/lib/Basic/LangStandards.cpp
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3173,178 +3173,6 @@
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
-void CompilerInvocation::setLangDefaults(LangOptions &Opts, InputKind IK,
- const llvm::Triple &T,
- std::vector &Includes,
- LangStandard::Kind LangStd) {
-  // Set some properties which depend solely on the input kind; it would be nice
-  // to move these to the language standard, and have the driver resolve the
-  // input kind + language standard.
-  //
-  // FIXME: Perhaps a better model would be for a single source file to have
-  // multiple language standards (C / C++ std, ObjC std, OpenCL std, OpenMP std)
-  // simultaneously active?
-  if (IK.getLanguage() == Language::Asm) {
-Opts.AsmPreprocessor = 1;
-  } else if (IK.isObjectiveC()) {
-Opts.ObjC = 1;
-  }
-
-  if (LangStd == LangStandard::lang_unspecified) {
-// Based on the base language, pick one.
-switch (IK.getLanguage()) {
-case Language::Unknown:
-case Language::LLVM_IR:
-  llvm_unreachable("Invalid input kind!");
-case Language::OpenCL:
-  LangStd = LangStandard::lang_opencl12;
-  break;
-case Language::OpenCLCXX:
-  LangStd = LangStandard::lang_openclcpp10;
-  break;
-case Language::CUDA:
-  LangStd = LangStandard::lang_cuda;
-  break;
-case Language::Asm:
-case Language::C:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  // The PS4 uses C99 as the default C standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnu99;
-  else
-LangStd = LangStandard::lang_gnu17;
-#endif
-  break;
-case Language::ObjC:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  LangStd = LangStandard::lang_gnu11;
-#endif
-  break;
-case Language::CXX:
-case Language::ObjCXX:
-#if defined(CLANG_DEFAULT_STD_CXX)
-  LangStd = CLANG_DEFAULT_STD_CXX;
-#else
-  LangStd = LangStandard::lang_gnucxx14;
-#endif
-  break;
-case Language::RenderScript:
-  LangStd = LangStandard::lang_c99;
-  break;
-case Language::HIP:
-  LangStd = LangStandard::lang_hip;
-  break;
-case Language::HLSL:
-  LangStd = LangStandard::lang_hlsl2021;
-  break;
-}
-  }
-
-  const LangStandard &Std = LangStandard::getLangStandardForKind(LangStd);
-  Opts.LangStd = LangStd;
-  Opts.LineComment = Std.hasLineComments();
-  Opts.C99 = Std.isC99();
-  Opts.C11 = Std.isC11();
-  Opts.C17 = Std.isC17();
-  Opts.C2x = Std.isC2x();
-  Opts.CPlusPlus = Std.isCPlusPlus();
-  Opts.CPlusPlus11 = Std.isCPlusPlus11();
-  Opts.CPlusPlus14 = Std.isCPlusPlus14();
-  Opts.CPlusPlus17 = Std.isCPlusPlus17();
-  Opts.CPlusPlus20 = Std.isCPlusPlus20();
-  Opts.CPlusPlus2b = Std.isCPlusPlus2b();
-  Opts.GNUMode = Std.isGNUMode();
-  Opts.GNUCVersion = 0;
-  Opts.HexFloats = Std.hasHexFloats();
-  Opts.ImplicitInt = Std.hasImplicitInt();
-  Opts.WChar = Std.isCPlusPlus();
-  Opts.Digraphs = Std.hasDigraphs();
-
-  Opts.HLSL = IK.getLanguage() == Language::HLSL;
-
-  // Set OpenCL Version.
-  Opts.OpenCL = Std.isOpenCL();
-  if (LangStd == LangStandard::lang_opencl10)
-Opts.OpenCLVersion = 100;
-  else if (LangStd == LangStandard::lang_opencl11)
-Opts.OpenCLVersion = 110;
-  else if (LangStd == LangStandard::lang_opencl12)
-Opts.OpenCLVersion = 120;
-  else if (LangStd == LangStandard::lang_opencl20)
-Opts.OpenCLVersion = 200;
-  else if (LangStd == LangStandard::lang_opencl30)
-Opts.OpenCLVersion = 300;
-  else if (LangStd == LangStandard::lang_openclcpp10)
-Opts.OpenCLCPlusPlusVersion = 100;
-  else if (LangStd == LangStandard::lang_openclcpp2021)
-Opts.OpenCLCPlusPlusVersion = 202100;
-  else if (LangStd == LangStandard::lang_hlsl2015)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2015;
-  else if (LangStd == LangStandard::lang_hlsl2016)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2016;
-  else if (LangStd == LangStandard::lang_hlsl2017)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2017;
-  else if (LangStd == LangStandard::lang_hlsl2018)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2018;
-  els

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

2022-04-11 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM once @sammccall 's new comments are addressed!


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


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

2022-04-11 Thread Sam McCall via Phabricator via cfe-commits
sammccall accepted this revision.
sammccall added inline comments.



Comment at: clang/include/clang/Basic/LangStandard.h:141
   static Kind getLangKind(StringRef Name);
+  static Kind getLangKind(clang::Language Lang, const llvm::Triple &T);
+

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)


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


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

2022-04-08 Thread Haojian Wu via Phabricator via cfe-commits
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 &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


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

2022-04-08 Thread Haojian Wu via Phabricator via cfe-commits
hokein updated this revision to Diff 421494.
hokein marked 2 inline comments as done.
hokein added a comment.

address comments;
move to LangOpts::setLangDefaults;
pull out a getNameKind in LangStandard;
remove the old API in CompilerInvocation;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121375/new/

https://reviews.llvm.org/D121375

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/LangStandard.h
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Basic/LangOptions.cpp
  clang/lib/Basic/LangStandards.cpp
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3175,176 +3175,6 @@
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
-void CompilerInvocation::setLangDefaults(LangOptions &Opts, InputKind IK,
- const llvm::Triple &T,
- std::vector &Includes,
- LangStandard::Kind LangStd) {
-  // Set some properties which depend solely on the input kind; it would be nice
-  // to move these to the language standard, and have the driver resolve the
-  // input kind + language standard.
-  //
-  // FIXME: Perhaps a better model would be for a single source file to have
-  // multiple language standards (C / C++ std, ObjC std, OpenCL std, OpenMP std)
-  // simultaneously active?
-  if (IK.getLanguage() == Language::Asm) {
-Opts.AsmPreprocessor = 1;
-  } else if (IK.isObjectiveC()) {
-Opts.ObjC = 1;
-  }
-
-  if (LangStd == LangStandard::lang_unspecified) {
-// Based on the base language, pick one.
-switch (IK.getLanguage()) {
-case Language::Unknown:
-case Language::LLVM_IR:
-  llvm_unreachable("Invalid input kind!");
-case Language::OpenCL:
-  LangStd = LangStandard::lang_opencl12;
-  break;
-case Language::OpenCLCXX:
-  LangStd = LangStandard::lang_openclcpp10;
-  break;
-case Language::CUDA:
-  LangStd = LangStandard::lang_cuda;
-  break;
-case Language::Asm:
-case Language::C:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  // The PS4 uses C99 as the default C standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnu99;
-  else
-LangStd = LangStandard::lang_gnu17;
-#endif
-  break;
-case Language::ObjC:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  LangStd = LangStandard::lang_gnu11;
-#endif
-  break;
-case Language::CXX:
-case Language::ObjCXX:
-#if defined(CLANG_DEFAULT_STD_CXX)
-  LangStd = CLANG_DEFAULT_STD_CXX;
-#else
-  LangStd = LangStandard::lang_gnucxx14;
-#endif
-  break;
-case Language::RenderScript:
-  LangStd = LangStandard::lang_c99;
-  break;
-case Language::HIP:
-  LangStd = LangStandard::lang_hip;
-  break;
-case Language::HLSL:
-  LangStd = LangStandard::lang_hlsl2021;
-  break;
-}
-  }
-
-  const LangStandard &Std = LangStandard::getLangStandardForKind(LangStd);
-  Opts.LangStd = LangStd;
-  Opts.LineComment = Std.hasLineComments();
-  Opts.C99 = Std.isC99();
-  Opts.C11 = Std.isC11();
-  Opts.C17 = Std.isC17();
-  Opts.C2x = Std.isC2x();
-  Opts.CPlusPlus = Std.isCPlusPlus();
-  Opts.CPlusPlus11 = Std.isCPlusPlus11();
-  Opts.CPlusPlus14 = Std.isCPlusPlus14();
-  Opts.CPlusPlus17 = Std.isCPlusPlus17();
-  Opts.CPlusPlus20 = Std.isCPlusPlus20();
-  Opts.CPlusPlus2b = Std.isCPlusPlus2b();
-  Opts.GNUMode = Std.isGNUMode();
-  Opts.GNUCVersion = 0;
-  Opts.HexFloats = Std.hasHexFloats();
-  Opts.ImplicitInt = Std.hasImplicitInt();
-
-  Opts.HLSL = IK.getLanguage() == Language::HLSL;
-
-  // Set OpenCL Version.
-  Opts.OpenCL = Std.isOpenCL();
-  if (LangStd == LangStandard::lang_opencl10)
-Opts.OpenCLVersion = 100;
-  else if (LangStd == LangStandard::lang_opencl11)
-Opts.OpenCLVersion = 110;
-  else if (LangStd == LangStandard::lang_opencl12)
-Opts.OpenCLVersion = 120;
-  else if (LangStd == LangStandard::lang_opencl20)
-Opts.OpenCLVersion = 200;
-  else if (LangStd == LangStandard::lang_opencl30)
-Opts.OpenCLVersion = 300;
-  else if (LangStd == LangStandard::lang_openclcpp10)
-Opts.OpenCLCPlusPlusVersion = 100;
-  else if (LangStd == LangStandard::lang_openclcpp2021)
-Opts.OpenCLCPlusPlusVersion = 202100;
-  else if (LangStd == LangStandard::lang_hlsl2015)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2015;
-  else if (LangStd == LangStandard::lang_hlsl2016)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2016;
-  else if (LangStd == LangStandard::lang_hlsl2017)
-Opts.HLSLVersion = (unsigned)LangOptions::HLSL_2017;
-  else if (LangStd == LangStandard::lang_hlsl2018)
-Opts.HLSLVersion = (unsig

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

2022-04-06 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits
dexonsmith added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.h:519-521
+void setLangDefaults(LangOptions &Opts, Language Lang, const llvm::Triple &T,
+ std::vector &Includes,
+ LangStandard::Kind LangStd);

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.


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


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

2022-04-06 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.h:519-521
+void setLangDefaults(LangOptions &Opts, Language Lang, const llvm::Triple &T,
+ std::vector &Includes,
+ LangStandard::Kind LangStd);

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.


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


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

2022-04-05 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits
dexonsmith added a comment.

In 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 &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


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

2022-04-05 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment.

+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.

In D121375#3428123 , @dexonsmith 
wrote:

> 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?

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.




Comment at: clang/include/clang/Basic/LangOptions.h:517
+/// \param T - The target triple.
+/// \param Includes - The affected list of included files.
+/// \param LangStd - The input language standard.

while here: this param is non-obvious and this comment doesn't clarify much.
Maybe "If the language requires extra headers to be implicitly included, they 
will be appended to this list"



Comment at: clang/include/clang/Basic/LangOptions.h:519-521
+void setLangDefaults(LangOptions &Opts, Language Lang, const llvm::Triple &T,
+ std::vector &Includes,
+ LangStandard::Kind LangStd);

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.



Comment at: clang/lib/Basic/LangOptions.cpp:104
+
+  if (LangStd == LangStandard::lang_unspecified) {
+// Based on the base language, pick one.

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


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


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

2022-04-04 Thread 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 &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 &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


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

2022-04-01 Thread Haojian Wu via Phabricator via cfe-commits
hokein added a comment.

in case you missed that, friendly ping :)


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


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

2022-03-10 Thread Haojian Wu via Phabricator via cfe-commits
hokein created this revision.
hokein added a reviewer: sammccall.
Herald added a subscriber: dexonsmith.
Herald added a project: All.
hokein requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.
Herald added a project: clang.

The function will be moved from clangFrontend to clangBasic, which
allows tools (clang pseudoparser)which don't depend on clangFrontend to use
this function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121375

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Basic/LangOptions.cpp
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3111,149 +3111,7 @@
  const llvm::Triple &T,
  std::vector &Includes,
  LangStandard::Kind LangStd) {
-  // Set some properties which depend solely on the input kind; it would be nice
-  // to move these to the language standard, and have the driver resolve the
-  // input kind + language standard.
-  //
-  // FIXME: Perhaps a better model would be for a single source file to have
-  // multiple language standards (C / C++ std, ObjC std, OpenCL std, OpenMP std)
-  // simultaneously active?
-  if (IK.getLanguage() == Language::Asm) {
-Opts.AsmPreprocessor = 1;
-  } else if (IK.isObjectiveC()) {
-Opts.ObjC = 1;
-  }
-
-  if (LangStd == LangStandard::lang_unspecified) {
-// Based on the base language, pick one.
-switch (IK.getLanguage()) {
-case Language::Unknown:
-case Language::LLVM_IR:
-  llvm_unreachable("Invalid input kind!");
-case Language::OpenCL:
-  LangStd = LangStandard::lang_opencl12;
-  break;
-case Language::OpenCLCXX:
-  LangStd = LangStandard::lang_openclcpp10;
-  break;
-case Language::CUDA:
-  LangStd = LangStandard::lang_cuda;
-  break;
-case Language::Asm:
-case Language::C:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  // The PS4 uses C99 as the default C standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnu99;
-  else
-LangStd = LangStandard::lang_gnu17;
-#endif
-  break;
-case Language::ObjC:
-#if defined(CLANG_DEFAULT_STD_C)
-  LangStd = CLANG_DEFAULT_STD_C;
-#else
-  LangStd = LangStandard::lang_gnu11;
-#endif
-  break;
-case Language::CXX:
-case Language::ObjCXX:
-#if defined(CLANG_DEFAULT_STD_CXX)
-  LangStd = CLANG_DEFAULT_STD_CXX;
-#else
-  LangStd = LangStandard::lang_gnucxx14;
-#endif
-  break;
-case Language::RenderScript:
-  LangStd = LangStandard::lang_c99;
-  break;
-case Language::HIP:
-  LangStd = LangStandard::lang_hip;
-  break;
-}
-  }
-
-  const LangStandard &Std = LangStandard::getLangStandardForKind(LangStd);
-  Opts.LangStd = LangStd;
-  Opts.LineComment = Std.hasLineComments();
-  Opts.C99 = Std.isC99();
-  Opts.C11 = Std.isC11();
-  Opts.C17 = Std.isC17();
-  Opts.C2x = Std.isC2x();
-  Opts.CPlusPlus = Std.isCPlusPlus();
-  Opts.CPlusPlus11 = Std.isCPlusPlus11();
-  Opts.CPlusPlus14 = Std.isCPlusPlus14();
-  Opts.CPlusPlus17 = Std.isCPlusPlus17();
-  Opts.CPlusPlus20 = Std.isCPlusPlus20();
-  Opts.CPlusPlus2b = Std.isCPlusPlus2b();
-  Opts.GNUMode = Std.isGNUMode();
-  Opts.GNUCVersion = 0;
-  Opts.HexFloats = Std.hasHexFloats();
-  Opts.ImplicitInt = Std.hasImplicitInt();
-
-  // Set OpenCL Version.
-  Opts.OpenCL = Std.isOpenCL();
-  if (LangStd == LangStandard::lang_opencl10)
-Opts.OpenCLVersion = 100;
-  else if (LangStd == LangStandard::lang_opencl11)
-Opts.OpenCLVersion = 110;
-  else if (LangStd == LangStandard::lang_opencl12)
-Opts.OpenCLVersion = 120;
-  else if (LangStd == LangStandard::lang_opencl20)
-Opts.OpenCLVersion = 200;
-  else if (LangStd == LangStandard::lang_opencl30)
-Opts.OpenCLVersion = 300;
-  else if (LangStd == LangStandard::lang_openclcpp10)
-Opts.OpenCLCPlusPlusVersion = 100;
-  else if (LangStd == LangStandard::lang_openclcpp2021)
-Opts.OpenCLCPlusPlusVersion = 202100;
-
-  // OpenCL has some additional defaults.
-  if (Opts.OpenCL) {
-Opts.AltiVec = 0;
-Opts.ZVector = 0;
-Opts.setDefaultFPContractMode(LangOptions::FPM_On);
-Opts.OpenCLCPlusPlus = Opts.CPlusPlus;
-Opts.OpenCLPipes = Opts.getOpenCLCompatibleVersion() == 200;
-Opts.OpenCLGenericAddressSpace = Opts.getOpenCLCompatibleVersion() == 200;
-
-// Include default header file for OpenCL.
-if (Opts.IncludeDefaultHeader) {
-  if (Opts.DeclareOpenCLBuiltins) {
-// Only include base header file for builtin types and constants.
-Includes.push_back("opencl-c-base.h");
-  } else {
-