Separate control flow from error reporting in command line parser #11800
Conversation
|
This needs more work :) The biggest problem is that you're modifying the wrong file - the issue is about Let me know if you need some more hints on how to continue. |
| @@ -44,6 +44,7 @@ struct InternalCompilerError: virtual util::Exception {}; | |||
| struct FatalError: virtual util::Exception {}; | |||
| struct UnimplementedFeatureError: virtual util::Exception {}; | |||
| struct InvalidAstError: virtual util::Exception {}; | |||
| struct CommandLineValidationError: virtual util::Exception {}; | |||
cameel
Aug 16, 2021
•
Member
I'd create a new Exceptions.h file in solc/ and put the exception there. liblangutil/ is for much more generic stuff and this exception will only be used by classes inside the solc/ directory.
| { | ||
| serr() << "Base path does not exist: " << m_fileReader.basePath() << endl; | ||
| return false; | ||
| BOOST_THROW_EXCEPTION(CommandLineValidationError() << errinfo_comment("Base path does not exist: ")); | ||
| } |
cameel
Aug 16, 2021
Member
These braces are no longer needed. Please remove them here and in other places. Making the code less verbose is one of the goals of this change after all.
| @@ -387,14 +387,12 @@ bool CommandLineInterface::readInputFiles() | |||
| { | |||
| if (!boost::filesystem::exists(m_fileReader.basePath())) | |||
| { | |||
| serr() << "Base path does not exist: " << m_fileReader.basePath() << endl; | |||
| return false; | |||
| BOOST_THROW_EXCEPTION(CommandLineValidationError() << errinfo_comment("Base path does not exist: ")); | |||
cameel
Aug 16, 2021
Member
The new message no longer contains m_fileReader.basePath().
Same below and some other places. Please make sure that the exceptions still contain all the information that used to be printed to stderr.
| @@ -505,15 +501,15 @@ void CommandLineInterface::createFile(string const& _fileName, string const& _da | |||
| string pathName = (m_options.output.dir / _fileName).string(); | |||
| if (fs::exists(pathName) && !m_options.output.overwriteFiles) | |||
| { | |||
| serr() << "Refusing to overwrite existing file \"" << pathName << "\" (use --overwrite to force)." << endl; | |||
| BOOST_THROW_EXCEPTION(CommandLineValidationError() << errinfo_comment("Refusing to overwrite existing file \"" << pathName << "\" (use --overwrite to force)."); | |||
| { | ||
| serr() << | ||
| "Internal compiler error during compilation:" << | ||
| endl << | ||
| boost::diagnostic_information(_exception); | ||
| return false; | ||
| BOOST_THROW_EXCEPTION(CommandLineValidationError() << errinfo_comment("Internal compiler error during compilation:" << boost::diagnostic_information(_exception)); | ||
| } |
cameel
Aug 16, 2021
•
Member
I just noticed that you're modifying CommandLineInterface. We should refactor it eventually too but we really need to have #11732 first - otherwise nothing is catching these exceptions. That's why this issue is only about CommandLineParser for now. Please remove changes from this file and start with CommandLineParser instead. The only changes here should be in CommandLineInterface::parseArguments() - that's where the exceptions should be caught.
| @@ -189,15 +189,13 @@ void CommandLineParser::printVersionAndExit() | |||
| "Version: " << | |||
| solidity::frontend::VersionString << | |||
| endl; | |||
| exit(EXIT_SUCCESS); | |||
cameel
Aug 16, 2021
•
Member
It's not as simple as just removing the exit() :) The compiler must still exit after printing this info, just not like this. You should instead make CommandLineParser::parse() return false after it runs one of these functions. But now returning false means that there was an error - which is why you first need to ensure that returning with an error is only done by raising an exception.
|
Thanks @cameel. I'm kind of embarrassed to have submitted this now :). I'll try to work on this and get back to you. Thanks for your help! |
Partially Resolves #11731
I did the following:
CommandLineParserto make them not useexit().serr()and returnsfalsewith an exception (uses theBOOST_THROW_EXCEPTIONmacro)Doesn't change too much. This is my first commit to this repository. Let me know what you think.