Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

command line arguments use single dash prefix but also accept doubledash #136

Merged
merged 3 commits into from Nov 1, 2017
Merged

command line arguments use single dash prefix but also accept doubledash #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7effeea
command line arguments use single dash prefix but also accept doubledash
kksubbu May 7, 2017
0e5d289
moved exit out of usage for clarity
kksubbu May 8, 2017
9613dc6
vm extensions also accept -- for options
kksubbu May 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions platforms/unix/vm-display-Quartz/zzz/sqUnixQuartz.m
View file
Open in desktop
Expand Up @@ -101,11 +101,7 @@
//
#undef FULLSCREEN_FADE 0.02

#ifdef PharoVM
# define VMOPTION(arg) "--"arg
#else
# define VMOPTION(arg) "-"arg
#endif

///
/// No more user-serviceable parts in this file. Stop Tweaking Now!
Expand Down
4 changes: 0 additions & 4 deletions platforms/unix/vm-display-X11/sqUnixX11.c
View file
Open in desktop
Expand Up @@ -160,11 +160,7 @@
# define xResName "squeak"
#endif

#ifdef PharoVM
# define VMOPTION(arg) "--"arg
#else
# define VMOPTION(arg) "-"arg
#endif

char *displayName= 0; /* name of display, or 0 for $DISPLAY */
Display *stDisplay= null; /* Squeak display */
Expand Down
4 changes: 0 additions & 4 deletions platforms/unix/vm-display-null/sqUnixDisplayNull.c
View file
Open in desktop
Expand Up @@ -185,11 +185,7 @@ SqDisplayDefine(null);

static void display_parseEnvironment(void) {}

#ifdef PharoVM
# define VMOPTION(arg) "--"arg
#else
# define VMOPTION(arg) "-"arg
#endif

static int display_parseArgument(int argc, char **argv)
{
Expand Down
29 changes: 15 additions & 14 deletions platforms/unix/vm/sqUnixMain.c
View file
Open in desktop
Expand Up @@ -1310,9 +1310,6 @@ static void loadModules(void)
checkModuleVersion(soundModule, SqSoundVersion, snd->version);
}

/* built-in main vm module */


static long
strtobkm(const char *str)
{
Expand Down Expand Up @@ -1345,6 +1342,7 @@ static int jitArgs(char *str)
}
#endif /* !STACKVM && !COGVM */

/* ----------------- built-in main vm module */

# include <locale.h>
static void vm_parseEnvironment(void)
Expand Down Expand Up @@ -1380,7 +1378,7 @@ static void vm_parseEnvironment(void)
}


static void usage(void);
static void usage();
static void versionInfo(void);


Expand All @@ -1398,7 +1396,8 @@ static int parseModuleArgument(int argc, char **argv, struct SqModule **addr, ch

static int vm_parseArgument(int argc, char **argv)
{
/* deal with arguments that implicitly load modules */
// parse arguments for main vm module including those that
// implicitly load modules.

if (!strncmp(argv[0], "-psn_", 5))
{
Expand All @@ -1418,13 +1417,8 @@ static int vm_parseArgument(int argc, char **argv)
return 1;
}

/* legacy compatibility */ /*** XXX to be removed at some time ***/

#ifdef PharoVM
# define VMOPTION(arg) "--"arg
#else
# define VMOPTION(arg) "-"arg
#endif

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for now, please revert this #ifdef per Eliot's feedback "IIABDFI" [1].
This will facilitate the rest of PR proceeding. (also, its mixing concerns.)

btw, What was the reason to have both work single and double dashes working in the one VM?

[1] http://forum.world.st/Re-OpenSmalltalk-opensmalltalk-vm-command-line-arguments-use-single-dash-prefix-but-also-accept-doub-td4945805.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the #ifdef because dash prefix is not a compile time option. Modules have their own parser for arguments :-(. While #ifdef seems like a simple fix for the core vm, it complicates argument parsing in modules, particularly those which are shared between Squeak and Pharo :-(. It also affects hard-coded messages like the one in imageNotFound().

# define moduleArg(arg, type, name) \
if (!strcmp(argv[0], VMOPTION(arg))) \
Expand Down Expand Up @@ -1467,7 +1461,8 @@ static int vm_parseArgument(int argc, char **argv)

/* vm arguments */

if (!strcmp(argv[0], VMOPTION("help"))) { usage(); return 1; }
if (!strcmp(argv[0], VMOPTION("help"))) { usage(); exit(0); }
else if (!strcmp(argv[0], VMOPTION("version"))) { versionInfo(); return 0; }
else if (!strcmp(argv[0], VMOPTION("noevents"))) { noEvents = 1; return 1; }
else if (!strcmp(argv[0], VMOPTION("nomixer"))) { noSoundMixer = 1; return 1; }
else if (!strcmp(argv[0], VMOPTION("notimer"))) { useItimer = 0; return 1; }
Expand All @@ -1481,7 +1476,6 @@ static int vm_parseArgument(int argc, char **argv)
else if (!strcmp(argv[0], VMOPTION("nojit"))) { useJit = 0; return 1; }
else if (!strcmp(argv[0], VMOPTION("spy"))) { withSpy = 1; return 1; }
#endif /* !STACKVM && !COGVM */
else if (!strcmp(argv[0], VMOPTION("version"))) { versionInfo(); return 1; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(deleted) Sorry I was late to understand my question here was already addressed.

else if (!strcmp(argv[0], VMOPTION("single"))) { runAsSingleInstance=1; return 1; }
/* option requires an argument */
else if (argc > 1)
Expand Down Expand Up @@ -1692,11 +1686,12 @@ SqModuleDefine(vm, Module);
/*** options processing ***/


static void usage(void)
static void usage()
{
struct SqModule *m= 0;
printf("Usage: %s [<option>...] [<imageName> [<argument>...]]\n", argVec[0]);
printf(" %s [<option>...] -- [<argument>...]\n", argVec[0]);
printf("options begin with single -, but -- prefix is silently accepted\n");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that single-hyphen default is then what is advised by --help. Why isn't it preferable to conform to Unix conventions [1] "The GNU double-hyphen option leader was chosen so that traditional single-letter options and GNU-style keyword options could be unambiguously mixed on the same command line. "
[1] http://www.catb.org/~esr/writings/taoup/html/ch10s05.html

The single-hyphen could be silently accepted for backward compatibility. Maybe better would be a deprecated message, but perhaps that could adversely affect existing scripts(?).

How do Squeakers feel about that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on limited information on the need for this, Eliot's reply "leave it as is" [1] .
Please revert and if there is pressing need, raise discussion on [vm-dev] list.

[1] http://forum.world.st/Re-OpenSmalltalk-opensmalltalk-vm-command-line-arguments-use-single-dash-prefix-but-also-accept-doub-td4945805.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the explanation in the first comment. Unix apps allow multiple single letter options to merge into one word (e.g. grep -lir for grep -l -i -r), so GNU introduced an extra dash for word options. Multi-platform virtual machines like qemu-* or squeak don't allow collapse of single letter options, so an extra dash is superfluous. It does eat into max line length without adding any value.

I have no stakes either way. I just want to fix it once, skip bike-shedding and move onto other important stuff.

sqIgnorePluginErrors= 1;
{
struct moduleDescription *md;
Expand All @@ -1721,7 +1716,6 @@ static void usage(void)
printf("\nAvailable drivers:\n");
for (m= modules; m->next; m= m->next)
printf(" %s\n", m->name);
exit(1);
}


Expand Down Expand Up @@ -1808,18 +1802,25 @@ static void parseArguments(int argc, char **argv)
{
struct SqModule *m= 0;
int n= 0;
int ddash=0;
if (!strcmp(*argv, "--")) /* escape from option processing */
break;
ddash = (argv[0][1] == '-');
if (ddash)
argv[0]++; /* skip one dash in double dash options */
modulesDo (m)
if ((n= m->parseArgument(argc, argv)))
break;
if (ddash)
argv[0]--;
# ifdef DEBUG_IMAGE
printf("parseArgument n = %d\n", n);
# endif
if (n == 0) /* option not recognised */
{
fprintf(stderr, "unknown option: %s\n", argv[0]);
usage();
exit(1);
}
while (n--)
saveArg();
Expand Down