Consistently get extensions from FileTypes.h #18203

jrose-apple · 2018-07-25T00:20:02Z

...instead of sometimes hardcoding them and sometimes using Strings.h. ~~The exceptions are the libraries that sit below Frontend; these can continue using strings.~~

There's one semantic change here, which is that LLVM IR files mistakenly had the extension "ir" instead of "ll" before. Nothing was actually depending on this until now, though.

jrose-apple · 2018-07-25T00:20:13Z

@swift-ci Please smoke test

jrose-apple · 2018-07-25T01:06:50Z

@swift-ci Please smoke test

gottesmm · 2018-07-25T02:59:13Z

Seems reasonable to me. I haven't looked through all of the details though.

davidungar

This is great stuff! Of all my comments, it's the ones about symmetry that strike me hardest. If it has to be this way, how about a comment somewhere explaining why all the type extensions can't be in one place?

davidungar · 2018-07-25T04:08:16Z

include/swift/Frontend/Types.def

@@ -46,13 +46,15 @@ TYPE("object",              Object,                    "o",               "")
 TYPE("dSYM",                dSYM,                      "dSYM",            "")
 TYPE("dependencies",        Dependencies,              "d",               "")
 TYPE("autolink",            AutolinkFile,              "autolink",        "")
-TYPE("swiftmodule",         SwiftModuleFile,           "swiftmodule",     "")
-TYPE("swiftdoc",            SwiftModuleDocFile,        "swiftdoc",        "")
+TYPE("swiftmodule",         SwiftModuleFile,


As I read this, I'm wondering why some of the extension strings are here but others are macros. Why the asymmetry?

davidungar · 2018-07-25T04:09:10Z

include/swift/Frontend/Types.def

 TYPE("swiftinterface",      SwiftModuleInterfaceFile,  "swiftinterface",  "")
 TYPE("assembly",            Assembly,                  "s",               "")
 TYPE("raw-sil",             RawSIL,                    "sil",             "")
 TYPE("raw-sib",             RawSIB,                    "sib",             "")
-TYPE("llvm-ir",             LLVM_IR,                   "ir",              "")
+TYPE("llvm-ir",             LLVM_IR,                   "ll",              "")


Did you mean to change "ir" to "ll"?

Yep, as commented above "ir" was a mistake but we weren't relying on it for anything until now.

Sorry-missed that comment.

davidungar · 2018-07-25T04:11:25Z

include/swift/Strings.h

-constexpr static const char SIB_EXTENSION[] = "sib";
-/// The extension for LLVM IR files.
-constexpr static const char LLVM_BC_EXTENSION[] = "bc";
-constexpr static const char LLVM_IR_EXTENSION[] = "ll";


W.r.t. the asymmetry comment above, another option would be to keep these here and use all of these consts in the TYPE definitions. Why one vs the other? (I'll bet you have a good reason.)

I have an okay reason. :-/ The remaining constants here are ones that are used in libraries below Frontend (in layering terms). I'd prefer not to put them all here because I think the file_types abstractions are better, but it does make an ugly asymmetry here.

Another option would be to go a step further and sink the information in file_types down to Basic, which means they could be included from anywhere.

I like that option: sinking the info down to Basic! It's a nice unification which I suspect will pay off in the future!

davidungar · 2018-07-25T04:13:35Z

lib/Driver/Driver.cpp

@@ -718,7 +718,7 @@ createStatsReporter(const llvm::opt::InputArgList *ArgList,
  if (Inputs.size() == 1) {
    InputName = Inputs[0].second->getSpelling();
  }
-  StringRef OutputType = file_types::getTypeTempSuffix(OI.CompilerOutputType);
+  StringRef OutputType = file_types::getExtension(OI.CompilerOutputType);


FWIW, @graydon has reminded me that it can be better to separate renaming changes from other changes, for instance in separate commits. I like what you're doing here--just passing this on.

Fair enough.

davidungar · 2018-07-25T04:14:01Z

lib/Driver/Driver.cpp

@@ -2718,7 +2718,8 @@ void Driver::chooseRemappingOutputPath(Compilation &C,
  } else {
    llvm::SmallString<128> Path(Output->getPrimaryOutputFilenames()[0]);
    bool isTempFile = C.isTemporaryFile(Path);
-    llvm::sys::path::replace_extension(Path, "remap");
+    llvm::sys::path::replace_extension(Path,
+        file_types::getExtension(file_types::ID::TY_Remapping));


davidungar · 2018-07-25T04:14:34Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

@@ -117,11 +117,14 @@ OutputFilesComputer::create(const llvm::opt::ArgList &args,
    return None;
  }

+  file_types::ID outputType =


Can this be const

davidungar · 2018-07-25T04:18:38Z

lib/Frontend/FrontendInputsAndOutputs.cpp

  }
  // If we have one primary input and it's a filename with extension "sil",
  // treat the input as SIL.
-  unsigned silPrimaryCount = numberOfPrimaryInputsEndingWith(SIL_EXTENSION);
+  unsigned silPrimaryCount = numberOfPrimaryInputsEndingWith(


I should have made this a const if/when I wrote it.

Let's be honest: this isn't just for temporary files. Also, it's not really a "suffix" if it doesn't include the leading dot. No functionality change.

...instead of sometimes hardcoding them and sometimes using Strings.h. The exceptions are the libraries that sit below Frontend; these can continue using strings.

jrose-apple · 2018-07-26T04:50:22Z

@swift-ci Please test

swift-ci · 2018-07-26T04:50:50Z

Build failed
Swift Test OS X Platform
Git Sha - 6631399969aa38e22146f5b082039dcec4e8b5b0

swift-ci · 2018-07-26T04:53:32Z

Build failed
Swift Test Linux Platform
Git Sha - 6631399969aa38e22146f5b082039dcec4e8b5b0

The next commit will take advantage of this, but this is just a mechanical change.

Remove the last few literal extension strings from Strings.h in favor of the file_types APIs, and use those APIs in a few more places.

No intended functionality change.

jrose-apple · 2018-07-26T05:23:40Z

@swift-ci Please test

swift-ci · 2018-07-26T05:25:35Z

Build failed
Swift Test OS X Platform
Git Sha - 457130fdf7441ed77b8585d8fea625853d264bd1

swift-ci · 2018-07-26T05:25:57Z

Build failed
Swift Test Linux Platform
Git Sha - 457130fdf7441ed77b8585d8fea625853d264bd1

davidungar

Very nice! Love the symmetry. Would still want the consts, but it's not a big deal.

jrose-apple requested review from davidungar and gottesmm July 25, 2018 00:20

jrose-apple force-pushed the strung-out branch from 6631399 to 205bbaf Compare July 25, 2018 01:06

davidungar reviewed Jul 25, 2018

View reviewed changes

jrose-apple added 2 commits July 25, 2018 21:49

Rename file_types::getTypeTempSuffix to getExtension

6e5aefc

Let's be honest: this isn't just for temporary files. Also, it's not really a "suffix" if it doesn't include the leading dot. No functionality change.

Consistently get extensions from swift/Frontend/FileTypes.h

798496c

...instead of sometimes hardcoding them and sometimes using Strings.h. The exceptions are the libraries that sit below Frontend; these can continue using strings.

jrose-apple force-pushed the strung-out branch from 205bbaf to 457130f Compare July 26, 2018 04:49

jrose-apple changed the title ~~Consistently get extensions from swift/Frontend/FileTypes.h~~ Consistently get extensions from FileTypes.h Jul 26, 2018

jrose-apple added 3 commits July 25, 2018 22:23

Move swift/Frontend/FileTypes.h down to Basic

67a6a4f

The next commit will take advantage of this, but this is just a mechanical change.

Consistently get extensions from swift/Basic/FileTypes.h (part 2)

97b152d

Remove the last few literal extension strings from Strings.h in favor of the file_types APIs, and use those APIs in a few more places.

Minor improvements to file_types

a5899ee

No intended functionality change.

jrose-apple force-pushed the strung-out branch from 457130f to a5899ee Compare July 26, 2018 05:23

davidungar approved these changes Jul 26, 2018

View reviewed changes

jrose-apple merged commit ca5eacf into swiftlang:master Jul 26, 2018

jrose-apple deleted the strung-out branch July 26, 2018 21:30

Consistently get extensions from FileTypes.h #18203

Consistently get extensions from FileTypes.h #18203

Uh oh!

Conversation

jrose-apple commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Jul 25, 2018

Uh oh!

jrose-apple commented Jul 25, 2018

Uh oh!

gottesmm commented Jul 25, 2018

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 26, 2018

Uh oh!

swift-ci commented Jul 26, 2018

Uh oh!

swift-ci commented Jul 26, 2018

Uh oh!

jrose-apple commented Jul 26, 2018

Uh oh!

swift-ci commented Jul 26, 2018

Uh oh!

swift-ci commented Jul 26, 2018

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrose-apple commented Jul 25, 2018 •

edited

Loading