From 1a1238c2995faa089a82017fad95e6b7b344ecbb Mon Sep 17 00:00:00 2001 From: Kevin Day Date: Tue, 11 Jan 2022 20:30:28 -0600 Subject: [PATCH] Bugfix: If/else condition logic is incorrect or incomplete. The if/else logic is not properly traversing. I believe the original design is to just have if and else be a single set rather than a chain. At some point I changed this to be chainable (or maybe I am incorrectly thinking that I did). It made sense to use this if/then chain logic for my unit test files. The Fake make unit test files are not working as I want them to because if/else chaining is not working. As a big disclaimer, this does not support nesting. I continue to refuse to design nesting of the if/else condition logic as I want to keep this as simple as possible. Which, by the way, this is not simple. I attempt to cut corners when I did my recent rewrite of the if/else logic. Redesign this (again) but this time attempt to do this more properly. The block process state now exclusively uses a special block process state enumeration rather than relying on a synthetic rule type. The synthetic rule type is now removed. The goal with this design is to: 1) On syntax error, bail out of the entire if/else chain. 2) On comparison error, continue an if/else chain passes or the end of chain is reached. 3) When any if/else chain passes, all subsequent conditions in the chain are not run. Replace the "done" condition result with an "error" condition result to more accurately communicate its use. Isolate the block and block result from the operate processing as much as possible so that all modifications of the variable are done outside of the operate processing. When the "fail" is set to "exit" the if/else should fail appropriately on syntax error. When the "fail" is set to "warn" or "ignore", then continue on after skipping the entire chain. The operate process still needs to check the if/else chain states to determine if it should skip or not. When an if/else condition terminates unexpectedly due to an end of file (or section) an error is more consistently reported regardless of place in the if/else chain. This still needs real testing to confirm that all of the possible permutations work as expected. Unit tests for this will need to be written but this is a long way away from being done. --- level_3/fake/c/private-common.h | 31 ++++- level_3/fake/c/private-make-operate.c | 128 +++++++++++++-------- level_3/fake/c/private-make-operate_process.c | 50 +++++--- level_3/fake/c/private-make-operate_process_type.c | 16 +-- level_3/fake/c/private-make-operate_validate.c | 4 +- level_3/fake/documents/fakefile.txt | 2 +- 6 files changed, 152 insertions(+), 79 deletions(-) diff --git a/level_3/fake/c/private-common.h b/level_3/fake/c/private-common.h index e2c9bca..3736ceb 100644 --- a/level_3/fake/c/private-common.h +++ b/level_3/fake/c/private-common.h @@ -951,8 +951,7 @@ extern "C" { extern const f_string_t fake_make_operation_argument_if_success_s; enum { - fake_make_operation_if_type_done_e = 1, - fake_make_operation_if_type_else_e, + fake_make_operation_if_type_else_e = 1, fake_make_operation_if_type_if_e, fake_make_operation_if_type_if_defined_e, fake_make_operation_if_type_if_equal_e, @@ -1172,7 +1171,7 @@ extern "C" { /** * A structure for managing the operation and if-condition states. * - * block: The operation type representing the block (either "if" or "else"). + * block: The process block state. * block_result: The result of the block. * condition: The current if-condition type. * condition_result: The result of the currently processed condition. @@ -1200,20 +1199,42 @@ extern "C" { #endif // _di_fake_state_process_t_ /** + * Designate the current process block state. + * + * fake_condition_result_*: + * - none: No if-condition is set. + * - if: An if-condition is set and in use. + * - if_skip: An else-if-condition that is to be skipped. + * - next: An if-condition completed, process the "else", if found. + * - skip: An if-condition completed, skip the "else", if found. + * - else: An else-condition is set and in use. + */ +#ifndef _di_fake_state_process_block_ + enum { + fake_state_process_block_none_e = 0, + fake_state_process_block_if_e, + fake_state_process_block_if_skip_e, + fake_state_process_block_next_e, + fake_state_process_block_skip_e, + fake_state_process_block_else_e, + }; +#endif // _di_fake_state_process_block_ + +/** * Designate that the if state is true, false, or undefined (none). * * fake_condition_result_*: * - none: The result of the if-condition is not set. * - false: The result of the if-condition is false. * - true: The result of the if-condition is true. - * - done: The if-condition is done and should no longer be processed. + * - error: An error occurred, any if-condition is to be skipped, falling back to the else condition, if any. */ #ifndef _di_fake_condition_result_ enum { fake_condition_result_none_e = 0, fake_condition_result_false_e, fake_condition_result_true_e, - fake_condition_result_done_e, + fake_condition_result_error_e, }; #endif // _di_fake_condition_result_ diff --git a/level_3/fake/c/private-make-operate.c b/level_3/fake/c/private-make-operate.c index c401966..cde0d7e 100644 --- a/level_3/fake/c/private-make-operate.c +++ b/level_3/fake/c/private-make-operate.c @@ -1331,9 +1331,8 @@ extern "C" { fake_make_operate_validate(data_make, section->name, arguments, &state_process, section_stack, status); } - // Block is set to the synthetic done type when an if-condition completes in order to inform any subsequent else condition. - // If there is no follow-up else condition, then reset the block (there is no condition block at this point). - if (state_process.block == fake_make_operation_if_type_done_e) { + // If next or skip is designated and the next operation is not an "else", then this is no longer in the condition block. + if (state_process.block == fake_state_process_block_next_e || state_process.block == fake_state_process_block_skip_e) { if (state_process.operation != fake_make_operation_type_else_e) { state_process.block = 0; state_process.block_result = 0; @@ -1341,66 +1340,101 @@ extern "C" { } if (F_status_is_error(*status)) { - if (state_process.block) { + if (state_process.block || state_process.operation == fake_make_operation_type_if_e || state_process.operation == fake_make_operation_type_and_e || state_process.operation == fake_make_operation_type_or_e) { - // Reset the block information for a completely processed block. - if (state_process.block == fake_make_operation_if_type_done_e && state_process.operation != fake_make_operation_type_else_e) { - state_process.block = 0; - state_process.block_result = 0; - } - - // Always set the condition result to done on error for unfinished blocks as there is no way to determine truth at this point. - else { - state_process.block_result = fake_condition_result_done_e; + // Setting error result informs the post-process that the operate process is not run due to an error during the pre-process. + if (state_process.operation == fake_make_operation_type_if_e || state_process.operation == fake_make_operation_type_and_e || state_process.operation == fake_make_operation_type_or_e || state_process.operation == fake_make_operation_type_else_e) { + state_process.block_result = fake_condition_result_error_e; } } } else { - if (!(state_process.operation == fake_make_operation_type_if_e || state_process.operation == fake_make_operation_type_else_e || state_process.operation == fake_make_operation_type_and_e || state_process.operation == fake_make_operation_type_or_e)) { + result = fake_make_operate_process(data_make, section->name, arguments, success, &state_process, section_stack, status); - // Only process a non-if-condition operation following an if condition, if the block result is true. - if (state_process.block == fake_make_operation_if_type_if_e) { - if (state_process.block_result != fake_condition_result_true_e) { - state_process.block = fake_make_operation_if_type_done_e; + if (*status == F_child) { + f_string_dynamics_resize(0, &arguments); + + return result; + } - success = F_true; + if (state_process.block && F_status_is_error(result)) { + state_process.block_result == fake_condition_result_error_e; + } + } - continue; + // When done processing an operation within an if-condition or an else-condition, update the process state. + if (state_process.block) { + if (state_process.block == fake_state_process_block_if_e || state_process.block == fake_state_process_block_if_skip_e) { + if (state_process.operation == fake_make_operation_type_if_e || state_process.operation == fake_make_operation_type_else_e) { + state_process.block = 0; + state_process.block_result = 0; + } + else if (state_process.operation == fake_make_operation_type_and_e || state_process.operation == fake_make_operation_type_or_e) { + if (state_process.block_result == fake_condition_result_error_e) { + state_process.block_result = fake_condition_result_false_e; } } - - // Only process a non-if-condition operation following an else condition, if the block result is false. - else if (state_process.block == fake_make_operation_if_type_else_e) { - if (state_process.block_result != fake_condition_result_false_e) { - state_process.block = 0; - state_process.block_result = 0; - - success = F_true; - - continue; + else { + if (state_process.block == fake_state_process_block_if_skip_e || state_process.block == fake_state_process_block_skip_e) { + state_process.block = fake_state_process_block_skip_e; + state_process.block_result = fake_condition_result_false_e; + } + else if (state_process.block_result == fake_condition_result_false_e || state_process.block_result == fake_condition_result_error_e) { + state_process.block = fake_state_process_block_next_e; + state_process.block_result = fake_condition_result_false_e; + } + else { + state_process.block = fake_state_process_block_skip_e; } } } + else if (state_process.block == fake_state_process_block_next_e || state_process.block == fake_state_process_block_skip_e) { - result = fake_make_operate_process(data_make, section->name, arguments, success, &state_process, section_stack, status); - - if (*status == F_child) { - f_string_dynamics_resize(0, &arguments); + // Anything other than an "else" operation should not make it here. + if (state_process.block == fake_state_process_block_next_e) { + state_process.block_result = fake_condition_result_true_e; + } + else { + state_process.block_result = fake_condition_result_false_e; + } - return result; + state_process.block = fake_state_process_block_else_e; } + else if (state_process.block == fake_state_process_block_else_e) { - // When done processing an operation within a block, continue onto next block stage or exit the block. - if (!(state_process.operation == fake_make_operation_type_if_e || state_process.operation == fake_make_operation_type_else_e || state_process.operation == fake_make_operation_type_and_e || state_process.operation == fake_make_operation_type_or_e)) { - if (state_process.block == fake_make_operation_if_type_if_e) { - state_process.block = fake_make_operation_if_type_done_e; + // An "else" followed by an "if" resets the state back to an "if". + if (state_process.operation == fake_make_operation_type_if_e) { + if (state_process.block_result == fake_condition_result_true_e) { + state_process.block = fake_state_process_block_if_e; + state_process.block_result = state_process.condition_result; + } + else { + state_process.block = fake_state_process_block_if_skip_e; + state_process.block_result == fake_condition_result_false_e; + } } - else if (state_process.block == fake_make_operation_if_type_else_e) { + else { state_process.block = 0; state_process.block_result = 0; } } } + else if (state_process.operation == fake_make_operation_type_if_e || state_process.operation == fake_make_operation_type_and_e || state_process.operation == fake_make_operation_type_or_e) { + if (state_process.block_result == fake_condition_result_error_e) { + state_process.block = fake_state_process_block_if_skip_e; + state_process.block_result = fake_condition_result_false_e; + } + else { + state_process.block = fake_state_process_block_if_e; + state_process.block_result = state_process.condition_result; + } + } + else if (state_process.operation == fake_make_operation_type_else_e) { + + // An else condition by itself is invalid and the operation associated with it should be skipped. + state_process.block = fake_state_process_block_else_e; + state_process.block_result = fake_condition_result_false_e; + } if (F_status_set_fine(*status) == F_interrupt) break; @@ -1464,23 +1498,17 @@ extern "C" { return 0; } - if (i == section->objects.used && F_status_is_error_not(*status) && state_process.condition) { + if (i == section->objects.used && F_status_is_error_not(*status) && (state_process.block == fake_state_process_block_if_e || state_process.block == fake_state_process_block_if_skip_e || state_process.block == fake_state_process_block_else_e)) { if (data_make->main->error.verbosity != f_console_verbosity_quiet_e && data_make->error.to.stream) { flockfile(data_make->error.to.stream); fl_print_format("%c%[%SIncomplete '%]", data_make->error.to.stream, f_string_eol_s[0], data_make->error.context, data_make->error.prefix, data_make->error.context); - if (state_process.condition == fake_make_operation_type_if_e) { - fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_if_s, data_make->error.notable); - } - else if (state_process.condition == fake_make_operation_type_and_e) { - fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_and_s, data_make->error.notable); - } - else if (state_process.condition == fake_make_operation_type_or_e) { - fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_or_s, data_make->error.notable); + if (state_process.block == fake_state_process_block_else_e) { + fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_else_s, data_make->error.notable); } else { - fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_else_s, data_make->error.notable); + fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_if_s, data_make->error.notable); } fl_print_format("%[' at end of section.%]%c", data_make->error.to.stream, data_make->error.context, data_make->error.context, f_string_eol_s[0]); diff --git a/level_3/fake/c/private-make-operate_process.c b/level_3/fake/c/private-make-operate_process.c index 443ce2e..4fca8f0 100644 --- a/level_3/fake/c/private-make-operate_process.c +++ b/level_3/fake/c/private-make-operate_process.c @@ -19,6 +19,28 @@ extern "C" { if (*status == F_child) return data_make->main->child; + if (state_process->block) { + if (state_process->block == fake_state_process_block_if_e) { + if (!(state_process->operation == fake_make_operation_type_and_e || state_process->operation == fake_make_operation_type_or_e)) { + if (state_process->block_result == fake_condition_result_false_e) { + return 0; + } + } + } + else if (state_process->block == fake_state_process_block_if_skip_e) { + if (!(state_process->operation == fake_make_operation_type_and_e || state_process->operation == fake_make_operation_type_or_e)) { + return 0; + } + } + else if (state_process->block == fake_state_process_block_else_e) { + if (state_process->operation != fake_make_operation_type_if_e) { + if (state_process->block_result == fake_condition_result_false_e) { + return 0; + } + } + } + } + if (state_process->operation == fake_make_operation_type_index_e) { const f_status_t result = fake_execute(data_make->main, data_make->environment, data_make->setting_build.build_indexer, arguments, status); @@ -146,9 +168,7 @@ extern "C" { if (state_process->operation == fake_make_operation_type_else_e) { - // Setup the block so that subsequent operations can know the current block and the result (which is preserved across to the else condition). - state_process->block = fake_make_operation_if_type_else_e; - + // There is nothing to do. return 0; } @@ -286,26 +306,30 @@ extern "C" { *status = fake_make_operate_process_type_if_owner(data_make, arguments, F_false, state_process); } - // Setup the block for subsequent operations. - state_process->block = fake_make_operation_if_type_if_e; + // When inside an else block that is already set to false, then all if conditions inside are always considered false. + if (state_process->block == fake_state_process_block_else_e && state_process->block_result == fake_condition_result_false_e) { + return 0; + } - if (state_process->condition_result == fake_condition_result_done_e || state_process->operation == fake_make_operation_type_if_e) { - state_process->block_result = state_process->condition_result; + // When inside an if block designated to be skipped, then all if conditions inside are always considered false. + if (state_process->block == fake_state_process_block_if_skip_e) { + return 0; } - else if (state_process->operation == fake_make_operation_type_or_e) { + + if (state_process->operation == fake_make_operation_type_or_e) { if (state_process->block_result == fake_condition_result_true_e || state_process->condition_result == fake_condition_result_true_e) { - state_process->block_result = fake_condition_result_true_e; + state_process->condition_result = fake_condition_result_true_e; } else { - state_process->block_result = fake_condition_result_false_e; + state_process->condition_result = fake_condition_result_false_e; } } - else { + else if (state_process->block) { if (state_process->block_result == fake_condition_result_true_e && state_process->condition_result == fake_condition_result_true_e) { - state_process->block_result = fake_condition_result_true_e; + state_process->condition_result = fake_condition_result_true_e; } else { - state_process->block_result = fake_condition_result_false_e; + state_process->condition_result = fake_condition_result_false_e; } } diff --git a/level_3/fake/c/private-make-operate_process_type.c b/level_3/fake/c/private-make-operate_process_type.c index 179cceb..f6d5ef8 100644 --- a/level_3/fake/c/private-make-operate_process_type.c +++ b/level_3/fake/c/private-make-operate_process_type.c @@ -521,7 +521,7 @@ extern "C" { status = f_file_exists(arguments.array[i].string); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; fll_error_file_print(data_make->error, F_status_set_fine(status), "f_file_exists", F_true, arguments.array[i].string, "find", fll_error_file_type_file_e); @@ -606,7 +606,7 @@ extern "C" { status = f_file_mode_read(arguments.array[i].string, &mode_file); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; fll_error_file_print(data_make->error, F_status_set_fine(status), "f_file_mode_read", F_true, arguments.array[i].string, "get mode of", fll_error_file_type_file_e); @@ -786,7 +786,7 @@ extern "C" { } if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; if (data_make->main->error.verbosity != f_console_verbosity_quiet_e && data_make->error.to.stream) { flockfile(data_make->error.to.stream); @@ -830,7 +830,7 @@ extern "C" { status = f_file_group_read(arguments.array[i].string, &id_file); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; fll_error_file_print(data_make->error, F_status_set_fine(status), "f_file_group_read", F_true, arguments.array[i].string, "get group of", fll_error_file_type_file_e); @@ -877,7 +877,7 @@ extern "C" { status = fake_make_get_id_mode(data_make->main, data_make->error, arguments.array[if_not ? 3 : 2], &mode_rule, &mode_replace); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; return status; } @@ -885,7 +885,7 @@ extern "C" { status = f_file_mode_to_mode(mode_rule, &mode_match); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; fll_error_print(data_make->error, F_status_set_fine(status), "f_file_mode_to_mode", F_true); @@ -902,7 +902,7 @@ extern "C" { status = f_file_mode_read(arguments.array[i].string, &mode_file); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; fll_error_file_print(data_make->error, F_status_set_fine(status), "f_file_mode_read", F_true, arguments.array[i].string, "get mode of", fll_error_file_type_file_e); @@ -965,7 +965,7 @@ extern "C" { status = f_file_owner_read(arguments.array[i].string, &id_file); if (F_status_is_error(status)) { - state_process->condition_result = fake_condition_result_done_e; + state_process->condition_result = fake_condition_result_error_e; fll_error_file_print(data_make->error, F_status_set_fine(status), "f_file_owner_read", F_true, arguments.array[i].string, "get owner of", fll_error_file_type_file_e); diff --git a/level_3/fake/c/private-make-operate_validate.c b/level_3/fake/c/private-make-operate_validate.c index 2d7d902..7fb6478 100644 --- a/level_3/fake/c/private-make-operate_validate.c +++ b/level_3/fake/c/private-make-operate_validate.c @@ -445,7 +445,7 @@ extern "C" { return; } - if (state_process->block != fake_make_operation_if_type_done_e) { + if (!state_process->block) { if (data_make->error.verbosity != f_console_verbosity_quiet_e && data_make->error.to.stream) { fl_print_format("%c%[%SHas no preceding '%]", data_make->error.to.stream, f_string_eol_s[0], data_make->error.context, data_make->error.prefix, data_make->error.context); fl_print_format("%[%s%]", data_make->error.to.stream, data_make->error.notable, fake_make_operation_if_s, data_make->error.notable); @@ -568,7 +568,7 @@ extern "C" { if (state_process->operation == fake_make_operation_type_if_e || state_process->operation == fake_make_operation_type_and_e || state_process->operation == fake_make_operation_type_or_e) { if (state_process->operation == fake_make_operation_type_if_e) { - if (state_process->block == fake_make_operation_type_if_e) { + if (state_process->operation_previous == fake_make_operation_type_if_e) { if (data_make->error.verbosity != f_console_verbosity_quiet_e && data_make->error.to.stream) { flockfile(data_make->error.to.stream); diff --git a/level_3/fake/documents/fakefile.txt b/level_3/fake/documents/fakefile.txt index cca780b..4a0c719 100644 --- a/level_3/fake/documents/fakefile.txt +++ b/level_3/fake/documents/fakefile.txt @@ -42,7 +42,7 @@ Fakefile Documentation: - warn: Designates to continue on and if in verbose mode then print a warning. - ignore: Designates to do nothing. - The return code for programs can still be retrieved through using the reserved iki vaiable "return". + The return code for programs can still be retrieved through using the reserved IKI vaiable "return". - indexer\: This represents the name of the indexer program to use, such as "ar". -- 1.8.3.1