Looks good except the launch code. See my inlined comment. BTW: to make inline comments in the code, you click and drag in the line numbers column and let go and then you can comment on specific bits of code.
|96–110 ↗||(On Diff #21599)|
You shouldn't have to do this, you should be able to leave the code like it was before.
I didn't see these options in doc: https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-File-Commands.html. Is it well-known options?
I use the the following code instead of this:
-interpreter-exec console "platform select remote-ios --sysroot \"/Users/IliaK/Library/Developer/Xcode/iOS DeviceSupport/8.1.2 (12B440)\" --build 12B440 --version 8.1.2" -interpreter-exec console "target create --arch arm64 \"~/Project1.app\""
I personally don't like using "-interpreter-exec" when there are proper MI commands available to do the job. But if start adding option/commands in lldb-mi for every options available in lldb proper then it will also get a little messy. Would it work for you to use "-interpreter-exec" as Ilia suggested above?
I'm not responsible for the MI code or its uses, so this is only a slight opinion:
It seems to me if a client has to call "interpreter-exec" with lldb-specific commands, you are already adding lldb specific options to the MI interface, just in a messy way. It would be cleaner to have some way to ask if the MI flavor is lldb, and then just add in the lldb options in an MI-pretty way. Of course, if you did it this way you should also document the extensions.
I could not find another way to do this. I agree with jignham that if the answer is to use interpret-exec, then there is a gap in the MI options. interpret-exec- output is harder to parse and is means the consumer of lldb-mi must add additional logic. While these are not officially documented flags in the gdb-mi docs, I do not think that should stop us from adding to lldb-mi where needed. I am happy to talk about a more organized way to add this ability, but simply saying this isn't in the gdb docs so we can't do it doesn't seem like a good answer.
I am all for adding new options if that make sense. But these options should be properly thought out as they get baked into the clients and then become difficult to change. So please address the comments by the other reviews and add a new text file in lldb-mi which documents these new options.
If be honest I'd prefer to add this comment into tools/lldb-mi/MICmdCmdFile.h. In that case everyone who are interesting in "-file-exec-and-symbols" will see that it has non-standard options.
There are 2 small fixes (see below):
A little fix:
const CMIUtilString m_constStrArgNamedPlatformName; // Added to support iOS platform selection
A little fix:
const CMIUtilString m_constStrArgNamedRemotePath; // Added to support iOS device remote file location
We may have more extensions in future so it would be good to have one place where all are documented. Your point is valid too. So I think we should add a comment in MICmdCmdFile.h that this command has some new options. Please see the MIExtensions.txt for details.
This patch adds some extra empty lines, please remove them. Also handle one inline comment and then feel free to commit.
I think remove the contents from "if breakpoints are set" onward (last 2 lines) as those lines are not making sense when you have not described that you can give this command without file. Also the purpose is to document the extension.