Skip to content

Commit

Permalink
Add further PR feedback
Browse files Browse the repository at this point in the history
- Use short (PascalCase) names for generic arguments
- Adapt copyright header in debug-requests
- Remove optional 'count' parameter in messaging and memory-provider
- Remove caching of session context and simply re-create if necessary
- Execute variable queries in parallel instead of sequentially
- Fix typo in comment
  • Loading branch information
martin-fleck-at committed Mar 13, 2024
1 parent 4b0c003 commit 83c51b5
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 29 deletions.
6 changes: 3 additions & 3 deletions src/common/debug-requests.ts
@@ -1,5 +1,5 @@
/********************************************************************************
* Copyright (C) 2018 Red Hat, Inc. and others.
* Copyright (C) 2024 EclipseSource.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
Expand Down Expand Up @@ -32,8 +32,8 @@ export interface DebugEvents {
'stopped': DebugProtocol.StoppedEvent
}

export type DebugRequest<COMMAND, ARGS> = Omit<DebugProtocol.Request, 'command' | 'arguments'> & { command: COMMAND, arguments: ARGS };
export type DebugResponse<COMMAND, BODY> = Omit<DebugProtocol.Response, 'command' | 'body'> & { command: COMMAND, body: BODY };
export type DebugRequest<C, A> = Omit<DebugProtocol.Request, 'command' | 'arguments'> & { command: C, arguments: A };
export type DebugResponse<C, B> = Omit<DebugProtocol.Response, 'command' | 'body'> & { command: C, body: B };
export type DebugEvent<T> = DebugProtocol.Event & { body: T };

export async function sendRequest<K extends keyof DebugRequestTypes>(session: DebugSession,
Expand Down
2 changes: 1 addition & 1 deletion src/common/messaging.ts
Expand Up @@ -29,7 +29,7 @@ export type MemoryOptions = Partial<DebugProtocol.ReadMemoryArguments>;
export type ReadMemoryArguments = DebugRequestTypes['readMemory'][0];
export type ReadMemoryResult = DebugRequestTypes['readMemory'][1];

export type WriteMemoryArguments = DebugRequestTypes['writeMemory'][0] & { count?: number };
export type WriteMemoryArguments = DebugRequestTypes['writeMemory'][0];
export type WriteMemoryResult = DebugRequestTypes['writeMemory'][1];

export type StoreMemoryArguments = MemoryOptions & { proposedOutputName?: string } | VariablesView.IVariablesContext | WebviewContext;
Expand Down
2 changes: 1 addition & 1 deletion src/plugin/adapter-registry/adapter-capabilities.ts
Expand Up @@ -112,7 +112,7 @@ export class AdapterVariableTracker implements vscode.DebugAdapterTracker {
throw new Error('To be implemented by derived classes!');
}

/** Resolves the address of a given variable in bytes withthe current context. */
/** Resolves the address of a given variable in bytes within the current context. */
getAddressOfVariable?(variableName: string, session: vscode.DebugSession): Promise<string | undefined>;

/** Resolves the size of a given variable in bytes within the current context. */
Expand Down
9 changes: 5 additions & 4 deletions src/plugin/adapter-registry/c-tracker.ts
Expand Up @@ -42,13 +42,14 @@ export class CTracker extends AdapterVariableTracker {
return undefined;
}
try {
const variableAddress = await this.getAddressOfVariable(variable.name, session);
const [variableAddress, variableSize] = await Promise.all([
variable.memoryReference && hexAddress.test(variable.memoryReference) ? variable.memoryReference : this.getAddressOfVariable(variable.name, session),
this.getSizeOfVariable(variable.name, session)
]);
if (!variableAddress) { return undefined; }
const variableSize = await this.getSizeOfVariable(variable.name, session);
if (!variableSize) { return undefined; }

const startAddress = BigInt(variableAddress);
const endAddress = BigInt(variableAddress) + variableSize;
const endAddress = variableSize !== undefined ? startAddress + variableSize : undefined;
this.logger.debug('Resolved', variable.name, { start: variableAddress, size: variableSize });
return {
name: variable.name,
Expand Down
31 changes: 15 additions & 16 deletions src/plugin/memory-provider.ts
Expand Up @@ -37,7 +37,6 @@ export class MemoryProvider {
private _onDidWriteMemory = new vscode.EventEmitter<WrittenMemory>();
public readonly onDidWriteMemory = this._onDidWriteMemory.event;

private _sessionContext: SessionContext = { canRead: false, canWrite: false };
private _onDidChangeSessionContext = new vscode.EventEmitter<SessionContext>();
public readonly onDidChangeSessionContext = this._onDidChangeSessionContext.event;

Expand All @@ -47,10 +46,6 @@ export class MemoryProvider {
constructor(protected adapterRegistry: AdapterRegistry) {
}

get sessionContext(): SessionContext {
return this._sessionContext;
}

public activate(context: vscode.ExtensionContext): void {
const createDebugAdapterTracker = (session: vscode.DebugSession): Required<vscode.DebugAdapterTracker> => {
const handlerForSession = this.adapterRegistry.getHandlerForSession(session.type);
Expand Down Expand Up @@ -105,16 +100,21 @@ export class MemoryProvider {
this.sessionClientCapabilities.delete(session.id);
}

protected setContext(session?: vscode.DebugSession): void {
const capabilities = session && this.sessionDebugCapabilities.get(session.id);
this._sessionContext = {
sessionId: session?.id,
createContext(session = vscode.debug.activeDebugSession): SessionContext {
const sessionId = session?.id;
const capabilities = sessionId ? this.sessionDebugCapabilities.get(sessionId) : undefined;
return {
sessionId,
canRead: !!capabilities?.supportsReadMemoryRequest,
canWrite: !!capabilities?.supportsWriteMemoryRequest
};
vscode.commands.executeCommand('setContext', MemoryProvider.ReadKey, this.sessionContext.canRead);
vscode.commands.executeCommand('setContext', MemoryProvider.WriteKey, this.sessionContext.canWrite);
this._onDidChangeSessionContext.fire(this.sessionContext);
}

protected setContext(session?: vscode.DebugSession): void {
const newContext = this.createContext(session);
vscode.commands.executeCommand('setContext', MemoryProvider.ReadKey, newContext.canRead);
vscode.commands.executeCommand('setContext', MemoryProvider.WriteKey, newContext.canWrite);
this._onDidChangeSessionContext.fire(newContext);
}

/** Returns the session if the capability is present, otherwise throws. */
Expand Down Expand Up @@ -145,15 +145,14 @@ export class MemoryProvider {
return sendRequest(this.assertCapability('supportsReadMemoryRequest', 'read memory'), 'readMemory', args);
}

public async writeMemory(args: DebugProtocol.WriteMemoryArguments & { count?: number }): Promise<WriteMemoryResult> {
public async writeMemory(args: DebugProtocol.WriteMemoryArguments): Promise<WriteMemoryResult> {
const session = this.assertCapability('supportsWriteMemoryRequest', 'write memory');
return sendRequest(session, 'writeMemory', args).then(response => {
const offset = response?.offset ? (args.offset ?? 0) + response.offset : args.offset;
// we accept count as an additional argument so we can skip the memory length calculation
const count = response?.bytesWritten ?? args.count ?? stringToBytesMemory(args.data).length;
if (!this.hasClientCapabilitiy(session, 'supportsMemoryEvent')) {
// we only send out a custom event if we don't expect the client to handle the memory event
// since our client is VS Code we can assume that they will always support this but better to be safe
const offset = response?.offset ? (args.offset ?? 0) + response.offset : args.offset;
const count = response?.bytesWritten ?? stringToBytesMemory(args.data).length;
this._onDidWriteMemory.fire({ memoryReference: args.memoryReference, offset, count });
}
return response;
Expand Down
2 changes: 1 addition & 1 deletion src/plugin/memory-storage.ts
Expand Up @@ -169,7 +169,7 @@ export class MemoryStorage {
memoryReference = toHexStringWithRadixMarker(address);
count = memory.length;
const data = bytesToStringMemory(memory);
await this.memoryProvider.writeMemory({ memoryReference, data, count });
await this.memoryProvider.writeMemory({ memoryReference, data });
}
await vscode.window.showInformationMessage(`Memory from '${vscode.workspace.asRelativePath(options.uri)}' applied.`);
return { memoryReference, count, offset: 0 };
Expand Down
6 changes: 3 additions & 3 deletions src/plugin/memory-webview-main.ts
Expand Up @@ -206,7 +206,7 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
const disposables = [
this.messenger.onNotification(readyType, () => {
this.setInitialSettings(participant, panel.title);
this.setSessionContext(participant, this.memoryProvider.sessionContext);
this.setSessionContext(participant, this.memoryProvider.createContext());
this.refresh(participant, options);
}, { sender: participant }),
this.messenger.onRequest(setOptionsType, o => {
Expand Down Expand Up @@ -316,7 +316,7 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
protected async storeMemory(storeArguments: StoreMemoryArguments): Promise<void> {
// Even if we disable the command in VS Code through enablement or when condition, programmatic execution is still possible.
// However, we want to fail early in case the user tries to execute a disabled command
if (!this.memoryProvider.sessionContext.canRead) {
if (!this.memoryProvider.createContext().canRead) {
throw new Error('Cannot read memory, no valid debug session.');
}
return vscode.commands.executeCommand(StoreCommandType, storeArguments);
Expand All @@ -325,7 +325,7 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
protected async applyMemory(): Promise<MemoryOptions> {
// Even if we disable the command in VS Code through enablement or when condition, programmatic execution is still possible.
// However, we want to fail early in case the user tries to execute a disabled command
if (!this.memoryProvider.sessionContext.canWrite) {
if (!this.memoryProvider.createContext().canWrite) {
throw new Error('Cannot write memory, no valid debug session.');
}
return vscode.commands.executeCommand(ApplyCommandType);
Expand Down

0 comments on commit 83c51b5

Please sign in to comment.