CommandLineParser #11518

cameel · 2021-06-10T17:49:27Z

Related to #11386. Partial solution for #5303.
~~Depends on #11571.~~ Merged.

The PR extracts the logic responsible for interpreting and validating command-line options from CommandLineInterface into a new class: CommandLineParser. The new class has clearly separated input and output and therefore can be easily tested.

Putting #11386 on top of this PR will let me actually have the whole path-related behavior covered by some sensible tests rather than just the part that has been extracted into FileReader. Hopefully it will also be a good first step for the full CLI refactor in the future.

To make this easier to review I tried to keep changes to the absolute minimum. I found some things that could be cleaned up or fixed but I'll be submitting these in separate PRs to keep the diff here understandable. I recommend reviewing individual commits and skipping the first one, which only copies CommandLineParser.cpp/.h without any modifications. This way you'll see in the second one that actual changes are pretty small and most of the churn is the removal of fragments that belong in the other class and switching to getting values from the new options struct.

chriseth · 2021-06-17T12:19:33Z

solc/CommandLineParser.cpp

+bool CommandLineOptions::operator==(CommandLineOptions const& _other) const
+{
+	return
+		sourceFilePaths == _other.sourceFilePaths &&


Do we really need this equality operator? Isn't it very dangerous in case we forget to add some new setting here?

The alternative is comparing individual properties one by one, with the same risk.

I thought about adding static_assert for the size of the struct like I did in operators for the structs containing booleans but the assert was almost as big as the comparison itself so I thought it was too much. It would decrease the risk a lot though.

Possible using something like here could help?

https://stackoverflow.com/questions/17660095/iterating-over-a-struct-in-c

I see that solutions boil down to writing a bunch of custom macros or using Boost.Fusion (which is also a bunch of macros and templates :)). I'd be fine with Boost.Fusion - looks like it would solve the problem of operators potentially going out of sync and usage is not complicated. Still, it adds a bunch of weird macro calls and I'd still have to repeat the list of members once (in macro invocation). But that's not a bad trade-off in my opinion. @chriseth What's your opinion on using Boost.Fusion?

Not sure about that, especially since we already have the code here. I also heard that fusion is a beast. About going out of sync: Would an assert using sizeof not be enough?

I think the assert is good enough but this particular function does not have one. I'd have to list all field types from CommandLineOptions in it and there's a lot of them. I considered adding it but in the end I thought that an assert the size of the whole function might be an overkill :)

Also, C++20 will provide default equality operators which would be a much cleaner solution.

chriseth · 2021-06-17T12:41:10Z

test/solc/CommandLineParser.cpp

+	CommandLineOptions expectedOptions;
+	expectedOptions.sourceFilePaths = {"contract.sol"};
+	expectedOptions.coloredOutput = true;
+	expectedOptions.expectedExecutionsPerDeployment = 200;


This is a magic number that should appear exactly once in the whole project source.

It does appear once in non-test code and I think that's the part that matters. But I'll make it a constant anyway. Less duplication is always good.

I mean the question is what do we want to test? Do we want to test that it is the number 200 or do we want to test that the parser sets it to OptimiserSettings{}.expectedExecutionsPerDeployment?

I'm often OK with hard-coding some things in tests because these values get cross-checked so you have to make a mistake in both places to miss the problem. Deduplicating stuff too much sometimes makes it easy for the wrong value to be carried over to the check in the test so I often prefer to see the "raw" values in tests.

That's my general approach but I get your point about this constant. Expecting a particular raw value is not the point of this test so we would indeed be better off with a constant. The problem is - this value is hard-coded all over the code base. I think this PR is not the best place to fix that. I have created a separate one: #11550.

cameel · 2021-06-17T14:06:30Z

I just pushed an updated version. It only changes the handling of the coloredOutput option to make it independent of the TTY. Checking the TTY is now done after option parsing, in CommandLineInterface. This should fix the test failures on Windows.

Once I see that it works, I'll remove the debug commits with stream operators and mark the PR as reviewable.

cameel · 2021-06-17T14:55:25Z

Still failing. This time because I missed one use of isatty() (for help text). This is a bit annoying because these things only fail on
Windows (probably becase on Linux isatty() already gets included through other headers and on Windows we need to define it).

I just pushed a fix. I changed CommandLineParser::parse() to take an argument saying whether we have an interactive terminal.

cameel · 2021-06-17T15:41:09Z

All tests passed so I'm done here. Marking as reviewable.

libsolidity/formal/ModelCheckerSettings.h

solc/CommandLineInterface.cpp

christianparpart · 2021-06-21T11:09:36Z

solc/CommandLineParser.cpp

+	catch (FileNotFound const&)
+	{
+		// Should not happen if `fs::is_regular_file` is correct.
+	}


For the filesystem_error case we could generate a human readable message so the user knows why it failed. If the catch below should not happen, we could make it solAssert(false, "reason here");?

Agreed. Again, something I did not want to touch yet in this PR.

christianparpart · 2021-06-21T11:12:36Z

solc/CommandLineParser.cpp

+	vector<string> libraries;
+	boost::split(libraries, data, boost::is_space() || boost::is_any_of(","), boost::token_compress_on);
+	for (string const& lib: libraries)
+		if (!lib.empty())


Huge nested if. What about:

for (string const& lib: libraries | ranges::views::filter([](auto const& lib) { return lib.empty(); }) // .... (now without the huge if-body)...

Agreed. Again, something I did not want to touch yet in this PR.

solc/CommandLineParser.cpp

solc/CommandLineParser.h

test/CMakeLists.txt

cameel · 2021-06-21T12:15:34Z

Fixed about half of the small suggestions. The other half is fine but I'd like to leave them out of this PR. This one was meant to be mostly a split and not a complete cleanup of CommandLineInterface. There are too many small things to fix and I wanted to keep the PR manageable.

There are two changes I have not done yet: the CMakeLists.txt suggestion (#11518 (comment)) and checking compilation result (#11518 (comment)).

Marenz

Looks good overall, found a few minor things.
Didn't check the fixup! ones yet.

solc/CommandLineParser.h

Marenz · 2021-06-21T12:05:08Z

solc/CommandLineParser.cpp

+	static_assert(
+		sizeof(*this) == 15 * sizeof(bool),
+		"Remember to update code below if you add/remove fields."
+	);


Marenz · 2021-06-21T12:08:14Z

solc/CommandLineParser.cpp

+bool CommandLineOptions::operator==(CommandLineOptions const& _other) const
+{
+	return
+		sourceFilePaths == _other.sourceFilePaths &&


Possible using something like here could help?

https://stackoverflow.com/questions/17660095/iterating-over-a-struct-in-c

test/solc/CommandLineParser.cpp

christianparpart

@cameel LGTM plus the rest where you said it's for future PRs, which I agree with (to get that one through) :).

christianparpart · 2021-07-05T08:50:52Z

@cameel well, a squash would make sense, I guess :)

solc/CommandLineInterface.cpp

cameel · 2021-07-05T12:10:56Z

Fixups squashed.

cameel · 2021-07-06T15:17:59Z

Just pushed the fix for the test -> text typo and squashed a fixup. No other changes.

cameel self-assigned this Jun 10, 2021

cameel mentioned this pull request Jun 10, 2021

Local output streams in CommandLineParser #11520

Merged

cameel force-pushed the command-line-parser branch from 06297f3 to fcab7e0 Compare June 10, 2021 18:23

cameel marked this pull request as draft June 14, 2021 13:14

cameel force-pushed the command-line-parser branch 2 times, most recently from 3f499a6 to 305eed6 Compare June 16, 2021 20:57

chriseth reviewed Jun 17, 2021

View reviewed changes

cameel force-pushed the command-line-parser branch from 305eed6 to e070441 Compare June 17, 2021 14:03

cameel mentioned this pull request Jun 17, 2021

Remove arg aliases. #11548

Merged

cameel force-pushed the command-line-parser branch from e070441 to 2cf6e62 Compare June 17, 2021 14:51

cameel mentioned this pull request Jun 17, 2021

Constant for default --runs #11550

Merged

cameel force-pushed the command-line-parser branch from 2cf6e62 to 924430b Compare June 17, 2021 15:40

cameel marked this pull request as ready for review June 17, 2021 15:41

cameel requested review from Marenz, christianparpart and aarlt June 17, 2021 15:42

cameel mentioned this pull request Jun 21, 2021

Stripping base path from CLI paths #11545

Merged

6 tasks

christianparpart suggested changes Jun 21, 2021

View reviewed changes

cameel force-pushed the command-line-parser branch from 924430b to dab3783 Compare June 21, 2021 12:09

Marenz suggested changes Jun 21, 2021

View reviewed changes

cameel force-pushed the command-line-parser branch from dab3783 to 2aa689f Compare June 22, 2021 15:53

cameel mentioned this pull request Jun 22, 2021

Create libs for yul-phaser and solc to avoid recompiling their sources for soltest #11571

Merged

cameel changed the base branch from develop to libsolcli-and-libphaser June 22, 2021 15:55

cameel requested review from Marenz and christianparpart June 22, 2021 16:04

cameel force-pushed the command-line-parser branch 2 times, most recently from 82198a0 to 7200fda Compare July 2, 2021 19:58

christianparpart reviewed Jul 5, 2021

View reviewed changes

chriseth reviewed Jul 5, 2021

View reviewed changes

solc/CommandLineInterface.cpp Outdated Show resolved Hide resolved

cameel force-pushed the command-line-parser branch from 7200fda to 50e1b7a Compare July 5, 2021 12:07

christianparpart previously approved these changes Jul 5, 2021

View reviewed changes

cameel dismissed christianparpart’s stale review via de7881c July 5, 2021 19:06

cameel force-pushed the command-line-parser branch from 50e1b7a to de7881c Compare July 5, 2021 19:06

cameel mentioned this pull request Jul 5, 2021

Stripping base path from CLI paths (without CLI refactor) #11617

Closed

cameel force-pushed the command-line-parser branch from de7881c to 61e994b Compare July 6, 2021 12:45

cameel mentioned this pull request Jul 6, 2021

Inconsistent library address validations between CLI and Standard JSON #10299

Open

Marenz previously approved these changes Jul 6, 2021

View reviewed changes

cameel added 5 commits July 6, 2021 17:17

Copy CommandLineInterface.cpp/.h to CommandLineParser.cpp/.h

b0c7088

Extract CommandLineParser out of CommandLineInterface

d781955

Equality operators for ModelCheckerSettings and ImportRemapper

7d16c7b

CommandLineParser: Equality operators for settings

c17ee4f

Tests for all input modes in CommandLineParser

ef7abd2

cameel dismissed Marenz’s stale review via ef7abd2 July 6, 2021 15:17

cameel force-pushed the command-line-parser branch from 61e994b to ef7abd2 Compare July 6, 2021 15:17

Marenz approved these changes Jul 6, 2021

View reviewed changes

cameel mentioned this pull request Jul 6, 2021

Make --pretty-json work with Standard JSON output #11625

Closed

cameel merged commit ce11ebb into develop Jul 7, 2021

cameel deleted the command-line-parser branch July 7, 2021 10:51

cameel mentioned this pull request Jul 13, 2021

[solidity-upgrade] Input file processing is out of sync with solc #11651

Closed

This was referenced Jul 27, 2021

Better error message when base path does not exist #11713

Merged

[CLI] Make more settings optional in CommandLineOptions #11727

Closed

[CLI] Separate control flow from error reporting in CommandLineParser #11731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CommandLineParser #11518

CommandLineParser #11518

cameel commented Jun 10, 2021 •

edited

Loading

chriseth Jun 17, 2021

cameel Jun 17, 2021

Marenz Jun 21, 2021

cameel Jun 22, 2021

chriseth Jun 23, 2021

cameel Jun 23, 2021

chriseth Jun 17, 2021

cameel Jun 17, 2021

chriseth Jun 17, 2021

cameel Jun 17, 2021 •

edited

Loading

cameel commented Jun 17, 2021

cameel commented Jun 17, 2021

cameel commented Jun 17, 2021

christianparpart Jun 21, 2021

cameel Jun 21, 2021

christianparpart Jun 21, 2021

cameel Jun 21, 2021

cameel commented Jun 21, 2021

Marenz left a comment

Marenz Jun 21, 2021

Marenz Jun 21, 2021

christianparpart left a comment

christianparpart commented Jul 5, 2021

cameel commented Jul 5, 2021

cameel commented Jul 6, 2021 •

edited

Loading

CommandLineParser #11518

CommandLineParser #11518

Conversation

cameel commented Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

cameel commented Jun 17, 2021

cameel commented Jun 17, 2021

cameel commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Jun 21, 2021

Marenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianparpart left a comment

Choose a reason for hiding this comment

christianparpart commented Jul 5, 2021

cameel commented Jul 5, 2021

cameel commented Jul 6, 2021 • edited Loading

cameel commented Jun 10, 2021 •

edited

Loading

cameel Jun 17, 2021 •

edited

Loading

cameel commented Jul 6, 2021 •

edited

Loading