Make BatchFile Testable by MeAreJeenius · Pull Request #2618 · dosbox-staging/dosbox-staging

MeAreJeenius · 2023-06-19T07:02:42Z

There shouldn't be any functional changes except for a fix for a crash when attempting to read a batch file without read permissions on that file.

kcgen · 2023-06-21T02:08:50Z

Sorry for the delay @MeAreJeenius .. so heavy weight PRs moving through the pipes :-)

If you can look into the 'Checks' tab above:

https://github.com/dosbox-staging/dosbox-staging/pull/2618/checks

MacOS and Linux show a warning - which I think it causing link problems on Windows. Once that fixed it, CI should go green.

The refactoring and new tests look great!

Will get the review underway soon.

MeAreJeenius · 2023-06-21T03:04:45Z

@kcgen
It looks like I don't have the needed permissions on the feature branch to push a rebase with my fixes. Can that be fixed?

kcgen · 2023-06-21T03:21:30Z

@kcgen It looks like I don't have the needed permissions on the feature branch to push a rebase with my fixes. Can that be fixed?

Edit: looks like force-pushes are OK. Snipped out the noise to make way for PR-related signal :-)

These functions were never used by the Program base class

kcgen · 2023-06-26T13:42:55Z

Just checked the CI results (see the 'checks' tab above).

PVS Studio - Download, extract, and open the PR's report versus main's report, and compare the shell_cmds.cpp; the PR adds one new issue versus main (I'm just looking at the counts)

MSVC reports 40 new warnings attributed to shell_*.cpp files:

  shell_cmds.cpp  : 38
  pdcscrn.cpp     : 5
  enet.h          : 5
  debug.c         : 3
  scr_dump.c      : 2
  slk.c           : 1
  shell_misc.cpp  : 1
  shell_batch.cpp : 1
  keyname.c       : 1
  instr.c         : 1
  initscr.c       : 1
  color.c         : 1

Warnings grouped by type:

  C4996 : 55
  C4068 : 3
  C4244 : 2

Total: 60 warnings (out of 20 allowed)

I suspect the majority are #include related. Newly added .h and .cpp's should have the usual license text, #include dosbox.h followed by standard C++ includes and then project includes. Take a look at include/mouse.h and src/hardware/input/mouse.cpp to copy from :-)

kcgen · 2023-06-26T15:41:39Z

All commits are building; bisect is healthy 👍

Using gcc native file: .github/meson/native-gcc-13.ini
Using clang native file: .github/meson/native-clang.ini
Compiling 45e355721 Add BatchFile tests                     .. gcc clang
Compiling 138ef6d07 Use HostShell interface in BatchFile    .. gcc clang
Compiling 6c2006b58 Change BatchFile constructor args to str.. gcc clang
Compiling ec888bd42 Delete move constructor and assigment ex.. gcc clang
Compiling d8cdeef26 Make BatchFiles contain their echo value.. gcc clang
Compiling b57858e7c Make a DOS_Shell function to handle Batc.. gcc clang
Compiling 5d687314c Replace DOS_Shell BatchFile pointer with.. gcc clang
Compiling fc90a638f Add HostShell interface                 .. gcc clang
Compiling bd0727e8c Replace BatchFile CommandLine pointer wi.. gcc clang
Compiling 57b4a6a3f Refactor CommandLine constructor        .. gcc clang
Compiling f5fe41b6e Move CommandLine implementation to its o.. gcc clang
Compiling 2c80d5186 Use ByteReader in BatchFile             .. gcc clang
Compiling c5f8bff2b Add ByteReader interface and FileReader .. gcc clang
Compiling 4c3b7b909 Format shell.h                          .. gcc clang
Compiling 84e87eb37 Move env functions from Program to DOS_S.. gcc clang

The FileReader class is a simple helper class that reads the contents of a file and uses RAII to handle its resources. This class interface implements the ByteReader interface, which allows for non-file implentations to be substituted through polymorphism instead of having dependencies on the concrete file reader. This is especially useful for tests.

Abstracts away file management in BatchFile. Prevents a crash when attempting to read a file without the necessary permissions.

Change the constructor to take string views instead of const char* Shorten constructor body

CommandLine doesn't currently need to be used through polymorphism, so this avoids an allocation.

Allows classes which implement this to read value from an environment without the hard requirement of a DOS_Shell. Most useful for testing.

Previously, a BatchFile would store its parent Batchfile, which would then set to the DOS_Shell's BatchFile pointer upon destruction. Replacing this system with a stack of BatchFiles makes this behaviour more intuitive and easier to follow without changing the functionality.

RunInternal() and the BatchFile branch of the main loop mostly overlapped, so they have been consolidated.

Previously, BatchFiles contained their parent's echo value, which would then be applied to the shell upon destruction. Having the BatchFile contain its own echo value makes it easier to read and reason about.

Instead of having a hard dependency on DOS_Shell, BatchFiles can use any class that can query an environment variable. This makes the BatchFile class easier to test.

MeAreJeenius · 2023-06-28T06:30:24Z

@kcgen
Those warnings were caused by me removing #include "dosbox.h" from shell.h.
Why does including that header suppress those warnings, which were about usage of cstring functions?
Why do we even want those suppressed?

shermp · 2023-06-28T06:51:39Z

@kcgen Those warnings were caused by me removing #include "dosbox.h" from shell.h. Why does including that header suppress those warnings, which were about usage of cstring functions? Why do we even want those suppressed?

Because MSVC is overzealous in warning about using "unsafe" (but standard) c-string functions in favor of their own decidedly non-standard _s versions.

MeAreJeenius force-pushed the mj/testable-batch-files branch 2 times, most recently from d0232f2 to 110c882 Compare June 21, 2023 06:30

dosbox-staging deleted a comment from MeAreJeenius Jun 21, 2023

kcgen reviewed Jun 21, 2023

View reviewed changes

Comment thread src/shell/shell_misc.cpp Outdated

kcgen assigned MeAreJeenius Jun 21, 2023

kcgen added cleanup Non-functional changes that simplify, improve maintainability, or squash warnings tests Changes that verify code behavior labels Jun 21, 2023

MeAreJeenius force-pushed the mj/testable-batch-files branch 4 times, most recently from df74de6 to b4122b1 Compare June 24, 2023 04:10