SR-7165. Excise <iostream> from reflection #27081

ismetanin · 2019-09-08T08:57:40Z

Replace iostream with raw_ostream to follow LLVM Coding Standards.

compnerd · 2019-09-08T16:05:46Z

This is reasonable. Removing the header usage isn't a bad idea. It would be nice if the two changes were squashed into a single change.

compnerd · 2019-09-08T16:05:55Z

@swift-ci please test

swift-ci · 2019-09-08T16:44:54Z

Build failed
Swift Test Linux Platform
Git Sha - 683c7564e43e6a22fae845a68ceb93587a683dc7

swift-ci · 2019-09-08T17:09:31Z

Build failed
Swift Test OS X Platform
Git Sha - 683c7564e43e6a22fae845a68ceb93587a683dc7

gottesmm · 2019-09-08T17:37:51Z

I haven't read this PR (don't treat this as a review), but I want to give a big plus one to the effort. I wanted to look into this some time ago but had other pressing things to look at.

gottesmm

Looks overall pretty reasonable. Two things:

I did a git grep with your branch and found there are still iostream includes. Can you remove any that are unnecessary as a result of this patch? Seems like some are in the locations you have been touching.
Is there a reason you changed that CMakeLists.txt line I pointed out? Can you remove?

gottesmm · 2019-09-08T17:40:07Z

stdlib/public/SwiftRemoteMirror/CMakeLists.txt

@@ -1,3 +1,5 @@
+append("-undefined dynamic_lookup" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)


Whats with this part of the change here?

Ugh, somehow I missed this part of the change. This seems wrong; I think that it is trying to work around the missing LLVMSupport linkage. Thank you @gottesmm for catching this oversight.

gottesmm · 2019-09-08T17:43:46Z

Specifically:

include/swift/Reflection/TypeRefBuilder.h:#include <iostream>
tools/swift-demangle-yamldump/swift-demangle-yamldump.cpp:#include <iostream>
tools/swift-demangle/swift-demangle.cpp:#include <iostream>
tools/swift-reflection-dump/swift-reflection-dump.cpp:#include <iostream>
unittests/Parse/SyntaxParsingCacheTests.cpp:#include <iostream>

ismetanin · 2019-09-08T18:09:21Z

@compnerd

This is reasonable. Removing the header usage isn't a bad idea. It would be nice if the two changes were squashed into a single change.

Okay, I'll squash my changes but at first, can someone help me resolve linking error on Linux platform? I just thought that -undefined dynamic_lookup is OSX-specific and Linux shared libraries behave as if -undefined dynamic_lookup was enabled all the time

ismetanin · 2019-09-08T18:13:13Z

Looks overall pretty reasonable. Two things:

I did a git grep with your branch and found there are still iostream includes. Can you remove any that are unnecessary as a result of this patch? Seems like some are in the locations you have been touching.

Is there a reason you changed that CMakeLists.txt line I pointed out? Can you remove?

@gottesmm

I'll definitely look there and will try to remove, thanks
It resolves Undefined symbols for architecture x86_64: error, you can find more details in that commit from the previous PR of @nafu

compnerd · 2019-09-08T18:13:21Z

@ismetanin, I think that we are missing a link against LLVMSupport. Letting this be undefined is a bad idea, we don't want to end up in the case where the symbol is unresolved at load time.

ismetanin · 2019-09-08T18:16:04Z

@compnerd
So how we should fix missing LLVMSupport linkage in the correct way?

jrose-apple · 2019-09-09T16:32:45Z

Sorry, the intent of SR-7165 is to replace iostreams with printf or similar, not raw_ostream. Reflection libraries should not link in LLVMSupport because the client may have their own copy of LLVM. See the discussion in the previous #15677.

ismetanin · 2019-09-09T19:15:54Z

Sorry, the intent of SR-7165 is to replace iostreams with printf or similar, not raw_ostream. Reflection libraries should not link in LLVMSupport because the client may have their own copy of LLVM. See the discussion in the previous #15677.

@jrose-apple oh, okay, I’ll try to fix it

_{Sent with GitHawk}

ismetanin · 2019-09-19T06:25:00Z

@jrose-apple Hey, I'm here again and I have a question.
Should I replace iostreams in this way?

Instead llvm::errs() << indent() << "## New NodeFactory\n";
use fprintf(stderr, "%s%s", indent(), "## New NodeFactory\n");

Instead std::cout << "<null type info>\n";
use printf("<null type info>\n")

And should I replace the use of std::ostream? If yes, with what?

jrose-apple · 2019-09-19T15:23:03Z

That looks reasonable to me. I think the use of plain std::ostream is fine (via the <ostream> header rather than <iostream>); it is the global constructors of std::cin, std::cout, and std::cerr that we are trying to avoid.

ismetanin · 2019-10-01T14:19:34Z

@jrose-apple I reasearced a bit and found that in some places we use std::ostream as an input parameter in this way: dump(std::cerr, 0); but std::cerr declared in <iostream> and I don't know what to use instead std::cerr. For example, we can change the parameter to C-style FILE* and do something like dump(stderr, 0) but I'm not sure that it is ok

jrose-apple · 2019-10-01T16:56:13Z

Hm. @jckarter, what do you think? (Too bad C++ doesn't have a built-in std::ostream subclass that takes a FILE * or a file descriptor.)

jckarter · 2019-10-01T19:54:15Z

I would personally just use FILE* instead of trying to use ostream.

ismetanin · 2019-10-14T14:22:59Z

@jrose-apple @jckarter I've just replaced iostreams with fprintf and now PR is ready to review 👾

jckarter · 2019-10-14T15:56:43Z

include/swift/Demangling/Demangler.h

@@ -24,7 +24,7 @@
 //#define NODE_FACTORY_DEBUGGING

 #ifdef NODE_FACTORY_DEBUGGING
-#include <iostream>
+#include <ostream>


Can <ostream> be removed as well?

jckarter · 2019-10-14T17:42:53Z

@swift-ci Please test

swift-ci · 2019-10-14T17:44:53Z

Build failed
Swift Test OS X Platform
Git Sha - 683c7564e43e6a22fae845a68ceb93587a683dc7

swift-ci · 2019-10-14T17:45:53Z

Build failed
Swift Test Linux Platform
Git Sha - 683c7564e43e6a22fae845a68ceb93587a683dc7

jrose-apple

Thanks for sticking with this! A few comments.

jrose-apple · 2019-10-14T17:40:39Z

include/swift/Demangling/Demangler.h

@@ -141,8 +133,7 @@ class NodeFactory {
    size_t ObjectSize = NumObjects * sizeof(T);
    CurPtr = align(CurPtr, alignof(T));
 #ifdef NODE_FACTORY_DEBUGGING
-    std::cerr << indent() << "alloc " << ObjectSize << ", CurPtr = "
-              << (void *)CurPtr << "\n";
+    fprintf(stderr, "%salloc%zu, CurPtr = %p\n", indent().c_str(), ObjectSize, (void *)CurPtr)


Nitpick: space after "alloc" got lost here.

jrose-apple · 2019-10-14T17:42:01Z

stdlib/public/Reflection/MetadataSource.cpp

  unsigned Indent;

-  std::ostream &indent(unsigned Amount) {
+  FILE * &indent(unsigned Amount) {


These should all drop the &. (They probably shouldn't return anything at this point since they're not going to be chained.)

jrose-apple · 2019-10-14T17:43:09Z

stdlib/public/Reflection/TypeLowering.cpp

+    printField("alignment", std::to_string(TI.getAlignment()));
+    printField("stride", std::to_string(TI.getStride()));
+    printField("num_extra_inhabitants", std::to_string(TI.getNumExtraInhabitants()));
+    printField("bitwise_takable", TI.isBitwiseTakable() ? "1" : "0");


Might as well go with "true" and "false" if you're changing it anyway.

jrose-apple · 2019-10-14T17:44:41Z

stdlib/public/Reflection/TypeRefBuilder.cpp

      for (auto &fieldRef : *descriptor.getLocalBuffer()) {
        auto field = descriptor.getField(fieldRef);
        auto fieldName = getTypeRefString(readTypeRef(field, field->FieldName));
-        OS << std::string(fieldName.begin(), fieldName.end());
+        fprintf(file, "%s", std::string(fieldName.begin(), fieldName.end()).c_str());


@jckarter Is this necessary or is the field name going to be null-terminated?

It would be better to use fprintf(file, "%*s", (int)fieldName.size(), fieldName.data()).

jrose-apple · 2019-10-14T17:45:18Z

stdlib/public/Reflection/TypeRefBuilder.cpp

+      fprintf(file, "Size: %s\n", std::to_string(descriptor->Size).c_str());
+      fprintf(file, "Alignment: %s:\n", std::to_string(descriptor->getAlignment()).c_str());
+      fprintf(file, "Stride: %s:\n", std::to_string(descriptor->Stride).c_str());
+      fprintf(file, "NumExtraInhabitants: %s:\n", std::to_string(descriptor->NumExtraInhabitants).c_str());


These are all numeric values, why go through %s?

Oh, my bad 🧐
I'll fix it

ismetanin · 2019-10-14T19:23:43Z

Can someone help me with fails on @swift-ci? I'm confused with error 20:44:51 ERROR: Couldn't find any revision to build. Verify the repository and branch configuration for this job. (the same on Linux and macOS)
Probably it's because I force pushed to the repo (Now I'm just pushing new commits thought)

theblixguy · 2019-10-14T19:40:54Z

You can ignore it - the job gets restarted if you look at the status checks. I don't know why it fails initially most of the time; might have something to do with force pushes/rebase or maybe something's wrong with the bot.

jrose-apple · 2019-10-15T16:42:46Z

@swift-ci Please test

swift-ci · 2019-10-15T16:44:59Z

Build failed
Swift Test OS X Platform
Git Sha - dd7d173

swift-ci · 2019-10-15T16:45:14Z

Build failed
Swift Test Linux Platform
Git Sha - dd7d173

jckarter · 2019-10-15T16:57:30Z

@swift-ci Please test

jrose-apple · 2019-10-15T17:53:25Z

10:44:46 
/home/buildnode/jenkins/workspace/swift-PR-Linux@2/branch-master/swift/test/Reflection/typeref_lowering.swift:13:19: error: CHECK-64-NEXT: expected string not found in input
10:44:46 // CHECK-64-NEXT: (struct size=16 alignment=4 stride=16 num_extra_inhabitants=0 bitwise_takable=1
10:44:46                   ^
10:44:46 <stdin>:2:1: note: scanning from here
10:44:46 (struct size=16 alignment=4 stride=16 num_extra_inhabitants=0 bitwise_takable=true
10:44:46 ^

Oops, that's my fault for wanting it to say "true".

swift-ci · 2019-10-15T17:58:22Z

Build failed
Swift Test Linux Platform
Git Sha - 8036edd

jckarter · 2019-10-15T18:08:01Z

Sorry, looks like some of the tests need to be updated for the change in the way we present bool output:

/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/Reflection/typeref_lowering.swift:13:19: error: CHECK-64-NEXT: expected string not found in input
// CHECK-64-NEXT: (struct size=16 alignment=4 stride=16 num_extra_inhabitants=0 bitwise_takable=1
                  ^
<stdin>:2:1: note: scanning from here
(struct size=16 alignment=4 stride=16 num_extra_inhabitants=0 bitwise_takable=true
^

swift-ci · 2019-10-15T18:12:56Z

Build failed
Swift Test OS X Platform
Git Sha - 8036edd

ismetanin · 2019-10-16T06:23:11Z

Sorry, I didn't retest after PR fixes, now everything should be ok 👌

Could someone help me with the development process?
I mean I changed all the tests, rebuild with ninja swift and then ninja swift-stdlib but tests are still failing with CHECK-NEXT: expected string not found in input ... bitwise_takable=1 but everywhere in tests bitwise_takable=true. Looks like I should rebuild the entire project shouldn't I?

jckarter · 2019-10-16T14:51:41Z

@swift-ci Please test

swift-ci · 2019-10-16T14:53:49Z

Build failed
Swift Test OS X Platform
Git Sha - 8036edd

swift-ci · 2019-10-16T14:53:59Z

Build failed
Swift Test Linux Platform
Git Sha - 8036edd

ismetanin changed the title ~~[SR-7165] Excise iostream from reflection~~ SR-7165. Excise iostream from reflection Sep 8, 2019

ismetanin changed the title ~~SR-7165. Excise iostream from reflection~~ SR-7165. Excise <iostream> from reflection Sep 8, 2019

theblixguy requested a review from jrose-apple September 8, 2019 15:49

gottesmm reviewed Sep 8, 2019

View reviewed changes

jckarter reviewed Oct 14, 2019

View reviewed changes

[stdlib] [reflection] Excise iostream from reflection

dd7d173

jrose-apple reviewed Oct 14, 2019

View reviewed changes

[stdlib] [reflection] Fix printing, remove & from

0d27475

[stdlib] [reflection] Fix print in dumpFieldSection

8036edd

jrose-apple self-assigned this Oct 15, 2019

jrose-apple approved these changes Oct 15, 2019

View reviewed changes

[reflection] Fix tests with bitwise_takable output

b906348

jrose-apple merged commit 52035c1 into swiftlang:master Oct 16, 2019

jrose-apple mentioned this pull request Oct 16, 2019

[reflection] Go back to 0/1 for bitwise_takable instead of false/true #27735

Merged

swift-ci mentioned this pull request Sep 8, 2019

[SR-7165] Excise <iostream> from the Reflection libraries #49713

Closed

		@@ -1,3 +1,5 @@
		append("-undefined dynamic_lookup" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)

SR-7165. Excise <iostream> from reflection #27081

SR-7165. Excise <iostream> from reflection #27081

Uh oh!

Conversation

ismetanin commented Sep 8, 2019

Uh oh!

compnerd commented Sep 8, 2019

Uh oh!

compnerd commented Sep 8, 2019

Uh oh!

swift-ci commented Sep 8, 2019

Uh oh!

swift-ci commented Sep 8, 2019

Uh oh!

gottesmm commented Sep 8, 2019

Uh oh!

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

gottesmm commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ismetanin commented Sep 8, 2019

Uh oh!

ismetanin commented Sep 8, 2019

Uh oh!

compnerd commented Sep 8, 2019

Uh oh!

ismetanin commented Sep 8, 2019

Uh oh!

jrose-apple commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ismetanin commented Sep 9, 2019

Uh oh!

ismetanin commented Sep 19, 2019

Uh oh!

jrose-apple commented Sep 19, 2019

Uh oh!

ismetanin commented Oct 1, 2019

Uh oh!

jrose-apple commented Oct 1, 2019

Uh oh!

jckarter commented Oct 1, 2019

Uh oh!

ismetanin commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jckarter commented Oct 14, 2019

Uh oh!

swift-ci commented Oct 14, 2019

Uh oh!

swift-ci commented Oct 14, 2019

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

gottesmm commented Sep 8, 2019 •

edited

Loading

jrose-apple commented Sep 9, 2019 •

edited

Loading

ismetanin commented Oct 14, 2019 •

edited

Loading

theblixguy commented Oct 14, 2019 •

edited

Loading