Community
Participate
Working Groups
Created attachment 162544 [details] patch against HEAD This is the API for an IExpressions2, an extension of IExpressions that lets a backend support cast to type and displaying arrays as directed by the CDT UI core (ICastToType, ICastToArray). See discussion in #219203 for the design discussion. This has been tested successfully with EDC, so it works great in practice. We just want to see if the gdb side can work with the concept of an ICastedExpressionDMContext interface that extends IExpressionDMContext.
Thanks for splitting out the bugs. This bug is an API change so we have a process issue to consider: 1) need to email cdt-dev upon making this change. 2) in the past releases in DSF we required a review (from another committer) before committing the change. This is not a formal CDT process but it would be a good idea. I think the patch looks good except one issue that I've come across when testing it. The cast to type actions are enabled and visible whether the underlying service supports them or not. I don't think this is acceptable, because it's really a regression for debuggers that won't implement IExpressions2. We can solve this by making ICastToType and ICastToArray accessible through getAdapter() and converting the cast actions to commands (in the same way that I convnerted the breakpoint properties action in bug 303553).
(In reply to comment #1) > I think the patch looks good except one issue that I've come across when > testing it. The cast to type actions are enabled and visible whether the > underlying service supports them or not. Oh yes, I had completely forgotten that. I'll convert it to the command framework like you mentioned. Then we can alert cdt-dev...
Created attachment 162705 [details] updated patch with CDT changes to move support into command framework Ok, this patch changes the cast to type/array and view as array commands into the new-style commands and fetches the ICastToType/Array through adapters. Tested with cdi/gdb (works), dsf/gdb (does not show up, as expected), and EDC. Along the way, since I thought I had to hook up the adapter in a different way, I moved all the DSF casting support out into a standalone class (DsfCastToTypeSupport). Turns out that wasn't necessary. But anyway, it's done, and looks cleaner. VariableVMProvider and ExpressionVMProvider will attach the DsfCastToTypeSupport at construction time to the VariableVMNode based on finding IExpressions2 support.
(In reply to comment #1) > Thanks for splitting out the bugs. This bug is an API change so we have a > process issue to consider: > 1) need to email cdt-dev upon making this change. Do I ask permission first, or just announce it after the fact? > 2) in the past releases in DSF we required a review (from another committer) > before committing the change. This is not a formal CDT process but it would be > a good idea. Randy, could you review the DSF change?
(In reply to comment #4) > Do I ask permission first, or just announce it after the fact? I think announce after the fact is enough. > > 2) in the past releases in DSF we required a review (from another committer) > > before committing the change. This is not a formal CDT process but it would be > > a good idea. > > Randy, could you review the DSF change? I'm sorry I wasn't very clear. One person's review is enough, at least until we get to the release candidates stage. But of course you're welcome to invite anyone else to review as well.
Committed patch (with minor revisions).
Created attachment 163262 [details] fixes for assertion errors Argh, I wasn't running my workbench with "-ea", and there were problems with not properly using the executor thread in actual usage. This addresses them.
Applied patch.
(In reply to comment #7) > Argh, I wasn't running my workbench with "-ea", and there were problems with > not properly using the executor thread in actual usage. I've always thought a new runtime-workbench launch config type should have that VM switch by default.
I notice that the javadoc for IExpressions2#createCastedExpression is a bit off. It mentions an rm param which does not exist. * @param rm request monitor returning a casted expression data model context object that must be passed to the appropriate * data retrieval routine to obtain the value of the expression. The object must * report the casted type (if any) via {@link #getExpressionData(IExpressionDMContext, DataRequestMonitor)} * and report alternate children according to the array casting context via * {@link #getSubExpressionCount(IExpressionDMContext, DataRequestMonitor)} * and {@link #getSubExpressions}. However, some information in that text seems valuable and should probably be kept. Ed, maybe you can shed some light on what the proper javadoc should say? Thanks
(In reply to comment #10) > I notice that the javadoc for IExpressions2#createCastedExpression is a bit > off. It mentions an rm param which does not exist. > > However, some information in that text seems valuable and should probably be > kept. > > Ed, maybe you can shed some light on what the proper javadoc should say? Oops, the "@param rm a request monitor which" became "@return". Everything else still applies. I updated the file in CVS. Thanks!
Randy, please review sometime.
