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

src/search/utils/input_file_parser.h

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.

src/search/utils/input_file_parser.h

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.

src/search/tasks/root_task.cc

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.

src/search/utils/input_file_parser.h

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.

…ion, more documentation

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

Address parts of reviews. Main changes: may_start_line is now a funct…

9ae051f

…ion, more documentation

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

SimonDold Jul 25, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening Jul 19, 2024

FlorianPommerening 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


		// 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