New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert static SourceReferenceFormatter functions to member ones #3135

Merged
merged 2 commits into from Feb 19, 2018

Conversation

Projects
3 participants
@federicobond
Contributor

federicobond commented Oct 26, 2017

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

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

This comment has been minimized.

@chriseth

chriseth Oct 27, 2017

Contributor

Can you change it to a class, please?

@chriseth

chriseth Oct 27, 2017

Contributor

Can you change it to a class, please?

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

This comment has been minimized.

@chriseth

chriseth Oct 27, 2017

Contributor

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

@chriseth

chriseth Oct 27, 2017

Contributor

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

This comment has been minimized.

@chriseth

chriseth Oct 27, 2017

Contributor

Or actually just call typeName().

@chriseth

chriseth Oct 27, 2017

Contributor

Or actually just call typeName().

This comment has been minimized.

@federicobond

federicobond Oct 27, 2017

Contributor

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

@federicobond

federicobond Oct 27, 2017

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Oct 27, 2017

Contributor

@chriseth thanks! fixed.

Contributor

federicobond commented Oct 27, 2017

@chriseth thanks! fixed.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Oct 27, 2017

Contributor

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

Contributor

chriseth commented Oct 27, 2017

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

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

This comment has been minimized.

@axic

axic Nov 15, 2017

Member

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

@axic

axic Nov 15, 2017

Member

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

This comment has been minimized.

@federicobond

federicobond Nov 15, 2017

Contributor

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

@federicobond

federicobond Nov 15, 2017

Contributor

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

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Nov 21, 2017

Contributor

@chriseth anything blocking this?

Contributor

federicobond commented Nov 21, 2017

@chriseth anything blocking this?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 22, 2017

Member

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.

Member

axic commented Nov 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 22, 2017

Member

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
Member

axic commented Nov 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 22, 2017

Member

Retriggered tests, maybe that fixes it.

Member

axic commented Nov 22, 2017

Retriggered tests, maybe that fixes it.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 22, 2017

Member

@federicobond it is still failing the tests :(

Member

axic commented Nov 22, 2017

@federicobond it is still failing the tests :(

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 27, 2017

Member

Tried this locally and cannot reproduce it :(

Member

axic commented Nov 27, 2017

Tried this locally and cannot reproduce it :(

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 27, 2017

Member

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

Member

axic commented Nov 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Dec 11, 2017

Contributor

Still failing :(

Contributor

chriseth commented Dec 11, 2017

Still failing :(

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Dec 11, 2017

Contributor

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

Contributor

federicobond commented Dec 11, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 11, 2017

Member

@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.

Member

axic commented Dec 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 11, 2017

Member

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.

Member

axic commented Dec 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Dec 11, 2017

Contributor

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

Contributor

federicobond commented Dec 11, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

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

Member

axic commented Dec 12, 2017

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

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Dec 12, 2017

Contributor

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

Contributor

federicobond commented Dec 12, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

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(
  ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
Member

axic commented Dec 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 22, 2017

Member

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

Member

axic commented Dec 22, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 5, 2018

Member

Rebased again, perhaps it will work?

Member

axic commented Jan 5, 2018

Rebased again, perhaps it will work?

@axic axic added the nextrelease label Feb 14, 2018

@chriseth chriseth added this to Planned for 0.4.21 in 0.4.21 Feb 16, 2018

@chriseth chriseth moved this from Planned for 0.4.21 to In Progress in 0.4.21 Feb 16, 2018

@chriseth chriseth moved this from In Progress to Under Review in 0.4.21 Feb 19, 2018

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 19, 2018

Contributor

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

Contributor

chriseth commented Feb 19, 2018

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

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

This comment has been minimized.

@chriseth

chriseth Feb 19, 2018

Contributor

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.

@chriseth

chriseth Feb 19, 2018

Contributor

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.

This comment has been minimized.

@federicobond

federicobond Feb 19, 2018

Contributor

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

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

@federicobond

federicobond Feb 19, 2018

Contributor

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

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

This comment has been minimized.

@chriseth

chriseth Feb 19, 2018

Contributor

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

@chriseth

chriseth Feb 19, 2018

Contributor

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

@chriseth chriseth merged commit abc23ac into ethereum:develop Feb 19, 2018

6 checks passed

ci/circleci: build_emscripten Your tests passed on CircleCI!
Details
ci/circleci: build_x86 Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_external Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_solcjs Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

0.4.21 automation moved this from Under Review to Done Feb 19, 2018

@federicobond federicobond deleted the federicobond:formatter-instance branch Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment