[issue1146] Validate SAS file #227

tanjaschindler · 2024-07-18T08:11:41Z

No description provided.

… InputFileLine in root_task

…n value.

…ze 1.

FlorianPommerening

I had a look at the code. I don't know the list of things this should check so I didn't verify completeness.

FlorianPommerening · 2024-07-19T11:52:46Z

src/search/utils/input_file_parser.h

+ void confirm_end_of_file();
+ // TODO: Should this be public at all? Or should we add a get_line method?
+ void error(const std::string &message) const;
+private:


Should be together with the other private members at the top of the class (with an implicit private: rather than an explicit one).

FlorianPommerening · 2024-07-19T11:56:18Z

src/search/utils/input_file_parser.h

+ bool may_start_line = true;
+ const std::regex only_whitespaces;
+public:
+ explicit InputFileParser(std::istream &stream);


Since the parser takes a stream and not a file stream, would it make sense to drop File from the name? Maybe instead describe the functionality better, for example LineInputParser?

FlorianPommerening · 2024-07-19T11:57:11Z

src/search/utils/input_file_parser.h

+ const std::regex only_whitespaces;
+public:
+ explicit InputFileParser(std::istream &stream);
+ ~InputFileParser();


We typically leave out default destructors.

FlorianPommerening · 2024-07-19T12:22:45Z

src/search/utils/input_file_parser.h

+ /*
+ Set context for error reporting.
+ */
+ void set_context(const std::string &context);


Not sure if this is still relevant, but we have classes for the command line parsing that do something similar (for example SyntaxAnalyzerContext, DecorateContext, and ConstructContext, they are different private classes for different use cases but all parallel code). They have an advantage over setting the context manually because they leave the context once they go out of scope. This way you can produce error messages with a stack trace of contexts.

One suggestion would be to use a similar design here, another (more complicated) option would be to extract the common functionality.

FlorianPommerening · 2024-07-19T14:20:36Z

src/search/utils/input_file_parser.h

+ void set_context(const std::string &context);
+ /*
+ Read a single token within a line. Tokens within a line are
+ separated by arbitrary whitespaces. Set curser to the end of the


Suggested change

separated by arbitrary whitespaces. Set curser to the end of the

separated by arbitrary whitespaces. Set cursor to the end of the

FlorianPommerening · 2024-07-19T16:07:26Z

src/search/tasks/root_task.cc

+ int num_facts = file_parser.read_line_int("number of facts in mutex group");
+ if (num_facts < 1) {
+ file_parser.error("Number of facts in mutex group is less than 1, should be at least 1.");
+ }
 vector<FactPair> invariant_group;
 invariant_group.reserve(num_facts);
 for (int j = 0; j < num_facts; ++j) {


Don't we already have a function that reads a list of facts? And doesn't it also need the check that there are no duplicate facts?

FlorianPommerening · 2024-07-19T16:09:50Z

src/search/tasks/root_task.cc

 const vector<ExplicitVariable> &variables) {
- int count;
- in >> count;
+ in.set_context(is_axiom ? "axiom section" : "operator section");


You could use a single if ... else ... here instead of two ?-operators.

that would be

int count; if (is_axiom) { in.set_context("axiom section"); count = in.read_line_int("number of axioms"); } else { in.set_context("operator section"); count = in.read_line_int("number of operators"); }

It is only personal preference but I like the variant with the ? operator more. In my opinion it emphasizes the idea of setting the context first and reading the number afterwards with slight changes of the string parameter if the is_axiom flag is true.
? also requires no declaration of count with later assignment.

FlorianPommerening · 2024-07-19T16:11:28Z

src/search/tasks/root_task.cc

- read_and_verify_version(in);
- bool use_metric = read_metric(in);
- variables = read_variables(in);
+ utils::InputFileParser file(in);


This doesn't have to be a file. How about parser?

FlorianPommerening · 2024-07-19T16:13:09Z

src/search/tasks/root_task.cc


+ // TODO: Maybe we could move this into a separate function as well


sounds good

FlorianPommerening · 2024-07-19T16:13:27Z

src/search/tasks/root_task.cc

- axioms = read_actions(in, true, use_metric, variables);
+ operators = read_actions(file, false, use_metric, variables);
+ axioms = read_actions(file, true, use_metric, variables);
+ file.confirm_end_of_file();
 /* TODO: We should be stricter here and verify that we


The TODO is fixed now.

SimonDold · 2024-07-19T11:36:08Z

src/search/tasks/root_task.cc

- ExplicitOperator(istream &in, bool is_an_axiom, bool use_metric);
+ void read_pre_post(utils::InputFileParser &in);
+ void read_axiom(utils::InputFileParser &in);
+ ExplicitOperator(utils::InputFileParser &in, bool is_an_axiom, bool use_metric);


InputFileParser is called file_parser at other places. I would prefer it to be called file_parser instead of in here (and at other places), too.

SimonDold · 2024-07-25T10:03:09Z

src/search/tasks/root_task.cc

- vector<FactPair> conditions = read_facts(in);
- int var, value_pre, value_post;
- in >> var >> value_pre >> value_post;
+void ExplicitOperator::read_axiom(utils::InputFileParser &in) {


you might want to add a flag to indicate axiom or pre_post as the read_axiom and read_pre_post are exactly the same besides the boolean flag in the read_facts call and the string in the assignment of var (and the order of the confirm_end_of_line, precondition.emplace_back which should be the same both ways).

SimonDold · 2024-07-25T10:42:04Z

src/search/tasks/root_task.cc

- in >> version;
- check_magic(in, "end_version");
+static void read_and_verify_version(utils::InputFileParser &in) {
+ in.set_context("version section");


you sometimes use spaces and other times use _ in the context string.
this should be consistent.

SimonDold · 2024-07-25T10:58:28Z

src/search/tasks/root_task.cc

 const vector<ExplicitVariable> &variables) {
- int count;
- in >> count;
+ in.set_context(is_axiom ? "axiom section" : "operator section");


that would be

int count; if (is_axiom) { in.set_context("axiom section"); count = in.read_line_int("number of axioms"); } else { in.set_context("operator section"); count = in.read_line_int("number of operators"); }

It is only personal preference but I like the variant with the ? operator more. In my opinion it emphasizes the idea of setting the context first and reading the number afterwards with slight changes of the string parameter if the is_axiom flag is true.
? also requires no declaration of count with later assignment.

SimonDold · 2024-07-25T11:13:29Z

src/search/utils/input_file_parser.h

+ separated by arbitrary whitespaces. Set curser to the end of the
+ read token.
+ */
+ std::string read(const std::string &message); // TODO: templates


combining read and read_int into one templated function to call
read<string> and read<int> instead.

SimonDold · 2024-07-25T11:18:56Z

src/search/utils/input_file_parser.h

+ Read a complete line as a single string. Set cursor to the
+ beginning of the next line.
+ */
+ std::string read_line(const std::string &message); // TODO: templates


The docu comment should add a sentence like Requires cursor to be at the beginning of the current line.

SimonDold · 2024-07-25T14:43:41Z

src/search/utils/input_file_parser.h

+ std::string line;
+ std::vector<std::string> tokens;
+ bool may_start_line = true;
+ const std::regex only_whitespaces;


Suggested change

const std::regex only_whitespaces;

constexpr std::regex only_whitespaces = "\\s*";

and remove it from the constructor.

SimonDold · 2024-07-25T15:01:30Z

src/search/utils/input_file_parser.cc

+ getline(stream, next_line);
+ ++line_number;
+ if (!regex_match(next_line, only_whitespaces)) {
+ return next_line;


find_next_line changes line_number as side effect but returns a string that is used to change line every time (except in confirm_end_of_file but would fit to do it there, too)
You could make the change of the field line a side effect there, too. This would make it a void function where it is more clear that all effects are side effects.

SimonDold · 2024-07-25T15:17:19Z

src/search/utils/input_file_parser.cc

+ if(next_line != "") {
+ error("Expected end of file, found non-empty line " + next_line);
+ }
+}


Suggested change

}

void InputFileParser::confirm_end_of_file() {

assert(may_start_line);

if (!stream.eof()) {

line = find_next_line();

error("Expected end of file, found non-empty line " + line);

}

this would also remove the need of a bool flag in find_next_line.

tanjaschindler and others added 8 commits July 12, 2024 14:10

[issue 1146] Check that the input contains a number where expected.

729fcbe

Add fact check for initial state and mutexes.

7659cd6

[Issue 1146] First version of sas file parser.

777bef8

[Issue 1146] Error handling only on InputFileParser level; get rid of…

5c189e7

… InputFileLine in root_task

[Issue 1146] Simplify InputFileParser, fix usage in RootTask.

53c5096

[Issue 1146] Add documentation to parser.

62c7e7f

[Issue1146] Check that there is at least one state variable and domai…

0ba73dc

…n value.

[Issue1146] Check that mutex groups have unique facts and at least si…

f30e600

…ze 1.

FlorianPommerening reviewed Jul 19, 2024

View reviewed changes

SimonDold reviewed Jul 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[issue1146] Validate SAS file #227

[issue1146] Validate SAS file #227

tanjaschindler commented Jul 18, 2024

FlorianPommerening left a comment

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

SimonDold Jul 25, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

SimonDold Jul 19, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

SimonDold Jul 25, 2024

	separated by arbitrary whitespaces. Set curser to the end of the
	separated by arbitrary whitespaces. Set cursor to the end of the


		// TODO: Maybe we could move this into a separate function as well

	const std::regex only_whitespaces;
	constexpr std::regex only_whitespaces = "\\s*";

-}
+void InputFileParser::confirm_end_of_file() {
+ assert(may_start_line);
+ if (!stream.eof()) {
+ line = find_next_line();
+ error("Expected end of file, found non-empty line " + line);
+ }

[issue1146] Validate SAS file #227

Are you sure you want to change the base?

[issue1146] Validate SAS file #227

Conversation

tanjaschindler commented Jul 18, 2024

FlorianPommerening left a 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

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

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