[Batch Mode] Move supplementary output paths into a dedicated structure in FrontendInputsAndOutputs (3) #14402

davidungar · 2018-02-04T02:13:20Z

In preparation for many sets of supplementary outputs, reify the set, and move it into FrontendInputsAndOutputs. Also move the computation of those.

davidungar · 2018-02-04T02:14:27Z

@swift-ci please smoke test

davidungar · 2018-02-06T03:37:33Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-06T04:06:15Z

@swift-ci please smoke test

davidungar · 2018-02-06T08:49:15Z

There’s a new diagnostic in this PR. I’ll try to write a test for it tomorrow.

jrose-apple

The next step is to sink this into the individual inputs, then?

jrose-apple · 2018-02-06T16:47:34Z

include/swift/Frontend/ArgsToFrontendOutputsConverter.h

@@ -41,7 +42,8 @@ class ArgsToFrontendOutputsConverter {
      : Args(args), ModuleName(moduleName), InputsAndOutputs(inputsAndOutputs),
        Diags(diags) {}

-  Optional<std::vector<std::string>> convert();
+  Optional<std::pair<std::vector<std::string>, SupplementaryOutputPaths>>


Opinion: This is too complicated now, better to use out parameters.

I love the functional style, but since this will get even more complicated when a vector of SupplementaryPaths is returned, I'm changing it as suggested.

I guess another alternative would be to use a one-off struct.

Yes, that would work, too. I just went with out-parameters.

jrose-apple · 2018-02-06T16:48:51Z

include/swift/Frontend/ArgsToFrontendOutputsConverter.h

+
+  /// \return None if error
+  /// Otherwise return a vector with one entry per file producing supplementary
+  /// outputs.


This gets collected into a single section in Doxygen, so you'll want a period after the word "error". But I also don't understand what this function does from its name or its comment.

Expanded the comment and tweaked the name. If it is still unclear, would you like to suggest something?

jrose-apple · 2018-02-06T16:49:16Z

include/swift/Frontend/ArgsToFrontendOutputsConverter.h

+  /// Otherwise return a vector with one entry per file producing supplementary
+  /// outputs.
+  Optional<std::vector<std::string>>
+  readSupplementaryOutputArguments(swift::options::ID path_id) const;


Nitpicks: No need for swift::, pathID instead of path_id.

Fixed, thanks.

jrose-apple · 2018-02-06T16:50:41Z

include/swift/Frontend/ArgsToFrontendOutputsConverter.h

+      const SupplementaryOutputPaths &pathsFromFilelists,
+      const InputFile &) const;
+
+  StringRef deriveImplicitBasis(StringRef outputFilename,


Nitpick: I don't think "basis" is a standard term. I happen to know what you mean (directory + basename but not extension), but if you want to call it this please leave a doc comment.

Renamed function to: deriveDefaultSupplementaryOutputPathExcludingExtension and renamed implicitBasis to defaultSupplementaryOutputPathExcludingExtension. May be long, but at least it's clear.

jrose-apple · 2018-02-06T16:53:25Z

include/swift/Frontend/SupplementaryOutputPaths.h

+                           std::string referenceDependenciesFilePath,
+                           std::string serializedDiagnosticsPath,
+                           std::string loadedModuleTracePath,
+                           std::string tbdPath)


I think it'd be more declarative to set these individually rather than as a long list of undifferentiated arguments.

If only C++ had C's named member initializer syntax. (It does in C++17.)

Yes, a problematic tradeoff. I've made the change.

jrose-apple · 2018-02-06T17:04:29Z

include/swift/AST/DiagnosticsFrontend.def

@@ -190,6 +190,9 @@ ERROR(error_index_failed_status_check,none,
 ERROR(error_index_inputs_more_than_outputs,none,
  "index output filenames do not match input source files", ())

+ERROR(error_too_many_supplementary_outputs,none,
+"too many supplementary outputs '%0'", (StringRef))


This should probably just be too many %0 arguments or perhaps too many %0 arguments (expected %1, got %2).

Very nice! Fixed.

jrose-apple · 2018-02-06T17:07:16Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+  const unsigned N =
+      InputsAndOutputs.countOfFilesProducingSupplementaryOutput();
+  if (paths.size() > N) {
+    std::unique_ptr<llvm::opt::OptTable> Table = createSwiftOptTable();


You shouldn't have to greate a new OptTable for this; each Arg already has a reference to the full Option info in the existing table.

Thanks. Figured out a way to do it without generating a new table.

jrose-apple · 2018-02-06T17:09:45Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+    return None;
+  }
+  while (paths.size() < N)
+    paths.push_back(std::string());


This doesn't make sense; like with -o, there should always be exactly the right number of paths, or 0 paths.

(Honestly it is useful to be able to override previous options by having the last argument win, but that's still not what's going on here.)

Do you mean that the code should emit a diagnostic and bail out if there are paths, but not the right number?

I think so, yes.

OK, done, I think.

jrose-apple · 2018-02-06T17:12:38Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+  if (!serializedDiagnosticsPath)
+    return None;
+
+  auto objCHeaderOutputPath = determineSupplementaryOutputFilename(


Super nitpick: "ObjC" is one word when abbreviated as such, so its lowercase form is "objc".

jrose-apple · 2018-02-06T17:13:50Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+    return outputFilename;
+
+  // If we have a primary input, so use that as the basis for the name of the
+  // serialized diagnostics file, otherwise fall back on the


Comment needs updating.

Good catch, thanks! Much of the comments in that function were redundant, so I streamlined them.

davidungar · 2018-02-06T17:26:24Z

Thanks for getting on this so quickly! You are keeping me energized. I have a midday commitment, but otherwise will be working on these comments to get you a revision ASAP, including a test for the new diagnostic.

The next step will be the introduction of the PrimarySpecificPaths structure, which can be passed around in the compiler and holds both main and supp outputs. Then, putting that in the inputs.

davidungar · 2018-02-07T00:30:50Z

@swift-ci please smoke test

davidungar · 2018-02-07T00:34:16Z

At this point, I've taken a whack at addressing all of the suggestions. Feel free to let me know what ought to be done next. I'll also be writing a test for the new diagnostic.

davidungar · 2018-02-07T01:23:53Z

@swift-ci please smoke test

davidungar · 2018-02-07T02:44:53Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T03:15:18Z

@swift-ci please smoke test os x platform

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

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T04:49:21Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T05:15:45Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T05:45:22Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T06:31:23Z

@swift-ci please test

swift-ci · 2018-02-07T06:31:50Z

Build failed
Swift Test OS X Platform
Git Sha - e4c59343b54f86bb9cf089e6839be8820135bccd

swift-ci · 2018-02-07T06:32:52Z

Build failed
Swift Test Linux Platform
Git Sha - e4c59343b54f86bb9cf089e6839be8820135bccd

jrose-apple · 2018-02-07T17:08:45Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+    return None;
+  }
+  while (paths.size() < N)
+    paths.push_back(std::string());


…which means this can become, unconditionally, paths.rezise(N).

Thanks for reviewing so quickly! I went down a rathole last night and had to back up.
Maybe have backed up too far. I have rewritten this routine and directly constructs a vector with N blanks if needed.

jrose-apple · 2018-02-07T17:11:35Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+StringRef SupplementaryOutputPathsComputer::
+    deriveDefaultSupplementaryOutputPathExcludingExtension(
+        StringRef outputFilename, const InputFile &input) const {
+  // Put the serialized diagnostics file next to the output file if possible.


Another specific-rather-than-generic comment.

Thank you for the catch! Fixed.

jrose-apple · 2018-02-07T17:11:57Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+    return pathFromArguments;
+
+  if (!Args.hasArg(emitOpt))
+    return std::string();


Shouldn't this be None?

std::string() is correct, but the code was unclear because it was using None for error. However,
there is no error condition in this version. So, I modified this function and its caller to just return a string, instead of an optional. And, I added a Doxygen comment. (The optional may come back in a future PR, but I'll comment it if so.) Thanks for catching this.

jrose-apple · 2018-02-07T17:21:30Z

lib/Frontend/FrontendInputsAndOutputs.cpp

+    ArrayRef<std::string> outputFiles,
+    SupplementaryOutputPaths supplementaryOutputs) {
+  if (outputFiles.empty())
+    ;


Does this need to be handled explicitly at all? When is the assertion from before no longer valid?

I just pushed a new commit that cleans up these corner cases. Check it out!

jrose-apple · 2018-02-07T17:22:05Z

lib/FrontendTool/FrontendTool.cpp

-static bool serializeSIB(FrontendOptions &opts, SILModule *SM,
-                         ASTContext &Context, ModuleOrSourceFile MSF) {
-  const std::string &moduleOutputPath = opts.ModuleOutputPath;
+static bool serializeSIBIfNeeded(FrontendOptions &opts, SILModule *SM,


Looks like the wrong name snuck back into this version.

Thanks. Fixed.

davidungar · 2018-02-07T18:23:39Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T18:36:10Z

@swift-ci please smoke test os x platform

davidungar · 2018-02-07T21:50:52Z

@swift-ci please smoke test

…ndOutputs.

davidungar · 2018-02-07T22:00:23Z

@swift-ci please smoke test

jrose-apple

Last few comments, but no need for another round of review after that!

jrose-apple · 2018-02-08T19:53:29Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+  Diags.diagnose(SourceLoc(), diag::error_wrong_number_of_arguments,
+                 Args.getLastArg(pathID)->getOption().getName(), N,
+                 paths.size());
+  return None;


Nice, I like the way this came out!

jrose-apple · 2018-02-08T19:54:11Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+    options::ID emitOpt, std::string pathFromArguments, StringRef extension,
+    StringRef mainOutputIfUsable,
+    StringRef defaultSupplementaryOutputPathExcludingExtension) const {
+  using namespace options;


Nitpick: this is no longer necessary.

Thanks. Removed.

jrose-apple · 2018-02-08T19:56:45Z

lib/Frontend/ArgsToFrontendOutputsConverter.cpp

+    return std::vector<std::string>(N, std::string());
+
+  Diags.diagnose(SourceLoc(), diag::error_wrong_number_of_arguments,
+                 Args.getLastArg(pathID)->getOption().getName(), N,


getPrefixedName() produces a slightly prettier diagnostic (it says "-emit-module-path" instead of "emit-module-path").

Great! Done.

davidungar · 2018-02-08T20:31:32Z

Good catches! I'll make the fixes, retest, and merge. Thank you.

davidungar · 2018-02-08T20:36:19Z

@swift-ci please smoke test

…ere are no inputs.

davidungar · 2018-02-08T21:23:25Z

@swift-ci please smoke test

Tweaks to shorten performCompile* a bit more.

7271f68

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from e4c5934 to c145b55 Compare February 6, 2018 03:37

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from c145b55 to b4852f9 Compare February 6, 2018 04:05

davidungar requested a review from jrose-apple February 6, 2018 08:48

jrose-apple reviewed Feb 6, 2018

View reviewed changes

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from b4852f9 to 1714740 Compare February 7, 2018 00:30

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 1714740 to c98189b Compare February 7, 2018 02:44

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from c98189b to 6672d5a Compare February 7, 2018 03:15

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 6672d5a to 987cc1f Compare February 7, 2018 04:08

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 987cc1f to a5fdb7f Compare February 7, 2018 04:49

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from a5fdb7f to 0d290a3 Compare February 7, 2018 05:14

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 0d290a3 to b2b052e Compare February 7, 2018 05:44

jrose-apple reviewed Feb 7, 2018

View reviewed changes

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from b2b052e to 526441c Compare February 7, 2018 18:23

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 526441c to 1096388 Compare February 7, 2018 18:28

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 1096388 to e32afe8 Compare February 7, 2018 18:44

Move supplementary outputs to SupplementaryOutputs in FrontendInputsA…

ba4f2a8

…ndOutputs.

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 521f102 to 93daabb Compare February 7, 2018 21:59

jrose-apple approved these changes Feb 8, 2018

View reviewed changes

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from 93daabb to fda0bf3 Compare February 8, 2018 20:35

Cleaner handling of cases where action produces no output or where th…

ef02d0b

…ere are no inputs.

davidungar force-pushed the PR-18-5a-SupplementaryOutputs branch from fda0bf3 to ef02d0b Compare February 8, 2018 21:23

davidungar merged commit 95f70f7 into swiftlang:master Feb 8, 2018

davidungar deleted the PR-18-5a-SupplementaryOutputs branch May 16, 2018 16:59

[Batch Mode] Move supplementary output paths into a dedicated structure in FrontendInputsAndOutputs (3) #14402

[Batch Mode] Move supplementary output paths into a dedicated structure in FrontendInputsAndOutputs (3) #14402

Uh oh!

Conversation

davidungar commented Feb 4, 2018

Uh oh!

davidungar commented Feb 4, 2018

Uh oh!

davidungar commented Feb 6, 2018

Uh oh!

davidungar commented Feb 6, 2018

Uh oh!

davidungar commented Feb 6, 2018

Uh oh!

jrose-apple 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!

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!

Choose a reason for hiding this comment

Uh oh!

davidungar commented Feb 6, 2018

Uh oh!

davidungar commented Feb 7, 2018

Uh oh!

davidungar commented Feb 7, 2018

Uh oh!

davidungar commented Feb 7, 2018

Uh oh!

davidungar commented Feb 7, 2018

Uh oh!

davidungar commented Feb 7, 2018

davidungar Feb 7, 2018 •

edited

Loading