M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 10
Code Issues 99 Pull Requests 21 Releases Wiki Activity

Demote dead code into a stdout warning #328

Merged
sb10q merged 1 commits from issue-118 into master 2024-08-17 17:37:20 +08:00
Conversation 8 Commits 1 Files Changed 3 +44 -28
derppening commented 2023-09-29 17:52:35 +08:00
Collaborator
Copy Link

Things to consider:

  • nix build currently fails because the error message is outputted to stdout, and it is included in the diff when running check_demos.sh. Should we add a flag to suppress this warning, or change check_demos.sh to ignore outputs from the compiler?

Workaround for #118.

Things to consider: - `nix build` currently fails because the error message is outputted to stdout, and it is included in the diff when running `check_demos.sh`. Should we add a flag to suppress this warning, or change `check_demos.sh` to ignore outputs from the compiler? Workaround for #118.
derppening self-assigned this 2023-09-29 17:52:35 +08:00
derppening commented 2023-09-29 17:53:08 +08:00
Author
Collaborator
Copy Link

@sb10q Could you take a look at the "Thing to consider" section of the PR and let me know your view on this?

@sb10q Could you take a look at the "Thing to consider" section of the PR and let me know your view on this?
sb10q commented 2023-09-29 22:52:37 +08:00
Owner
Copy Link

Just add a third optional argument to run_demo.sh (and interpret_demo.sh and run_demo_lli.sh) to write the output of the NAC3 program (and the NAC3 program alone) to a file. Defaults to stdout and merged with the output so the scripts can be run manually and produce immediately visible output.

Just add a third optional argument to run_demo.sh (and interpret_demo.sh and run_demo_lli.sh) to write the output of the NAC3 program (and the NAC3 program alone) to a file. Defaults to stdout and merged with the output so the scripts can be run manually and produce immediately visible output.
derppening force-pushed issue-118 from 88587ea871 to e1e219e884 2023-10-03 11:57:54 +08:00 Compare
derppening commented 2023-10-03 11:58:31 +08:00
Author
Collaborator
Copy Link

v2: Rebased against master, added flags for redirecting/suppressing output, added forking of demo execution.

v2: Rebased against master, added flags for redirecting/suppressing output, added forking of demo execution.
derppening changed title from WIP: Demote dead code into a stdout warning to Demote dead code into a stdout warning 2023-10-03 11:58:40 +08:00
sb10q commented 2023-10-03 12:53:01 +08:00
Owner
Copy Link

added flags for redirecting/suppressing output, added forking of demo execution.

Why?

You just needed to add an argument to the run script to redirect the output of the compiled binary (and no others) to a file, everything else is unwanted complexity as far as I can tell.

> added flags for redirecting/suppressing output, added forking of demo execution. Why? You just needed to add an argument to the run script to redirect the output of the compiled binary (and no others) to a file, everything else is unwanted complexity as far as I can tell.
derppening commented 2023-10-03 13:57:49 +08:00
Author
Collaborator
Copy Link

The need for suppressing output is because this will happen:

$ ./check_demo.sh src/dead_code_issue118.py 
Checking src/dead_code_issue118.py... warning: dead code at Location { row: 3, column: 2, file: FileName("src/dead_code_issue118.py") }

warning: dead code at Location { row: 3, column: 2, file: FileName("src/dead_code_issue118.py") }

ok

Would it be better if the --redirect-exe-stdout flag also suppressed warnings, or how would you prefer this to be handled?

added forking of demo execution

This is only to speed up running the tests, on my machine the execution time reduced by 50%.

The need for suppressing output is because this will happen: ``` $ ./check_demo.sh src/dead_code_issue118.py Checking src/dead_code_issue118.py... warning: dead code at Location { row: 3, column: 2, file: FileName("src/dead_code_issue118.py") } warning: dead code at Location { row: 3, column: 2, file: FileName("src/dead_code_issue118.py") } ok ``` Would it be better if the `--redirect-exe-stdout` flag also suppressed warnings, or how would you prefer this to be handled? > added forking of demo execution This is only to speed up running the tests, on my machine the execution time reduced by 50%.
sb10q commented 2023-10-03 22:08:59 +08:00
Owner
Copy Link

The need for suppressing output is because this will happen:

Doesn't sound like a significant problem.

This is only to speed up running the tests, on my machine the execution time reduced by 50%.

But those tests run fast anyway, so it doesn't sound like it is worth the extra complexity. There are other things that are more important to optimize.

> The need for suppressing output is because this will happen: Doesn't sound like a significant problem. > This is only to speed up running the tests, on my machine the execution time reduced by 50%. But those tests run fast anyway, so it doesn't sound like it is worth the extra complexity. There are other things that are more important to optimize.
derppening force-pushed issue-118 from e1e219e884 to f2cb171982 2023-10-04 11:32:27 +08:00 Compare
derppening commented 2023-10-04 11:32:52 +08:00
Author
Collaborator
Copy Link

v3: Reverted redundant changes in v2, fixed two script issues.

v3: Reverted redundant changes in v2, fixed two script issues.
sb10q reviewed 2023-10-04 13:18:27 +08:00
nac3standalone/demo/check_demo.sh Outdated
@ -15,2 +19,2 @@
./run_demo.sh "$@" "$demo" > run.log
./run_demo_lli.sh "$@" "$demo" > run_lli.log
./run_demo.sh --redirect-exe-stdout "${nac3args[@]}" "$demo"
./run_demo_lli.sh --redirect-exe-stdout "${nac3args[@]}" "$demo"
sb10q commented 2023-10-04 13:18:27 +08:00
Owner
Copy Link

That parameter name is too long and also it's unclear that this creates run.log.

Can it be changed to something like --out run.log?

That parameter name is too long and also it's unclear that this creates ``run.log``. Can it be changed to something like ``--out run.log``?
derppening marked this conversation as resolved
sb10q reviewed 2023-10-04 13:19:16 +08:00
nac3standalone/demo/src/dead_code_issue118.py Outdated
@ -0,0 +4,4 @@
def run() -> int32:
f()
sb10q commented 2023-10-04 13:19:16 +08:00
Owner
Copy Link

remove blank line

remove blank line
derppening marked this conversation as resolved
sb10q reviewed 2023-10-04 13:20:34 +08:00
nac3standalone/demo/run_demo.sh Outdated
@ -10,0 +19,4 @@
esac
shift
done
sb10q commented 2023-10-04 13:20:34 +08:00
Owner
Copy Link

You may want to merge run_demo and run_demo_lli to reduce code duplication, and add a --lli flag.

You may want to merge run_demo and run_demo_lli to reduce code duplication, and add a --lli flag.
derppening marked this conversation as resolved
derppening force-pushed issue-118 from f2cb171982 to f34c6053d6 2023-10-04 18:11:49 +08:00 Compare
derppening commented 2023-10-04 18:12:43 +08:00
Author
Collaborator
Copy Link

v4: Merged run_demo_lli.sh with run_demo.sh, changed to --out flag.

v4: Merged `run_demo_lli.sh` with `run_demo.sh`, changed to `--out` flag.
sb10q merged commit f34c6053d6 into master 2023-10-04 21:30:37 +08:00
sb10q deleted branch issue-118 2023-10-04 21:30:38 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
future-extension

   
high-priority

   
low-priority

   
UROP0622
No Label
future-extension
high-priority
low-priority
UROP0622
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
derppening
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#328
No description provided.
Delete Branch "issue-118"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?