NFC: Some small refactoring in `buildCompilation` #16762

davidungar · 2018-05-22T01:46:17Z

Minor refactoring of buildCompilation to try to get the function to be fewer lines, and also moving definitions closer to uses.

davidungar · 2018-05-22T01:46:26Z

@swift-ci please smoke test

davidungar · 2018-05-23T19:13:37Z

@swift-ci please smoke test

davidungar · 2018-05-23T21:57:16Z

@jrose-apple This a minor refactoring of buildCompilation. How does it strike you?

graydon

Looks good to me!

graydon · 2018-05-30T04:07:44Z

lib/Driver/Driver.cpp

@@ -857,7 +880,7 @@ Driver::buildCompilation(const ToolChain &TC,
  }

  // For getting bulk fixits, or for when users explicitly request to continue
-  // building despite errors.
+  // building despite errors.)


Stray parenthesis?

Thanks! Fixed.

davidungar · 2018-05-30T23:54:04Z

@swift-ci please smoke test

davidungar · 2018-05-30T23:55:16Z

@jrose-apple I've rebased to the latest. Looking forward to your thoughts.

davidungar · 2018-05-30T23:58:51Z

@CodaFi Don't know if you want to review this or not, but cc'ing you since you are into this code nowadays. I started refactoring this function a week ago because I felt we might be elaborating and extending it, and in lieu of a major restructure, thought I would do a few small things to help its malleability. If you do want to look at this, we can better align our efforts.

CodaFi · 2018-05-31T18:24:58Z

lib/Driver/Driver.cpp

+    SmallString<128> workingDirectory;
+    workingDirectory = A->getValue();
+    llvm::sys::fs::make_absolute(workingDirectory);
+    return workingDirectory.str();


I'm generally not a fan of escaping SmallString buffers like this. The lifetime of the buffer being in the scope of buildCompilation clarified that it should survive through its uses - here we're relying on the StringRef of its contents being copied by the conversion to std::string.

Thank you for this. Yes. In this case, the copying overhead is not significant, we get to shrink down a super-long function, and encapsulate the intension-shift.

I think Robert was saying that the implicit conversion here from StringRef to std::string was too subtle for him. Is that correct?

I'm not sure what "implicit" means here because I had thought that the .str() part as well as the return type made the conversion explicit. I would like to encapsulate this computation in a function in order to capture the intension-shift (aka abstract-level change), from the caller to this callee. Is there a way to do it that you would prefer? How else would one write a function that returns a string?

I'll let Robert decide what he'd prefer—I have no further comments—but str() produces a StringRef, not a std::string. The maximally explicit thing would be *sigh* workingDirectory.str().str().

@jrose-apple Thanks! I didn't realize that--not familiar enough with SmallString and str didn't convey that to me. I have changed that to make it clearer.
@CodaFi How do you feel about how it reads now?

CodaFi · 2018-05-31T18:28:09Z

lib/Driver/Driver.cpp

+        computeStatsReporter(ArgList.get(), Inputs, OI, DefaultTargetTriple);
+
+    C =
+    llvm::make_unique<Compilation>(


Nit: clang-format wants this to be

C = llvm::make_unique<Compilation>( Diags, TC, OI, Level, std::move(ArgList), std::move(TranslatedArgList), std::move(Inputs), buildRecordPath, outputBuildRecordForModuleOnlyBuild, ArgsHash, StartTime, LastBuildTime, DriverFilelistThreshold, Incremental, BatchMode, DriverBatchSeed, DriverForceOneBatchRepartition, SaveTemps, ShowDriverTimeCompilation, std::move(StatsReporter));

but I like the stacked argument list here, so I guess an outdent of the argument list would split the difference.

C = llvm::make_unique<Compilation>( Diags, TC, OI, Level, std::move(ArgList), std::move(TranslatedArgList), std::move(Inputs), buildRecordPath, outputBuildRecordForModuleOnlyBuild, ArgsHash, StartTime, LastBuildTime, DriverFilelistThreshold, Incremental, BatchMode, DriverBatchSeed, DriverForceOneBatchRepartition, SaveTemps, ShowDriverTimeCompilation, std::move(StatsReporter));

Yes, I'll outdent it. This is a case where git-clang-format does the "wrong" thing and makes the deltas harder to read.

CodaFi · 2018-05-31T18:30:00Z

Only little things I noticed on my pass through this.

LGTM

1. Compute things closer to use, and use const, and 2. Factor out some computations into subroutines in order to shorten buildCompilation.

davidungar · 2018-06-15T17:52:53Z

@swift-ci please smoke test

davidungar · 2018-06-15T17:53:24Z

@graydon I have redone this to include your recent SIGINT changes.

davidungar · 2018-06-15T17:54:22Z

@jrose-apple Now that WWDC is past, it would be great if you could give this PR a review. You always find good things to fix.

jrose-apple

Sure, works for me.

jrose-apple · 2018-06-15T17:54:54Z

lib/Driver/Driver.cpp

+                               const bool ShowIncrementalBuildDecisions) {
+  {
+    const bool WasIncrementalRequested =
+    ArgList->hasArg(options::OPT_incremental);


Indentation got messed up here, but also it seems like this doesn't need its own variable at all.

I tried it without the variable and you are right; it's better that way.

jrose-apple · 2018-06-15T17:56:07Z

lib/Driver/Driver.cpp

+}
+
+static std::unique_ptr<UnifiedStatsReporter>
+computeStatsReporter(const llvm::opt::InputArgList *ArgList,


createStatsReporter, maybe? The reporter isn't being computed.

Fixed, thanks.

jrose-apple · 2018-06-15T17:58:28Z

lib/Driver/Driver.cpp

+      ArgList->hasArg(options::OPT_whole_module_optimization)
+          ? "is not compatible with whole module optimization."
+          : ArgList->hasArg(options::OPT_embed_bitcode)
+                ? "is not currently compatible with embedding LLVM IR bitcode"


Tiny nitpick: you're being inconsistent about periods here. (The old code used them in both messages.)

Nice catch!

davidungar · 2018-06-15T18:10:28Z

@swift-ci please smoke test

davidungar · 2018-06-22T17:23:29Z

@jrose-apple Review ping (tx!)

davidungar · 2018-06-22T20:42:32Z

@swift-ci please smoke test

davidungar · 2018-06-22T20:42:44Z

@swift-ci please smoke test

jrose-apple · 2018-06-22T20:45:01Z

lib/Driver/Driver.cpp

@@ -700,7 +700,9 @@ computeWorkingDirectory(const llvm::opt::InputArgList *ArgList) {
    SmallString<128> workingDirectory;
    workingDirectory = A->getValue();
    llvm::sys::fs::make_absolute(workingDirectory);
-    return workingDirectory.str();
+    //  Create a new string that can outlive ArgList.


It's not about outliving ArgList; it's outliving the stack-based workingDirectory.

Sigh.. yes! Fixed!!. Thanks.

jrose-apple · 2018-06-22T21:22:26Z

lib/Driver/Driver.cpp

@@ -700,7 +700,7 @@ computeWorkingDirectory(const llvm::opt::InputArgList *ArgList) {
    SmallString<128> workingDirectory;
    workingDirectory = A->getValue();
    llvm::sys::fs::make_absolute(workingDirectory);
-    //  Create a new string that can outlive ArgList.
+    //  Create a new string that can outlive workingDirectory.


…

That doesn't add anything. It doesn't say why workingDirectory is worth outliving.

(I wouldn't have added a comment at all, but if you're going to add one, please make it as high-level / semantic as possible.)

davidungar · 2018-06-27T20:06:06Z

@swift-ci please smoke test

davidungar · 2018-06-27T20:06:41Z

@jrose-apple I have removed the comment you objected to. Is there anything else? Thanks.

jrose-apple

Go for it!

davidungar · 2018-06-27T20:57:52Z

@jrose-apple Thanks!

davidungar · 2018-06-28T04:58:11Z

@CodaFi thanks!

davidungar force-pushed the buildCompilation-refactor-1 branch from 6f10073 to 328b3b1 Compare May 23, 2018 19:12

davidungar changed the title ~~NFC: Some small refactoring in buildCompilation~~ NFC, WIP: Some small refactoring in buildCompilation May 23, 2018

davidungar requested a review from jrose-apple May 23, 2018 21:56

davidungar changed the title ~~NFC, WIP: Some small refactoring in buildCompilation~~ NFC: Some small refactoring in buildCompilation May 23, 2018

graydon approved these changes May 30, 2018

View reviewed changes

davidungar force-pushed the buildCompilation-refactor-1 branch from de3dd3f to 732f41b Compare May 30, 2018 23:53

CodaFi reviewed May 31, 2018

View reviewed changes

davidungar mentioned this pull request Jun 14, 2018

Rdar 40526328 emit sigint for cancelled batch constituents #17167

Merged

Minor refactor of buildCompilation

9fd23bb

1. Compute things closer to use, and use const, and 2. Factor out some computations into subroutines in order to shorten buildCompilation.

davidungar force-pushed the buildCompilation-refactor-1 branch from 12b3812 to 9fd23bb Compare June 15, 2018 17:52

jrose-apple reviewed Jun 15, 2018

View reviewed changes

Small improvements from Jordan’s review.

272c5d8

Clarify lifetime of result of computeWorkingDirectory.

77ec1e1

jrose-apple reviewed Jun 22, 2018

View reviewed changes

Fix wrong comment.

3045a25

jrose-apple reviewed Jun 22, 2018

View reviewed changes

Remove unneeded comment.

62d8501

jrose-apple approved these changes Jun 27, 2018

View reviewed changes

CodaFi approved these changes Jun 27, 2018

View reviewed changes

davidungar merged commit c1c9cb8 into swiftlang:master Jun 27, 2018

davidungar deleted the buildCompilation-refactor-1 branch June 14, 2019 18:03

NFC: Some small refactoring in buildCompilation #16762

NFC: Some small refactoring in buildCompilation #16762

Uh oh!

Conversation

davidungar commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented May 22, 2018

Uh oh!

davidungar commented May 23, 2018

Uh oh!

davidungar commented May 23, 2018

Uh oh!

graydon 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!

davidungar commented May 30, 2018

Uh oh!

davidungar commented May 30, 2018

Uh oh!

davidungar commented May 30, 2018

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!

CodaFi May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented May 31, 2018

Uh oh!

davidungar commented Jun 15, 2018

Uh oh!

davidungar commented Jun 15, 2018

Uh oh!

davidungar commented Jun 15, 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!

davidungar commented Jun 15, 2018

Uh oh!

davidungar commented Jun 22, 2018

Uh oh!

davidungar commented Jun 22, 2018

Uh oh!

davidungar commented Jun 22, 2018

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

NFC: Some small refactoring in `buildCompilation` #16762

NFC: Some small refactoring in `buildCompilation` #16762

davidungar commented May 22, 2018 •

edited

Loading

CodaFi May 31, 2018 •

edited

Loading