Page MenuHomePhabricator

Automate checking for "command that takes no arguments" being passed arguments in CommandObjectParsed
ClosedPublic

Authored by jingham on Jun 23 2022, 10:08 AM.

Details

Summary

At present, if a command takes no arguments, it's required to check by hand that it hasn't been passed an argument. That's error-prone and unnecessary, since CommandObject has a way for sub-classes to register what arguments they take, and that could be checked directly after the command line parse. I only do this for CommandObjectParsed. There isn't enough information to do a generic check for CommandObjectRaw, since these are explicitly unstructured commands.

This patch has four parts:

  1. The tiny bit of code in CommandObjectParsed::Execute that does the check
  2. Fixing up the handful of commands that hadn't registered their arguments (up to now this was not required)

These were pretty straightforward. I did have to fix up a bunch of the RenderScript commands. I don't know
who's responsible for the Renderscript plugin these days, so I picked one of the more recent contributors to
that file at random...

  1. Removing all the ad hoc checks for arguments passed to commands that don't take them
  2. Fixing up the tests that relied on the exact wording of the ad hoc errors

We can do a lot more with the argument info - automating completions and validation of arguments for commands that do take them. But here I'm only dealing with the automatic error generation for commands that take no arguments.

Diff Detail

Event Timeline

jingham created this revision.Jun 23 2022, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 10:08 AM
jingham requested review of this revision.Jun 23 2022, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 10:08 AM
jingham edited the summary of this revision. (Show Details)Jun 23 2022, 10:09 AM
JDevlieghere accepted this revision.Jun 23 2022, 2:29 PM
JDevlieghere added a subscriber: labath.

This is great, it both guarantees consistently and enforces command objects registering their arguments. LGTM.

For the RenderScript plugin, I remember this at an LLVM social when @labath was in town. At the time, we were very close to being able to get rid of it, which is now several years ago. Pavel, do you remember who we spoke to and if we've reached the point where this can go away?

This revision is now accepted and ready to land.Jun 23 2022, 2:29 PM

This is great, it both guarantees consistently and enforces command objects registering their arguments. LGTM.

For the RenderScript plugin, I remember this at an LLVM social when @labath was in town. At the time, we were very close to being able to get rid of it, which is now several years ago. Pavel, do you remember who we spoke to and if we've reached the point where this can go away?

I don't know what's the state of renderscript, but I think @srhines is the person you have in mind.

clayborg accepted this revision.Jun 24 2022, 2:52 PM

This is great, it both guarantees consistently and enforces command objects registering their arguments. LGTM.

For the RenderScript plugin, I remember this at an LLVM social when @labath was in town. At the time, we were very close to being able to get rid of it, which is now several years ago. Pavel, do you remember who we spoke to and if we've reached the point where this can go away?

I don't know what's the state of renderscript, but I think @srhines is the person you have in mind.

The RS-related changes here look fine for now. I'm trying to get confirmation on whether we can remove it entirely at this point (but you shouldn't do that in this patch anyways).