[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

Hi, could you have a look at it once more?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee created this revision.
bc-lee added reviewers: arphaman, MaskRay.
Herald added a project: All.
bc-lee requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch extends cc1as to export the build version load command with
LC_VERSION_MIN_MACOSX.
This is especially important for Mac Catalyst as Mac Catalyst uses
the MacOS's compiler rt built-ins.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121868

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -144,6 +144,9 @@
   /// otherwise.
   std::string TargetABI;
 
+  /// Darwin target variant triple, the variant of the deployment target
+  /// for which the code is being compiled.
+  llvm::Optional DarwinTargetVariantTriple;
   /// @}
 
 public:
@@ -209,6 +212,11 @@
 
   // Target Options
   Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
+  if (Arg *A = Args.getLastArg(options::OPT_darwin_target_variant_triple)) {
+llvm::Triple TVT(A->getValue());
+Opts.DarwinTargetVariantTriple = TVT;
+  }
+
   Opts.CPU = std::string(Args.getLastArgValue(OPT_target_cpu));
   Opts.Features = Args.getAllArgValues(OPT_target_feature);
 
@@ -407,6 +415,9 @@
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
   std::unique_ptr MOFI(
   TheTarget->createMCObjectFileInfo(Ctx, PIC));
+  if (Opts.DarwinTargetVariantTriple) {
+MOFI->setDarwinTargetVariantTriple(*Opts.DarwinTargetVariantTriple);
+  }
   Ctx.setObjectFileInfo(MOFI.get());
 
   if (Opts.SaveTemporaryLabels)
Index: clang/lib/Driver/ToolChains/Darwin.h
===
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -489,6 +489,12 @@
 : TargetVersion) < VersionTuple(V0, V1, V2);
   }
 
+  /// Returns the darwin target variant triple, the variant of the deployment
+  /// target for which the code is being compiled.
+  Optional getTargetVariantTriple() const override {
+return TargetVariantTriple;
+  }
+
 protected:
   /// Return true if c++17 aligned allocation/deallocation functions are not
   /// implemented in the c++ standard library of the deployment target we are
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7754,6 +7754,8 @@
 
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
+  const Optional TargetVariantTriple =
+  getToolChain().getTargetVariantTriple();
   const auto &D = getToolChain().getDriver();
 
   // Don't warn about "clang -w -c foo.s"
@@ -7771,6 +7773,10 @@
   // Add the "effective" target triple.
   CmdArgs.push_back("-triple");
   CmdArgs.push_back(Args.MakeArgString(TripleStr));
+  if (TargetVariantTriple) {
+CmdArgs.push_back("-darwin-target-variant-triple");
+CmdArgs.push_back(Args.MakeArgString(TargetVariantTriple->getTriple()));
+  }
 
   // Set the output mode, we currently only expect to be used as a real
   // assembler.
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -717,6 +717,10 @@
 return llvm::DenormalMode::getIEEE();
   }
 
+  virtual Optional getTargetVariantTriple() const {
+return llvm::Optional();
+  }
+
   // We want to expand the shortened versions of the triples passed in to
   // the values used for the bitcode libraries.
   static llvm::Triple getOpenMPTriple(StringRef TripleStr) {
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -4832,16 +4832,21 @@
   MarshallingInfoString>;
 def target_sdk_version_EQ : Joined<["-"], "target-sdk-version=">,
   HelpText<"The version of target SDK used for compilation">;
-def darwin_target_variant_triple : Separate<["-"], "darwin-target-variant-triple">,
-  HelpText<"Specify the darwin target variant triple">,
-  MarshallingInfoString>,
-  Normalizer<"normalizeTriple">;
 def darwin_target_variant_sdk_version_EQ : Joined<["-"],
   "darwin-target-variant-sdk-version=">,
   HelpText<"The version of darwin target variant SDK used for compilation">;
 
 } // let Flags = [CC1Option, CC1AsOption, NoDriverOption]
 
+let Flags = [CC1Option, CC1AsOption] in {
+
+def darwin_target_variant_triple : Separate<["-"], "darw

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 417595.
bc-lee added a comment.

Addressing review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/test/Misc/cc1as-darwin-target-variant-triple.s
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -144,6 +144,9 @@
   /// otherwise.
   std::string TargetABI;
 
+  /// Darwin target variant triple, the variant of the deployment target
+  /// for which the code is being compiled.
+  llvm::Optional DarwinTargetVariantTriple;
   /// @}
 
 public:
@@ -209,6 +212,9 @@
 
   // Target Options
   Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
+  if (Arg *A = Args.getLastArg(options::OPT_darwin_target_variant_triple))
+Opts.DarwinTargetVariantTriple = llvm::Triple(A->getValue());
+
   Opts.CPU = std::string(Args.getLastArgValue(OPT_target_cpu));
   Opts.Features = Args.getAllArgValues(OPT_target_feature);
 
@@ -407,6 +413,8 @@
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
   std::unique_ptr MOFI(
   TheTarget->createMCObjectFileInfo(Ctx, PIC));
+  if (Opts.DarwinTargetVariantTriple)
+MOFI->setDarwinTargetVariantTriple(*Opts.DarwinTargetVariantTriple);
   Ctx.setObjectFileInfo(MOFI.get());
 
   if (Opts.SaveTemporaryLabels)
Index: clang/test/Misc/cc1as-darwin-target-variant-triple.s
===
--- /dev/null
+++ clang/test/Misc/cc1as-darwin-target-variant-triple.s
@@ -0,0 +1,33 @@
+// Run cc1as using darwin-target-variant-triple
+// RUN: %clang -cc1as -triple x86_64-apple-macos10.9 -darwin-target-variant-triple x86_64-apple-ios13.1-macabi -filetype obj %s -o - \
+// RUN: | llvm-readobj --file-headers --macho-version-min - \
+// RUN: | FileCheck --check-prefix=CHECK %s
+
+// CHECK: File: 
+// CHECK: Format: Mach-O 64-bit x86-64
+// CHECK: Arch: x86_64
+// CHECK: AddressSize: 64bit
+// CHECK: MachHeader {
+// CHECK:   Magic: Magic64 (0xFEEDFACF)
+// CHECK:   CpuType: X86-64 (0x107)
+// CHECK:   CpuSubType: CPU_SUBTYPE_X86_64_ALL (0x3)
+// CHECK:   FileType: Relocatable (0x1)
+// CHECK:   NumOfLoadCommands: 3
+// CHECK:   SizeOfLoadCommands: 192
+// CHECK:   Flags [ (0x0)
+// CHECK:   ]
+// CHECK:   Reserved: 0x0
+// CHECK: }
+// CHECK: MinVersion {
+// CHECK:   Cmd: LC_VERSION_MIN_MACOSX
+// CHECK:   Size: 16
+// CHECK:   Version: 10.9
+// CHECK:   SDK: n/a
+// CHECK: }
+// CHECK: MinVersion {
+// CHECK:   Cmd: LC_BUILD_VERSION
+// CHECK:   Size: 24
+// CHECK:   Platform: macCatalyst
+// CHECK:   Version: 13.1
+// CHECK:   SDK: n/a
+// CHECK: }
Index: clang/lib/Driver/ToolChains/Darwin.h
===
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -489,6 +489,12 @@
 : TargetVersion) < VersionTuple(V0, V1, V2);
   }
 
+  /// Returns the darwin target variant triple, the variant of the deployment
+  /// target for which the code is being compiled.
+  Optional getTargetVariantTriple() const override {
+return TargetVariantTriple;
+  }
+
 protected:
   /// Return true if c++17 aligned allocation/deallocation functions are not
   /// implemented in the c++ standard library of the deployment target we are
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7754,6 +7754,8 @@
 
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
+  const Optional TargetVariantTriple =
+  getToolChain().getTargetVariantTriple();
   const auto &D = getToolChain().getDriver();
 
   // Don't warn about "clang -w -c foo.s"
@@ -7771,6 +7773,10 @@
   // Add the "effective" target triple.
   CmdArgs.push_back("-triple");
   CmdArgs.push_back(Args.MakeArgString(TripleStr));
+  if (TargetVariantTriple) {
+CmdArgs.push_back("-darwin-target-variant-triple");
+CmdArgs.push_back(Args.MakeArgString(TargetVariantTriple->getTriple()));
+  }
 
   // Set the output mode, we currently only expect to be used as a real
   // assembler.
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -717,6 +717,10 @@
 return llvm::DenormalMode::getIEEE();
   }
 
+  virtual Optional getTargetVariantTriple() const {
+return llvm::Optional();
+  }
+
   // We want to expand the 

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.h:495
+  Optional getTargetVariantTriple() const override {
+return TargetVariantTriple;
+  }

MaskRay wrote:
> llvm::None
Do you mean to modify the method in `clang/include/clang/Driver/ToolChain.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 423878.
bc-lee added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/test/Misc/cc1as-darwin-target-variant-triple.s
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -144,6 +144,9 @@
   /// otherwise.
   std::string TargetABI;
 
+  /// Darwin target variant triple, the variant of the deployment target
+  /// for which the code is being compiled.
+  llvm::Optional DarwinTargetVariantTriple;
   /// @}
 
 public:
@@ -209,6 +212,9 @@
 
   // Target Options
   Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
+  if (Arg *A = Args.getLastArg(options::OPT_darwin_target_variant_triple))
+Opts.DarwinTargetVariantTriple = llvm::Triple(A->getValue());
+
   Opts.CPU = std::string(Args.getLastArgValue(OPT_target_cpu));
   Opts.Features = Args.getAllArgValues(OPT_target_feature);
 
@@ -407,6 +413,8 @@
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
   std::unique_ptr MOFI(
   TheTarget->createMCObjectFileInfo(Ctx, PIC));
+  if (Opts.DarwinTargetVariantTriple)
+MOFI->setDarwinTargetVariantTriple(*Opts.DarwinTargetVariantTriple);
   Ctx.setObjectFileInfo(MOFI.get());
 
   if (Opts.SaveTemporaryLabels)
Index: clang/test/Misc/cc1as-darwin-target-variant-triple.s
===
--- /dev/null
+++ clang/test/Misc/cc1as-darwin-target-variant-triple.s
@@ -0,0 +1,33 @@
+// Run cc1as using darwin-target-variant-triple
+// RUN: %clang -cc1as -triple x86_64-apple-macos10.9 -darwin-target-variant-triple x86_64-apple-ios13.1-macabi -filetype obj %s -o - \
+// RUN: | llvm-readobj --file-headers --macho-version-min - \
+// RUN: | FileCheck --check-prefix=CHECK %s
+
+// CHECK: File: 
+// CHECK-NEXT: Format: Mach-O 64-bit x86-64
+// CHECK-NEXT: Arch: x86_64
+// CHECK-NEXT: AddressSize: 64bit
+// CHECK-NEXT: MachHeader {
+// CHECK-NEXT:   Magic: Magic64 (0xFEEDFACF)
+// CHECK-NEXT:   CpuType: X86-64 (0x107)
+// CHECK-NEXT:   CpuSubType: CPU_SUBTYPE_X86_64_ALL (0x3)
+// CHECK-NEXT:   FileType: Relocatable (0x1)
+// CHECK-NEXT:   NumOfLoadCommands: 3
+// CHECK-NEXT:   SizeOfLoadCommands: 192
+// CHECK-NEXT:   Flags [ (0x0)
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   Reserved: 0x0
+// CHECK-NEXT: }
+// CHECK-NEXT: MinVersion {
+// CHECK-NEXT:   Cmd: LC_VERSION_MIN_MACOSX
+// CHECK-NEXT:   Size: 16
+// CHECK-NEXT:   Version: 10.9
+// CHECK-NEXT:   SDK: n/a
+// CHECK-NEXT: }
+// CHECK-NEXT: MinVersion {
+// CHECK-NEXT:   Cmd: LC_BUILD_VERSION
+// CHECK-NEXT:   Size: 24
+// CHECK-NEXT:   Platform: macCatalyst
+// CHECK-NEXT:   Version: 13.1
+// CHECK-NEXT:   SDK: n/a
+// CHECK-NEXT: }
Index: clang/lib/Driver/ToolChains/Darwin.h
===
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -489,6 +489,12 @@
 : TargetVersion) < VersionTuple(V0, V1, V2);
   }
 
+  /// Returns the darwin target variant triple, the variant of the deployment
+  /// target for which the code is being compiled.
+  Optional getTargetVariantTriple() const override {
+return TargetVariantTriple;
+  }
+
 protected:
   /// Return true if c++17 aligned allocation/deallocation functions are not
   /// implemented in the c++ standard library of the deployment target we are
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7719,6 +7719,8 @@
 
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
+  const Optional TargetVariantTriple =
+  getToolChain().getTargetVariantTriple();
   const auto &D = getToolChain().getDriver();
 
   // Don't warn about "clang -w -c foo.s"
@@ -7736,6 +7738,10 @@
   // Add the "effective" target triple.
   CmdArgs.push_back("-triple");
   CmdArgs.push_back(Args.MakeArgString(TripleStr));
+  if (TargetVariantTriple) {
+CmdArgs.push_back("-darwin-target-variant-triple");
+CmdArgs.push_back(Args.MakeArgString(TargetVariantTriple->getTriple()));
+  }
 
   // Set the output mode, we currently only expect to be used as a real
   // assembler.
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -717,6 +717,10 @@
 return llvm::DenormalMode::getIE

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee marked 3 inline comments as done.
bc-lee added inline comments.



Comment at: clang/test/Misc/cc1as-darwin-target-variant-triple.s:2
+// Run cc1as using darwin-target-variant-triple
+// RUN: %clang -cc1as -triple x86_64-apple-macos10.9 
-darwin-target-variant-triple x86_64-apple-ios13.1-macabi -filetype obj %s -o - 
\
+// RUN: | llvm-readobj --file-headers --macho-version-min - \

MaskRay wrote:
> `-filetype obj` on the `%clang -cc1as` line seems weird.
Without this, I cannot create object file and validate it using `llvm-readobj`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

As you confirmed at https://crbug.com/1259122#c21, there is a problem with 
assembly files like `floatundidf.S` and this patch is intended to fix it. C/C++ 
files are fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 424209.
bc-lee added a comment.

Add `REQUIRES: x86-registered-target` on the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/test/Misc/cc1as-darwin-target-variant-triple.s
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -144,6 +144,9 @@
   /// otherwise.
   std::string TargetABI;
 
+  /// Darwin target variant triple, the variant of the deployment target
+  /// for which the code is being compiled.
+  llvm::Optional DarwinTargetVariantTriple;
   /// @}
 
 public:
@@ -209,6 +212,9 @@
 
   // Target Options
   Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
+  if (Arg *A = Args.getLastArg(options::OPT_darwin_target_variant_triple))
+Opts.DarwinTargetVariantTriple = llvm::Triple(A->getValue());
+
   Opts.CPU = std::string(Args.getLastArgValue(OPT_target_cpu));
   Opts.Features = Args.getAllArgValues(OPT_target_feature);
 
@@ -407,6 +413,8 @@
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
   std::unique_ptr MOFI(
   TheTarget->createMCObjectFileInfo(Ctx, PIC));
+  if (Opts.DarwinTargetVariantTriple)
+MOFI->setDarwinTargetVariantTriple(*Opts.DarwinTargetVariantTriple);
   Ctx.setObjectFileInfo(MOFI.get());
 
   if (Opts.SaveTemporaryLabels)
Index: clang/test/Misc/cc1as-darwin-target-variant-triple.s
===
--- /dev/null
+++ clang/test/Misc/cc1as-darwin-target-variant-triple.s
@@ -0,0 +1,34 @@
+// Run cc1as using darwin-target-variant-triple
+// REQUIRES: x86-registered-target
+// RUN: %clang -cc1as -triple x86_64-apple-macos10.9 -darwin-target-variant-triple x86_64-apple-ios13.1-macabi -filetype obj %s -o - \
+// RUN: | llvm-readobj --file-headers --macho-version-min - \
+// RUN: | FileCheck --check-prefix=CHECK %s
+
+// CHECK: File: 
+// CHECK-NEXT: Format: Mach-O 64-bit x86-64
+// CHECK-NEXT: Arch: x86_64
+// CHECK-NEXT: AddressSize: 64bit
+// CHECK-NEXT: MachHeader {
+// CHECK-NEXT:   Magic: Magic64 (0xFEEDFACF)
+// CHECK-NEXT:   CpuType: X86-64 (0x107)
+// CHECK-NEXT:   CpuSubType: CPU_SUBTYPE_X86_64_ALL (0x3)
+// CHECK-NEXT:   FileType: Relocatable (0x1)
+// CHECK-NEXT:   NumOfLoadCommands: 3
+// CHECK-NEXT:   SizeOfLoadCommands: 192
+// CHECK-NEXT:   Flags [ (0x0)
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   Reserved: 0x0
+// CHECK-NEXT: }
+// CHECK-NEXT: MinVersion {
+// CHECK-NEXT:   Cmd: LC_VERSION_MIN_MACOSX
+// CHECK-NEXT:   Size: 16
+// CHECK-NEXT:   Version: 10.9
+// CHECK-NEXT:   SDK: n/a
+// CHECK-NEXT: }
+// CHECK-NEXT: MinVersion {
+// CHECK-NEXT:   Cmd: LC_BUILD_VERSION
+// CHECK-NEXT:   Size: 24
+// CHECK-NEXT:   Platform: macCatalyst
+// CHECK-NEXT:   Version: 13.1
+// CHECK-NEXT:   SDK: n/a
+// CHECK-NEXT: }
Index: clang/lib/Driver/ToolChains/Darwin.h
===
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -489,6 +489,12 @@
 : TargetVersion) < VersionTuple(V0, V1, V2);
   }
 
+  /// Returns the darwin target variant triple, the variant of the deployment
+  /// target for which the code is being compiled.
+  Optional getTargetVariantTriple() const override {
+return TargetVariantTriple;
+  }
+
 protected:
   /// Return true if c++17 aligned allocation/deallocation functions are not
   /// implemented in the c++ standard library of the deployment target we are
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -7719,6 +7719,8 @@
 
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
+  const Optional TargetVariantTriple =
+  getToolChain().getTargetVariantTriple();
   const auto &D = getToolChain().getDriver();
 
   // Don't warn about "clang -w -c foo.s"
@@ -7736,6 +7738,10 @@
   // Add the "effective" target triple.
   CmdArgs.push_back("-triple");
   CmdArgs.push_back(Args.MakeArgString(TripleStr));
+  if (TargetVariantTriple) {
+CmdArgs.push_back("-darwin-target-variant-triple");
+CmdArgs.push_back(Args.MakeArgString(TargetVariantTriple->getTriple()));
+  }
 
   // Set the output mode, we currently only expect to be used as a real
   // assembler.
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolCha

[PATCH] D121868: [cc1as] Add support for emitting the build version load command for -darwin-target-variant

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added inline comments.



Comment at: clang/test/Misc/cc1as-darwin-target-variant-triple.s:2
+// Run cc1as using darwin-target-variant-triple
+// RUN: %clang -cc1as -triple x86_64-apple-macos10.9 
-darwin-target-variant-triple x86_64-apple-ios13.1-macabi -filetype obj %s -o - 
\
+// RUN: | llvm-readobj --file-headers --macho-version-min - \

thakis wrote:
> bc-lee wrote:
> > MaskRay wrote:
> > > `-filetype obj` on the `%clang -cc1as` line seems weird.
> > Without this, I cannot create object file and validate it using 
> > `llvm-readobj`.
> If you use `emit-obj` I think this test needs a `REQUIRES: 
> x86-registered-target`.
> 
> I'm not sure if piping binary data works on Windows – it might have stdout in 
> text mode, which will do some byte replacements.
A search for `-o -.*\n.*RUN.*llvm-readobj` in the LLVM codebase suggests that 
there are already several tests using that pattern. Does that really matter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121868

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

Some changes in this patch were applied as  https://reviews.llvm.org/D118759, 
so I'm re-uploading this patch to get rid of merge conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 405461.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sanitizer-ld.c
  compiler-rt/cmake/builtin-config-ix.cmake
  compiler-rt/cmake/config-ix.cmake

Index: compiler-rt/cmake/config-ix.cmake
===
--- compiler-rt/cmake/config-ix.cmake
+++ compiler-rt/cmake/config-ix.cmake
@@ -304,7 +304,7 @@
   if ("${platform}" STREQUAL "")
 message(FATAL_ERROR "platform cannot be empty")
   endif()
-  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?))$")
+  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?)|catalyst)$")
 set(is_valid TRUE)
   endif()
   set(${is_valid_out} ${is_valid} PARENT_SCOPE)
@@ -341,6 +341,7 @@
   include(CompilerRTDarwinUtils)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
+  find_darwin_sdk_dir(DARWIN_catalyst_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
   find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
@@ -366,6 +367,12 @@
 set(DARWIN_iossim_MIN_VER_FLAG -mios-simulator-version-min)
 set(DARWIN_iossim_SANITIZER_MIN_VER_FLAG
   ${DARWIN_iossim_MIN_VER_FLAG}=${DARWIN_ios_MIN_VER})
+list(APPEND DARWIN_EMBEDDED_PLATFORMS catalyst)
+set(DARWIN_catalyst_MIN_VER 13.0)
+set(DARWIN_catalyst_SANITIZER_MIN_VER_FLAG
+${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_catalyst_MIN_VER}
+-target
+${LIB_ARCH}-apple-ios${DARWIN_catalyst_MIN_VER}-macabi)
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
 list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
@@ -493,6 +500,10 @@
 endforeach()
   endif()
 
+  if("${platform}" STREQUAL "catalyst")
+set(DARWIN_${platform}_SKIP_CC_KEXT On)
+  endif()
+
   if(DARWIN_${platform}_SYSROOT)
 set(DARWIN_${platform}_CFLAGS
   ${DARWIN_COMMON_CFLAGS}
Index: compiler-rt/cmake/builtin-config-ix.cmake
===
--- compiler-rt/cmake/builtin-config-ix.cmake
+++ compiler-rt/cmake/builtin-config-ix.cmake
@@ -71,6 +71,7 @@
 if(APPLE)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
+  find_darwin_sdk_dir(DARWIN_catalyst_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
   find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
@@ -116,6 +117,13 @@
   ${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_ios_BUILTIN_MIN_VER})
 set(DARWIN_ios_BUILTIN_ALL_POSSIBLE_ARCHS ${ARM64} ${ARM32})
 set(DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64} arm64)
+list(APPEND DARWIN_EMBEDDED_PLATFORMS catalyst)
+set(DARWIN_catalyst_BUILTIN_MIN_VER 13.0)
+set(DARWIN_catalyst_BUILTIN_MIN_VER_FLAG
+${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_catalyst_BUILTIN_MIN_VER}
+-target
+${LIB_ARCH}-apple-ios${DARWIN_catalyst_BUILTIN_MIN_VER}-macabi)
+set(DARWIN_catalyst_BUILTIN_ALL_POSSIBLE_ARCHS ${X86_64} ${ARM64})
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
 list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
@@ -178,6 +186,10 @@
   endforeach()
 endif()
 
+if("${platform}" STREQUAL "catalyst")
+  set(DARWIN_${platform}_SKIP_CC_KEXT On)
+endif()
+
 if(DARWIN_${platform}_SYSROOT)
   darwin_test_archs(${platform}
 DARWIN_${platform}_BUILTIN_ARCHS
Index: clang/test/Driver/darwin-sanitizer-ld.c
===
--- clang/test/Driver/darwin-sanitizer-ld.c
+++ clang/test/Driver/darwin-sanitizer-ld.c
@@ -101,7 +101,7 @@
 // CHECK-ASAN-MACCATALYST: "{{.*}}ld{{(.exe)?}}"
 // CHECK-ASAN-MACCATALYST-NOT: "-lstdc++"
 // CHECK-ASAN-MACCATALYST-NOT: "-lc++"
-// CHECK-ASAN-MACCATALYST: libclang_rt.asan_osx_dynamic.dylib"
+// CHECK-ASAN-MACCATALYST: libclang_rt.asan_catalyst_dynamic.dylib"
 // CHECK-ASAN-MACCATALYST: "-rpath" "@executable_path"
 // CHECK-ASAN-MACCATALYST: "-rpath" "{{.*}}lib{{.*}}darwin"
 
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1219,7 +1219,7 @@
 return "osx";
   case DarwinPlatformKind::IPhoneOS:
 if (TargetEnvironment == MacCatalyst)
-  return "osx";
+  return "catalyst";
 return TargetEnvironment == NativeEnvironment || IgnoreSim ? "ios"
: "iossim";
   case DarwinPlatformKind::TvOS:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118875: [compiler-rt][builtins] build the macOS compiler-rt built-ins with Mac Catalyst support

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

Hi, I built and tested clang using this patch and 
https://reviews.llvm.org/D118862. It works great.
The current patch adds the -darwin-target-variant flag to all object files for 
MacOS targets in the compiler_rt's builtin library. However, users building 
compiler_rt may have compilers that don't support such flag, so it would be 
nice to have a CMake option to enable compiler flags like 
`COMPILER_RT_ENABLE_CATALYST`.


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

https://reviews.llvm.org/D118875

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee marked an inline comment as done.
bc-lee added a comment.

It's okay for some reviewers to make this change on my behalf. Thanks for 
reviewing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

Can someone commit my changes on behalf of it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee created this revision.
bc-lee added a reviewer: MyDeveloperDay.
bc-lee added projects: clang, clang-format.
Herald added a subscriber: cfe-commits.
bc-lee requested review of this revision.

Some Java style guides and IDEs group Java static imports after
 non-static imports. This patch allows clang-format to control
 the location of static imports.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87201

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/SortImportsTestJava.cpp

Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -250,6 +250,30 @@
  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
+
+  EXPECT_EQ("import com.test.b;\n"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n"
+"\n"
+"import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"
+ "import com.test.c;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
 "import org.a;\n"));
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -541,6 +541,8 @@
Style.IndentWrappedFunctionNames);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
+IO.mapOptional("JavaStaticImportAfterImport",
+   Style.JavaStaticImportAfterImport);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
 IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
@@ -2310,12 +2312,15 @@
 JavaImportGroups.push_back(
 findJavaImportGroup(Style, Imports[i].Identifier));
   }
+  bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport;
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
 // Negating IsStatic to push static imports above non-static imports.
-return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-   Imports[LHSI].Identifier) <
-   std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-   Imports[RHSI].Identifier);
+return std::make_tuple(!Imports[LHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[LHSI], Imports[LHSI].Identifier) <
+   std::make_tuple(!Imports[RHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[RHSI], Imports[RHSI].Identifier);
   });
 
   // Deduplicate imports.
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1671,6 +1671,10 @@
   bool JavaScriptWrapImports;
   // clang-format on
 
+  /// If true, clang-format will put Java Static imports after all non-static
+  /// imports.
+  bool JavaStaticImportAfterImport;
+
   /// If true, the empty line at the start of blocks is kept.
   /// \code
   ///true:  false:
@@ -2389,6 +2393,7 @@
IndentWidth == R.IndentWidth && Language == R.Language &&
IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
JavaImportGroups == R.JavaImportGroups &&
+   JavaStaticImportAfterImport == R.JavaStaticImportAfterImport &&
JavaScriptQuotes == R.JavaScriptQuotes &&
JavaScriptWrapImports == R.JavaScriptWrapImports &&
KeepEmptyLinesAtTheStartOfBlocks ==
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -2021,6 +2021,20 @@
  false:
  import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
 
+**JavaStaticImportAfterImport** (``bool``)
+ If true, clang-format will put Java static imports after all non-static imports.

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 290137.
bc-lee added a comment.

Add missing initializer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/SortImportsTestJava.cpp

Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -250,6 +250,30 @@
  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
+
+  EXPECT_EQ("import com.test.b;\n"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n"
+"\n"
+"import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"
+ "import com.test.c;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
 "import org.a;\n"));
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -543,6 +543,8 @@
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
+IO.mapOptional("JavaStaticImportAfterImport",
+   Style.JavaStaticImportAfterImport);
 IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
Style.KeepEmptyLinesAtTheStartOfBlocks);
 IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
@@ -899,6 +901,7 @@
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
+  LLVMStyle.JavaStaticImportAfterImport = false;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
@@ -2310,12 +2313,15 @@
 JavaImportGroups.push_back(
 findJavaImportGroup(Style, Imports[i].Identifier));
   }
+  bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport;
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
 // Negating IsStatic to push static imports above non-static imports.
-return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-   Imports[LHSI].Identifier) <
-   std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-   Imports[RHSI].Identifier);
+return std::make_tuple(!Imports[LHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[LHSI], Imports[LHSI].Identifier) <
+   std::make_tuple(!Imports[RHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[RHSI], Imports[RHSI].Identifier);
   });
 
   // Deduplicate imports.
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1671,6 +1671,10 @@
   bool JavaScriptWrapImports;
   // clang-format on
 
+  /// If true, clang-format will put Java Static imports after all non-static
+  /// imports.
+  bool JavaStaticImportAfterImport;
+
   /// If true, the empty line at the start of blocks is kept.
   /// \code
   ///true:  false:
@@ -2391,6 +2395,7 @@
JavaImportGroups == R.JavaImportGroups &&
JavaScriptQuotes == R.JavaScriptQuotes &&
JavaScriptWrapImports == R.JavaScriptWrapImports &&
+   JavaStaticImportAfterImport == R.JavaStaticImportAfterImport &&
KeepEmptyLinesAtTheStartOfBlocks ==
R.KeepEmptyLinesAtTheStartOfBlocks &&
MacroBlockBegin == R.MacroBlockBegin &&
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -2021,6 +2021,20 @@
  false:
  import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
 
+**JavaStaticImpor

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 290246.
bc-lee added a comment.

Modify the comment of Format.h to sync ClangFormatStyleOptions.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/SortImportsTestJava.cpp

Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -250,6 +250,30 @@
  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
+
+  EXPECT_EQ("import com.test.b;\n"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n"
+"\n"
+"import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"
+ "import com.test.c;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
 "import org.a;\n"));
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -544,6 +544,8 @@
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
+IO.mapOptional("JavaStaticImportAfterImport",
+   Style.JavaStaticImportAfterImport);
 IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
Style.KeepEmptyLinesAtTheStartOfBlocks);
 IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
@@ -901,6 +903,7 @@
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
+  LLVMStyle.JavaStaticImportAfterImport = false;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
@@ -2312,12 +2315,15 @@
 JavaImportGroups.push_back(
 findJavaImportGroup(Style, Imports[i].Identifier));
   }
+  bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport;
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
 // Negating IsStatic to push static imports above non-static imports.
-return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-   Imports[LHSI].Identifier) <
-   std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-   Imports[RHSI].Identifier);
+return std::make_tuple(!Imports[LHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[LHSI], Imports[LHSI].Identifier) <
+   std::make_tuple(!Imports[RHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[RHSI], Imports[RHSI].Identifier);
   });
 
   // Deduplicate imports.
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1689,6 +1689,21 @@
   bool JavaScriptWrapImports;
   // clang-format on
 
+  /// If true, clang-format will put Java static imports after all non-static
+  /// imports.
+  /// \code{.java}
+  ///   true:
+  ///   import static org.example.function1;
+  ///
+  ///   import org.example.ClassA;
+  ///
+  ///   false:
+  ///   import org.example.ClassA;
+  ///
+  ///   import static org.example.function1;
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+
   /// If true, the empty line at the start of blocks is kept.
   /// \code
   ///true:  false:
@@ -2410,6 +2425,7 @@
JavaImportGroups == R.JavaImportGroups &&
JavaScriptQuotes == R.JavaScriptQuotes &&
JavaScriptWrapImports == R.JavaScriptWrapImports &&
+   JavaStaticImportAfterImport == R.JavaStaticImportAfterImport &&
KeepEmptyLinesAtTheStartOfBlocks ==
R.KeepEmptyLinesAtTheStartOfBlocks &&
MacroBlockBegin == R.MacroBlockBegin &&
Index: clang/docs/ClangFormatStyleOptions.rst
=

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2027
+
+ .. code-block:: java
+ true:

MyDeveloperDay wrote:
> The ClangFormatStyleOptions.rst is generated using 
> doc/tools/dump_format_style.py which reads Format.h and generates this,
> 
> If this code block in not in the Format.h it will get removed the next time 
> the script is run, please don't change ClangFormatStyleOption.rst by hand use 
> the script, so add the code block to the Format.h file (see others options 
> for now to do this)
Done.



Comment at: clang/include/clang/Format/Format.h:1705
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+

JakeMerdichAMD wrote:
> 3 things here:
> 
> 1. Did you mix up the true and false cases?
> 2. (nit) You should also note that if this option is false, all static 
> imports are before all non-static imports (as opposed to mixed together 
> alphabetically). I was confused on first glance.
> 3. Please add this config option to FormatTests.cpp:ParsesConfigurationBools.
I understand. The description is somewhat misleading. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 290303.
bc-lee added a comment.

Some comments have been corrected and a unittest has been added in 
FormatTest.ParsesConfigurationBools


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp

Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -250,6 +250,30 @@
  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
+
+  EXPECT_EQ("import com.test.b;\n"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n"
+"\n"
+"import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"
+ "import com.test.c;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
 "import org.a;\n"));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13929,6 +13929,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(JavaStaticImportAfterImport);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -544,6 +544,8 @@
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
+IO.mapOptional("JavaStaticImportAfterImport",
+   Style.JavaStaticImportAfterImport);
 IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
Style.KeepEmptyLinesAtTheStartOfBlocks);
 IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
@@ -901,6 +903,7 @@
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
+  LLVMStyle.JavaStaticImportAfterImport = false;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
@@ -2312,12 +2315,15 @@
 JavaImportGroups.push_back(
 findJavaImportGroup(Style, Imports[i].Identifier));
   }
+  bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport;
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
 // Negating IsStatic to push static imports above non-static imports.
-return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-   Imports[LHSI].Identifier) <
-   std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-   Imports[RHSI].Identifier);
+return std::make_tuple(!Imports[LHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[LHSI], Imports[LHSI].Identifier) <
+   std::make_tuple(!Imports[RHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[RHSI], Imports[RHSI].Identifier);
   });
 
   // Deduplicate imports.
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1618,10 +1618,12 @@
 
   /// A vector of prefixes ordered by the desired groups for Java imports.
   ///
-  /// Each group is separated by a newline. Static imports will also follow the
-  /// same grouping convention above all non-static imports. One group's prefix
-  /// can be a subset of another - the longest prefix is always matched. Within
-  /// a group, the imports are ordered lexicographically.
+  /// One group's prefix can be a subset of another - the longest 

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 290381.
bc-lee added a comment.

Fix the example to match the description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp

Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -250,6 +250,30 @@
  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
+
+  EXPECT_EQ("import com.test.b;\n"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n"
+"\n"
+"import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"
+ "import com.test.c;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
 "import org.a;\n"));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13929,6 +13929,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(JavaStaticImportAfterImport);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -544,6 +544,8 @@
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
 IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
+IO.mapOptional("JavaStaticImportAfterImport",
+   Style.JavaStaticImportAfterImport);
 IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
Style.KeepEmptyLinesAtTheStartOfBlocks);
 IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
@@ -901,6 +903,7 @@
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
+  LLVMStyle.JavaStaticImportAfterImport = false;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
@@ -2312,12 +2315,15 @@
 JavaImportGroups.push_back(
 findJavaImportGroup(Style, Imports[i].Identifier));
   }
+  bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport;
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
 // Negating IsStatic to push static imports above non-static imports.
-return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-   Imports[LHSI].Identifier) <
-   std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-   Imports[RHSI].Identifier);
+return std::make_tuple(!Imports[LHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[LHSI], Imports[LHSI].Identifier) <
+   std::make_tuple(!Imports[RHSI].IsStatic ^
+   StaticImportAfterNormalImport,
+   JavaImportGroups[RHSI], Imports[RHSI].Identifier);
   });
 
   // Deduplicate imports.
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1618,10 +1618,12 @@
 
   /// A vector of prefixes ordered by the desired groups for Java imports.
   ///
-  /// Each group is separated by a newline. Static imports will also follow the
-  /// same grouping convention above all non-static imports. One group's prefix
-  /// can be a subset of another - the longest prefix is always matched. Within
-  /// a group, the imports are ordered lexicographically.
+  /// One group's prefix can be a subset of another - the longest prefix is
+  /// always matched. Within a group, the imports a

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 290619.
bc-lee added a comment.

Add more tests and rename options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/SortImportsTestJava.cpp

Index: clang/unittests/Format/SortImportsTestJava.cpp
===
--- clang/unittests/Format/SortImportsTestJava.cpp
+++ clang/unittests/Format/SortImportsTestJava.cpp
@@ -250,6 +250,62 @@
  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, SortJavaStaticImport) {
+  FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before;
+  EXPECT_EQ("import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n"
+"\n"
+"import com.test.b;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"));
+
+  FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After;
+  EXPECT_EQ("import com.test.b;\n"
+"import com.test.c;\n"
+"\n"
+"import org.b;\n"
+"\n"
+"import com.b;\n"
+"\n"
+"import static com.test.a;\n"
+"\n"
+"import static org.a;\n"
+"\n"
+"import static com.a;\n",
+sort("import static com.test.a;\n"
+ "import static org.a;\n"
+ "import static com.a;\n"
+ "import com.test.b;\n"
+ "import org.b;\n"
+ "import com.b;\n"
+ "import com.test.c;\n"));
+}
+
+TEST_F(SortImportsTestJava, SortJavaStaticImportAsGroup) {
+  FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After;
+
+  EXPECT_EQ("import com.test.a;\n"
+"import com.test.b;\n"
+"\n"
+"import static org.a;\n"
+"import static org.b;\n",
+sort("import com.test.a;\n"
+ "import static org.a;\n"
+ "import com.test.b;\n"
+ "import static org.b;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
 "import org.a;\n"));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14276,6 +14276,12 @@
   CHECK_PARSE("BitFieldColonSpacing: After", BitFieldColonSpacing,
   FormatStyle::BFCS_After);
 
+  Style.SortJavaStaticImport = FormatStyle::SJSIO_Before;
+  CHECK_PARSE("SortJavaStaticImport: After", SortJavaStaticImport,
+  FormatStyle::SJSIO_After);
+  CHECK_PARSE("SortJavaStaticImport: Before", SortJavaStaticImport,
+  FormatStyle::SJSIO_Before);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -377,6 +377,15 @@
   }
 };
 
+template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO &IO,
+  FormatStyle::SortJavaStaticImportOptions &Value) {
+IO.enumCase(Value, "Before", FormatStyle::SJSIO_Before);
+IO.enumCase(Value, "After", FormatStyle::SJSIO_After);
+  }
+};
+
 template <> struct MappingTraits {
   static void mapping(IO &IO, FormatStyle &Style) {
 // When reading, read the language first, we need it for getPredefinedStyle.
@@ -574,6 +583,7 @@
 IO.mapOptional("RawStringFormats", Style.RawStringFormats);
 IO.mapOptional("ReflowComments", Style.ReflowComments);
 IO.mapOptional("SortIncludes", Style.SortIncludes);
+IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
 IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
 IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
@@ -947,6 +957,7 @@
 
   LLVMStyle.DisableFormat = false;
   LLVMStyle.SortIncludes = true;
+  LLVMStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before;
   LLVMStyle.SortUsingDeclarations = true;
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:1970
+  but this behavior is changed by another option,
+  ``JavaStaticImportAfterImport``.
 

MyDeveloperDay wrote:
> Can you add a test that shows if the sorting is still in the groups, i.e. I 
> can't tell if
> 
> ```
> import com.test.a;
> import static org.a;
> 
> import com.test.b;
> import static org.b;
> ```
> 
> becomes
> 
> ```
> import static org.a;
> import static org.b;
> import com.test.a;
> 
> import com.test.b;
> ```
> 
> or
> 
> ```
> import static org.a;
> import static org.b;
> 
> import com.test.a;
> 
> import com.test.b;
> ```
> 
> or
> 
> ```
> import static org.a;
> import com.test.a;
> 
> import static org.b;
> import com.test.b;
> 
> ```
> 
> 
> 
> 
> 
Done.



Comment at: clang/include/clang/Format/Format.h:1708
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+

JakeMerdichAMD wrote:
> MyDeveloperDay wrote:
> > Can we consider changing the name or the option to make it clearer what its 
> > for?
> > 
> > `SortJavaStaticImport`
> > 
> > and make it an enumeration rather than true/false
> > 
> > `Before,After,Never`
> > 
> > There need to be a "don't touch it option (.e.g. Never)" that does what it 
> > does today (and that should be the default, otherwise we end up causing 
> > clang-format to change ALL java code" which could be 100's of millions of 
> > lines+
> > 
> > 
> I'm confused, there is not currently a never option... Right now the behavior 
> is always 'before', there is no 'after' or 'never'.
> 
> Making it an enum would probably be more ergonomic though, even there is no 
> never option.
If `SortIncludes` is `true`, it doesn't make sense to not touch Java static 
include.
Making it an enum is also good for me.




Comment at: clang/unittests/Format/SortImportsTestJava.cpp:253
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;

JakeMerdichAMD wrote:
> MyDeveloperDay wrote:
> > please test all options
> > 
> > You are also missing a test for checking the PARSING of the options
> Parsing test was already noted and done (unless this changes to an enum of 
> course). But testing the 'false' case here makes sense.
Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee created this revision.
Herald added a subscriber: mgorny.
bc-lee requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

Mac Catalyst has different platform fields in build_version_command,
so it's natural to create a separate libclang_rt.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117924

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  compiler-rt/cmake/builtin-config-ix.cmake
  compiler-rt/cmake/config-ix.cmake


Index: compiler-rt/cmake/config-ix.cmake
===
--- compiler-rt/cmake/config-ix.cmake
+++ compiler-rt/cmake/config-ix.cmake
@@ -310,7 +310,7 @@
   if ("${platform}" STREQUAL "")
 message(FATAL_ERROR "platform cannot be empty")
   endif()
-  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?))$")
+  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?)|catalyst)$")
 set(is_valid TRUE)
   endif()
   set(${is_valid_out} ${is_valid} PARENT_SCOPE)
@@ -347,6 +347,7 @@
   include(CompilerRTDarwinUtils)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
+  find_darwin_sdk_dir(DARWIN_catalyst_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
   find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
@@ -372,6 +373,12 @@
 set(DARWIN_iossim_MIN_VER_FLAG -mios-simulator-version-min)
 set(DARWIN_iossim_SANITIZER_MIN_VER_FLAG
   ${DARWIN_iossim_MIN_VER_FLAG}=${DARWIN_ios_MIN_VER})
+list(APPEND DARWIN_EMBEDDED_PLATFORMS catalyst)
+set(DARWIN_catalyst_MIN_VER 13.0)
+set(DARWIN_catalyst_SANITIZER_MIN_VER_FLAG
+${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_catalyst_MIN_VER}
+-target
+${LIB_ARCH}-apple-ios${DARWIN_catalyst_MIN_VER}-macabi)
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
 list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
@@ -499,6 +506,10 @@
 endforeach()
   endif()
 
+  if("${platform}" STREQUAL "catalyst")
+set(DARWIN_${platform}_SKIP_CC_KEXT On)
+  endif()
+
   if(DARWIN_${platform}_SYSROOT)
 set(DARWIN_${platform}_CFLAGS
   ${DARWIN_COMMON_CFLAGS}
Index: compiler-rt/cmake/builtin-config-ix.cmake
===
--- compiler-rt/cmake/builtin-config-ix.cmake
+++ compiler-rt/cmake/builtin-config-ix.cmake
@@ -71,6 +71,7 @@
 if(APPLE)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
+  find_darwin_sdk_dir(DARWIN_catalyst_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
   find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
@@ -115,7 +116,14 @@
 set(DARWIN_ios_BUILTIN_MIN_VER_FLAG
   ${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_ios_BUILTIN_MIN_VER})
 set(DARWIN_ios_BUILTIN_ALL_POSSIBLE_ARCHS ${ARM64} ${ARM32})
-set(DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64})
+set(DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64} ${ARM64})
+list(APPEND DARWIN_EMBEDDED_PLATFORMS catalyst)
+set(DARWIN_catalyst_BUILTIN_MIN_VER 13.0)
+set(DARWIN_catalyst_BUILTIN_MIN_VER_FLAG
+${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_catalyst_BUILTIN_MIN_VER}
+-target
+${LIB_ARCH}-apple-ios${DARWIN_catalyst_BUILTIN_MIN_VER}-macabi)
+set(DARWIN_catalyst_BUILTIN_ALL_POSSIBLE_ARCHS ${X86_64} ${ARM64})
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
 list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
@@ -178,6 +186,10 @@
   endforeach()
 endif()
 
+if("${platform}" STREQUAL "catalyst")
+  set(DARWIN_${platform}_SKIP_CC_KEXT On)
+endif()
+
 if(DARWIN_${platform}_SYSROOT)
   darwin_test_archs(${platform}
 DARWIN_${platform}_BUILTIN_ARCHS
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1219,7 +1219,7 @@
 return "osx";
   case DarwinPlatformKind::IPhoneOS:
 if (TargetEnvironment == MacCatalyst)
-  return "osx";
+  return "catalyst";
 return TargetEnvironment == NativeEnvironment || IgnoreSim ? "ios"
: "iossim";
   case DarwinPlatformKind::TvOS:


Index: compiler-rt/cmake/config-ix.cmake
===
--- compiler-rt/cmake/config-ix.cmake
+++ compiler-rt/cmake/config-ix.cmake
@@ -310,7 +310,7 @@
   if ("${platform}" STREQUAL "")
 message(FATAL_ERROR "platform cannot be empty")
   endif()
-  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?))$")
+  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?)|catalyst)$")
 set(is_valid TRUE)
   endif()
   set(${is_valid_out} ${is_valid} PARENT_SCOPE)
@@ -347,6 +347,7 @@
   include(CompilerRTDarwinUti

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee updated this revision to Diff 402201.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sanitizer-ld.c
  compiler-rt/cmake/builtin-config-ix.cmake
  compiler-rt/cmake/config-ix.cmake

Index: compiler-rt/cmake/config-ix.cmake
===
--- compiler-rt/cmake/config-ix.cmake
+++ compiler-rt/cmake/config-ix.cmake
@@ -310,7 +310,7 @@
   if ("${platform}" STREQUAL "")
 message(FATAL_ERROR "platform cannot be empty")
   endif()
-  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?))$")
+  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?)|catalyst)$")
 set(is_valid TRUE)
   endif()
   set(${is_valid_out} ${is_valid} PARENT_SCOPE)
@@ -347,6 +347,7 @@
   include(CompilerRTDarwinUtils)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
+  find_darwin_sdk_dir(DARWIN_catalyst_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
   find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
@@ -372,6 +373,12 @@
 set(DARWIN_iossim_MIN_VER_FLAG -mios-simulator-version-min)
 set(DARWIN_iossim_SANITIZER_MIN_VER_FLAG
   ${DARWIN_iossim_MIN_VER_FLAG}=${DARWIN_ios_MIN_VER})
+list(APPEND DARWIN_EMBEDDED_PLATFORMS catalyst)
+set(DARWIN_catalyst_MIN_VER 13.0)
+set(DARWIN_catalyst_SANITIZER_MIN_VER_FLAG
+${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_catalyst_MIN_VER}
+-target
+${LIB_ARCH}-apple-ios${DARWIN_catalyst_MIN_VER}-macabi)
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
 list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
@@ -499,6 +506,10 @@
 endforeach()
   endif()
 
+  if("${platform}" STREQUAL "catalyst")
+set(DARWIN_${platform}_SKIP_CC_KEXT On)
+  endif()
+
   if(DARWIN_${platform}_SYSROOT)
 set(DARWIN_${platform}_CFLAGS
   ${DARWIN_COMMON_CFLAGS}
Index: compiler-rt/cmake/builtin-config-ix.cmake
===
--- compiler-rt/cmake/builtin-config-ix.cmake
+++ compiler-rt/cmake/builtin-config-ix.cmake
@@ -71,6 +71,7 @@
 if(APPLE)
 
   find_darwin_sdk_dir(DARWIN_osx_SYSROOT macosx)
+  find_darwin_sdk_dir(DARWIN_catalyst_SYSROOT macosx)
   find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator)
   find_darwin_sdk_dir(DARWIN_ios_SYSROOT iphoneos)
   find_darwin_sdk_dir(DARWIN_watchossim_SYSROOT watchsimulator)
@@ -115,7 +116,14 @@
 set(DARWIN_ios_BUILTIN_MIN_VER_FLAG
   ${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_ios_BUILTIN_MIN_VER})
 set(DARWIN_ios_BUILTIN_ALL_POSSIBLE_ARCHS ${ARM64} ${ARM32})
-set(DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64})
+set(DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64} ${ARM64})
+list(APPEND DARWIN_EMBEDDED_PLATFORMS catalyst)
+set(DARWIN_catalyst_BUILTIN_MIN_VER 13.0)
+set(DARWIN_catalyst_BUILTIN_MIN_VER_FLAG
+${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_catalyst_BUILTIN_MIN_VER}
+-target
+${LIB_ARCH}-apple-ios${DARWIN_catalyst_BUILTIN_MIN_VER}-macabi)
+set(DARWIN_catalyst_BUILTIN_ALL_POSSIBLE_ARCHS ${X86_64} ${ARM64})
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
 list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
@@ -178,6 +186,10 @@
   endforeach()
 endif()
 
+if("${platform}" STREQUAL "catalyst")
+  set(DARWIN_${platform}_SKIP_CC_KEXT On)
+endif()
+
 if(DARWIN_${platform}_SYSROOT)
   darwin_test_archs(${platform}
 DARWIN_${platform}_BUILTIN_ARCHS
Index: clang/test/Driver/darwin-sanitizer-ld.c
===
--- clang/test/Driver/darwin-sanitizer-ld.c
+++ clang/test/Driver/darwin-sanitizer-ld.c
@@ -101,7 +101,7 @@
 // CHECK-ASAN-MACCATALYST: "{{.*}}ld{{(.exe)?}}"
 // CHECK-ASAN-MACCATALYST-NOT: "-lstdc++"
 // CHECK-ASAN-MACCATALYST-NOT: "-lc++"
-// CHECK-ASAN-MACCATALYST: libclang_rt.asan_osx_dynamic.dylib"
+// CHECK-ASAN-MACCATALYST: libclang_rt.asan_catalyst_dynamic.dylib"
 // CHECK-ASAN-MACCATALYST: "-rpath" "@executable_path"
 // CHECK-ASAN-MACCATALYST: "-rpath" "{{.*}}lib{{.*}}darwin"
 
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1219,7 +1219,7 @@
 return "osx";
   case DarwinPlatformKind::IPhoneOS:
 if (TargetEnvironment == MacCatalyst)
-  return "osx";
+  return "catalyst";
 return TargetEnvironment == NativeEnvironment || IgnoreSim ? "ios"
: "iossim";
   case DarwinPlatformKind::TvOS:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/lis

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

It may not be appropriate to add other runtime libraries specifically for Mac 
Catalyst. However, currently `lld` does not allow linking with dynamic 
libraries with different types of build_version_command, whereas `ld64` does 
allow for Mac Catalyst. Considering compatibility with lld, Mac Catalyst’s 
runtime libraries need to be added also.

See my other patch https://reviews.llvm.org/D117925 for more details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

For the Chromium side issue is https://crbug.com/1259122. Also I opened LLVM 
issue https://github.com/llvm/llvm-project/issues/53477.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117924: [compiler_rt] Add a seperate runtime for Mac Catalyst

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee abandoned this revision.
bc-lee added a comment.

Closed in favor of https://reviews.llvm.org/D118862 and 
https://reviews.llvm.org/D118875


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117924

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

[Byoungchan Lee via Phabricator via cfe-commits]

bc-lee added a comment.

Thanks for the review. Since I don't have commit access, so I want someone else 
to apply this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87201

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits