This is an archive of the discontinued LLVM Phabricator instance.

Decoupled Options parsing from CommandInterpreter.
ClosedPublic

Authored by tfiala on Aug 11 2016, 12:00 PM.

Details

Reviewers
clayborg
jingham
Summary

Options used to store a reference to the CommandInterpreter
instance in the base Options class. This made it impossible
to parse options independent of a CommandInterpreter.

This change removes the reference from the base class.
Instead, it modifies the options-parsing-related methods
to take an ExecutionContext pointer, which the options
may inspect if they need to do so.

Diff Detail

Event Timeline

tfiala updated this revision to Diff 67725.Aug 11 2016, 12:00 PM
tfiala retitled this revision from to Decoupled Options parsing from CommandInterpreter..
tfiala updated this object.
tfiala added reviewers: clayborg, jingham.
tfiala added a subscriber: Restricted Project.
tfiala added inline comments.Aug 11 2016, 1:00 PM
source/Interpreter/Args.cpp
553–554

We can get rid of the new PlatformSP and require_validation arguments added here if/when we have OptionValidator take an ExecutionContext *and* we add Platform to ExecutionContext.

Otherwise, we don't have enough information to do the validation job that some of the validators do. So for now, we pass in the platform.

clayborg accepted this revision.Aug 11 2016, 2:59 PM
clayborg edited edge metadata.

I like that we have decoupled option parsing from the interpreter and debugger. It will make our option parsing classes more useful. If we run into problems, we can always add a Debugger to the ExecutionContext if needed.

This revision is now accepted and ready to land.Aug 11 2016, 2:59 PM
jingham accepted this revision.Aug 11 2016, 4:32 PM
jingham edited edge metadata.

That looks great.

I think it would make more sense to add the Platform to the Execution Context than the Debugger. The point of using the Execution Context was to get around having to ask for "currently selected" things from the Debugger, so from that standpoint adding the Platform makes more sense. Plus, that better matches the hierarchy of entities involved in getting something to run somewhere, which is what the ExecutionContext represents.

I actually think that would be a good change in itself, and would clean up the validator code after your patch. But that's another big change, and this one is fine to go in as is.

tfiala closed this revision.Aug 11 2016, 5:00 PM

I actually think that would be a good change in itself, and would clean up the validator code after your patch. But that's another big change, and this one is fine to go in as is.

Excellent, thanks Jim and Greg!

I can look at doing the Platform addition to the execution context later.

Committed as r278440.