Convert static SourceReferenceFormatter functions to member ones by federicobond · Pull Request #3135 · argotorg/solidity

federicobond · 2017-10-26T21:14:40Z

This cleans up the method signatures and also opens the door to configuring and even replacing the formatter implementations.

chriseth · 2017-10-27T07:39:18Z

libsolidity/interface/SourceReferenceFormatter.h

@@ -42,35 +42,37 @@ struct SourceReferenceFormatter
 {


Can you change it to a class, please?

chriseth · 2017-10-27T07:40:59Z

libsolidity/interface/SourceReferenceFormatter.cpp

-		_stream << line << endl;
+		m_stream << line << endl;
+
+		auto& stream = m_stream;


You can just capture this by value and then use m_value, no reason to create a local reference.

chriseth · 2017-10-27T07:42:13Z

solc/CommandLineInterface.cpp

 	m_compiler.reset(new CompilerStack(fileReader));
+
 	auto scannerFromSourceName = [&](string const& _sourceName) -> solidity::Scanner const& { return m_compiler->scanner(_sourceName); };
+	auto formatter = SourceReferenceFormatter(cerr, scannerFromSourceName);


I would prefer the (actually shorter) and more explicit version:

SourceReferenceFormatter formatter(cerr, scannerFromSourceName);

chriseth · 2017-10-27T07:43:07Z

solc/CommandLineInterface.cpp

 				*error,
-				(error->type() == Error::Type::Warning) ? "Warning" : "Error",
-				scannerFromSourceName
+				(error->type() == Error::Type::Warning) ? "Warning" : "Error"


A next refactor could pull this expression into the formatter, I would say.

Or actually just call typeName().

Yes, I did not want to rock the boat too much with this one, just lay the foundation for better encapsulation and configuration.

chriseth

Great apart from minor details.

federicobond · 2017-10-27T14:25:30Z

@chriseth thanks! fixed.

chriseth · 2017-10-27T14:53:16Z

The test failure is weird, since this PR should not change that...

axic · 2017-11-15T17:27:51Z

libsolidity/interface/SourceReferenceFormatter.cpp

 			line.cbegin(),
 			line.cbegin() + startColumn,
-			[&_stream](char const& ch) { _stream << (ch == '\t' ? '\t' : ' '); }
+			[this](char const& ch) { m_stream << (ch == '\t' ? '\t' : ' '); }


Not sure which is faster for closures, giving the entire context or just the member variable?

It's not a big object for copy, so I don't think it makes that much of a difference here.

federicobond · 2017-11-21T18:32:28Z

@chriseth anything blocking this?

axic · 2017-11-22T02:25:30Z

I guess it is fine, but with time I'd like to use StandardCompiler in the CLI too (e.g. to reduce the number of public interfaces we have) and that would mean most of these changes aren't really utilised.

axic · 2017-11-22T02:26:45Z

The blocker here was as @chriseth mentioned the test failure:

Compilation failed on:
Skipping non-existent input file ""*/*.sol""
Warning: This is a pre-release compiler version, please do not use it in production.
While calling:
"/home/travis/build/ethereum/solidity/build/solc/solc" --optimize --combined-json abi,asm,ast,bin,bin-runtime,clone-bin,compact-format,devdoc,hashes,interface,metadata,opcodes,srcmap,srcmap-runtime,userdoc announcementTypes.sol ico.sol moduleHandler.sol module.sol multiOwner.sol owned.sol premium.sol provider.sol publisher.sol safeMath.sol schelling.sol tokenDB.sol token.sol */*.sol
Inside directory:
/home/travis/build/ethereum/solidity/test/compilationTests/corion

axic · 2017-11-22T02:27:39Z

Retriggered tests, maybe that fixes it.

axic · 2017-11-22T04:40:23Z

@federicobond it is still failing the tests :(

axic · 2017-11-27T15:50:31Z

Tried this locally and cannot reproduce it :(

axic · 2017-11-27T16:04:21Z

Okay the problem is the different version of the shell, which emits the Skipping non-existent input file ""*/*.sol"" warning.

This is the code filtering out the pre-release line from the output and ensuring the output is empty afterwards: https://github.com/ethereum/solidity/blob/develop/test/cmdlineTests.sh#L75-L81

chriseth · 2017-12-11T11:04:48Z

Still failing :(

federicobond · 2017-12-11T16:33:08Z

@axic I did not understand your last comment fully. Is the shell not globbing one of the function arguments?

axic · 2017-12-11T16:35:42Z

@federicobond I had that hunch, but then I think it was something else but related, unfortunately had to leave the task and don't fully remember.

I am sure it had something to do with the shell or the script.

axic · 2017-12-11T16:40:53Z

This is the output:

Compilation failed on:
Skipping non-existent input file ""*/*.sol""
Warning: This is a pre-release compiler version, please do not use it in production.

The script removes any line containing pre-release. If there are any lines remaining in the output it is a failure. Whoever emits that "Skipping non-existent" (it must be the shell as it is not solc) is causing it to fail.

federicobond · 2017-12-11T16:42:40Z

Sounds like solc is receiving a literal */*.sol as argument. Perhaps shellcheck and/or some reading can clarify.

axic · 2017-12-12T02:25:52Z

@federicobond can you try to fix it? Would be keen merging this.

federicobond · 2017-12-12T02:45:41Z

Let me try... (Travis being so slow today doesn't help much)

axic · 2017-12-12T17:10:55Z

Fails compilation on:

/home/travis/build/ethereum/solidity/test/libjulia/Common.cpp:44:29: error: call to non-static member function without an object argument
  SourceReferenceFormatter::printExceptionInformation(
  ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

axic · 2017-12-22T12:26:08Z

This is so frustrating, the change should have nothing to do with that test failure.

axic · 2018-01-05T00:39:47Z

Rebased again, perhaps it will work?

chriseth · 2018-02-19T14:15:57Z

Ok, fingers crossed. I was not able to reproduce it, but I might have the fix. Explanation inline.

chriseth · 2018-02-19T14:18:19Z

solc/CommandLineInterface.cpp

 	m_compiler.reset(new CompilerStack(fileReader));
+
 	auto scannerFromSourceName = [&](string const& _sourceName) -> solidity::Scanner const& { return m_compiler->scanner(_sourceName); };
+	SourceReferenceFormatter formatter(cerr, scannerFromSourceName);


The type of a lambda function is different from std::function, you can only implicitly convert into std::function. This means since auto is used above, the type of scannerFromSourceNam is not std::function. Since formatter takes a const&, the type is converted on the fly, creating a temporary copy of scannerFromSourceName, converted to std::function. Since SourceReferenceFormatter (used to) store a const&, it stores a const& to this temporary which is destroyed after this line and thus invalid. I changed it to storing a value instead of a reference.

That’s awesome 👏 . Thanks for taking the time to review this Chris!

I should definitely improve my understanding of that part of the language.

Yeah, that's the problem with C++: It does not warn you about ownership problems.

chriseth reviewed Oct 27, 2017

View reviewed changes

chriseth suggested changes Oct 27, 2017

View reviewed changes

federicobond force-pushed the formatter-instance branch from fb91b58 to cac378a Compare October 27, 2017 13:44

chriseth approved these changes Oct 27, 2017

View reviewed changes

axic reviewed Nov 15, 2017

View reviewed changes

federicobond force-pushed the formatter-instance branch from cac378a to fa5177c Compare November 22, 2017 04:54

federicobond force-pushed the formatter-instance branch from fa5177c to b87b359 Compare November 30, 2017 17:38

federicobond force-pushed the formatter-instance branch from 328759a to c7e4499 Compare December 12, 2017 18:11

axic force-pushed the formatter-instance branch from c7e4499 to d3d762a Compare January 5, 2018 00:39

federicobond force-pushed the formatter-instance branch from d3d762a to efba425 Compare January 5, 2018 17:15

axic force-pushed the formatter-instance branch from efba425 to 886d716 Compare February 12, 2018 23:05

axic added the nextrelease label Feb 14, 2018

federicobond and others added 2 commits February 19, 2018 15:04

Convert static SourceReferenceFormatter functions to member ones

d7532f7

Fix scannerFromSourceName use-after-free.

305d5f7

chriseth force-pushed the formatter-instance branch from 886d716 to 305d5f7 Compare February 19, 2018 14:15

chriseth reviewed Feb 19, 2018

View reviewed changes

chriseth merged commit abc23ac into argotorg:develop Feb 19, 2018

federicobond deleted the formatter-instance branch February 20, 2018 18:35

Conversation

federicobond commented Oct 26, 2017 • 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!

chriseth Oct 27, 2017 • 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!

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!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

federicobond commented Oct 27, 2017

Uh oh!

chriseth commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

federicobond commented Nov 21, 2017

Uh oh!

axic commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Nov 22, 2017

Uh oh!

axic commented Nov 22, 2017

Uh oh!

axic commented Nov 22, 2017

Uh oh!

axic commented Nov 27, 2017

Uh oh!

axic commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriseth commented Dec 11, 2017

Uh oh!

federicobond commented Dec 11, 2017

Uh oh!

axic commented Dec 11, 2017

Uh oh!

axic commented Dec 11, 2017

Uh oh!

federicobond commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Dec 12, 2017

Uh oh!

federicobond commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Dec 12, 2017

Uh oh!

axic commented Dec 22, 2017

Uh oh!

axic commented Jan 5, 2018

Uh oh!

chriseth commented Feb 19, 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!

Reviewers

Assignees

Labels

Projects

Milestone

federicobond commented Oct 26, 2017 •

edited

Loading

chriseth Oct 27, 2017 •

edited

Loading

axic commented Nov 22, 2017 •

edited

Loading

axic commented Nov 27, 2017 •

edited

Loading

federicobond commented Dec 11, 2017 •

edited

Loading

federicobond commented Dec 12, 2017 •

edited

Loading