More tweaks to def sym editing

If you edit an existing symbol, the "is the label unique" test will
always false-positive match itself, so we have to explicitly handle
that case.  Dialogs like Edit Instruction Operand make things a bit
more complicated because they don't flush results to the symbol
table immediately, which means the symbol we pass into the Edit Def
Symbol dialog to edit isn't necessarily the one we need to exclude
from the label uniqueness test.

The dialog was using the initial value as both "original" and
"initial", which caused some issues.  We now pass both values in.

Also, removed some dead code identified by VS.
This commit is contained in:
Andy McFadden 2022-03-01 15:03:12 -08:00
parent 1e8205159e
commit 13dca8b78c
7 changed files with 103 additions and 94 deletions

View File

@ -1141,7 +1141,7 @@ namespace SourceGen {
/// <param name="start">Offset of first byte in range.</param>
/// <param name="end">Offset of last byte in range.</param>
/// <param name="charTest">Character test delegate.</param>
/// <param name="limitHiBit">If set, the high bit in all character must be the
/// <param name="limitHiBit">If set, the high bit in all characters must be the
/// same. Used to enforce a single encoding when "low or high ASCII" is used.</param>
/// <returns>Number of strings found, or -1 if bad data identified.</returns>
public static int RecognizeLen16Strings(byte[] fileData, int start, int end,

View File

@ -734,7 +734,7 @@ namespace SourceGen {
DisasmProject proj = new DisasmProject();
mDataPathName = dataPathName;
mProjectPathName = string.Empty;
byte[] fileData = null;
byte[] fileData;
try {
fileData = LoadDataFile(dataPathName);
} catch (Exception ex) {
@ -1070,7 +1070,7 @@ namespace SourceGen {
return;
}
SystemDefSet sds = null;
SystemDefSet sds;
try {
sds = SystemDefSet.ReadFile(sysDefsPath);
} catch (Exception ex) {
@ -1231,7 +1231,7 @@ namespace SourceGen {
cancel = (dataPathName == null);
return null;
}
byte[] fileData = null;
byte[] fileData;
try {
fileData = LoadDataFile(dataPathName);
} catch (Exception ex) {
@ -1877,7 +1877,6 @@ namespace SourceGen {
int firstOffset = CodeLineList[selIndex].FileOffset;
int lastOffset = CodeLineList[lastIndex].FileOffset;
int nextOffset = lastOffset + CodeLineList[lastIndex].OffsetSpan;
AddressMap addrMap = mProject.AddrMap;
// The offset of an arend directive is the last byte in the address region. It
// has a span length of zero because it's a directive, so if it's selected as
@ -2246,7 +2245,7 @@ namespace SourceGen {
} else {
// We allow the selection to include meta-data like .org and Notes.
//Debug.Assert(CodeLineList[selIndex].LineType == LineListGen.Line.Type.Data);
EditDataOperand(selOffset);
EditDataOperand();
}
}
@ -2291,22 +2290,27 @@ namespace SourceGen {
Debug.WriteLine("No change to label");
}
// Check for changes to a project property. The dialog can create a new entry or
// modify an existing entry.
if (dlg.ProjectPropertyResult != null) {
DefSymbol oldSym = dlg.PrevProjectPropertyResult;
DefSymbol newSym = dlg.ProjectPropertyResult;
ProjectProperties newProps = new ProjectProperties(mProject.ProjectProps);
// Add new entry, or replace existing entry.
if (oldSym != null) {
newProps.ProjectSyms.Remove(oldSym.Label);
// Check for changes to a project symbol. The dialog can create a new entry or
// modify an existing entry, but can't delete an entry.
if (dlg.ProjectSymbolResult != null) {
DefSymbol oldSym = dlg.OrigProjectSymbolResult;
DefSymbol newSym = dlg.ProjectSymbolResult;
if (oldSym == newSym) {
Debug.WriteLine("No actual change to project symbol");
} else {
// Generate a completely new set of project properties.
ProjectProperties newProps = new ProjectProperties(mProject.ProjectProps);
// Add new symbol entry, or replace existing entry.
if (oldSym != null) {
newProps.ProjectSyms.Remove(oldSym.Label);
}
newProps.ProjectSyms.Add(newSym.Label, newSym);
UndoableChange uc = UndoableChange.CreateProjectPropertiesChange(
mProject.ProjectProps, newProps);
cs.Add(uc);
}
newProps.ProjectSyms.Add(newSym.Label, newSym);
UndoableChange uc = UndoableChange.CreateProjectPropertiesChange(
mProject.ProjectProps, newProps);
cs.Add(uc);
} else {
Debug.WriteLine("No change to project property");
Debug.WriteLine("No change to project symbol");
}
Debug.WriteLine("EditInstructionOperand: " + cs.Count + " changes");
@ -2315,7 +2319,7 @@ namespace SourceGen {
}
}
private void EditDataOperand(int offset) {
private void EditDataOperand() {
Debug.Assert(mMainWin.CodeListView_GetSelectionCount() > 0);
TypedRangeSet trs = GroupedOffsetSetFromSelected();
@ -2396,7 +2400,7 @@ namespace SourceGen {
Debug.Assert(origDefSym.SymbolSource == Symbol.Source.Project);
EditDefSymbol dlg = new EditDefSymbol(mMainWin, mFormatter,
mProject.ProjectProps.ProjectSyms, origDefSym, null);
mProject.ProjectProps.ProjectSyms, origDefSym, origDefSym, null);
switch (col) {
case CodeListColumn.Operand:
@ -2427,7 +2431,6 @@ namespace SourceGen {
if (SelectionAnalysis.mNumItemsSelected != 1) {
return false;
}
EntityCounts counts = SelectionAnalysis.mEntityCounts;
// Single line, must be code or a RegWidth directive.
return (SelectionAnalysis.mLineType == LineListGen.Line.Type.Code ||
SelectionAnalysis.mLineType == LineListGen.Line.Type.RegWidthDirective);
@ -2455,7 +2458,6 @@ namespace SourceGen {
if (SelectionAnalysis.mNumItemsSelected != 1) {
return false;
}
EntityCounts counts = SelectionAnalysis.mEntityCounts;
// Single line, must be a visualization set or a place where one can be created.
LineListGen.Line.Type lineType = SelectionAnalysis.mLineType;
return (lineType == LineListGen.Line.Type.VisualizationSet ||
@ -2680,7 +2682,6 @@ namespace SourceGen {
Debug.Assert(thisAttr.DataDescriptor.Length == 1);
int nextOffset = rng.Low + 1;
Anattrib nextAttr = mProject.GetAnattrib(nextOffset);
// This must match what GroupedOffsetSetFromSelected() does.
if (!mProject.UserLabels.ContainsKey(nextOffset) &&
!mProject.HasCommentNoteOrVis(nextOffset) &&
@ -2963,7 +2964,6 @@ namespace SourceGen {
matchType = LineListGen.Line.Type.ArEndDirective;
}
// Find first instance of specified type.
LineListGen.Line tmpLine = CodeLineList[topLineIndex];
while (CodeLineList[topLineIndex].LineType != matchType) {
if (CodeLineList[topLineIndex].FileOffset > newLoc.Offset) {
// This can happen if the region got deleted.
@ -3666,7 +3666,6 @@ namespace SourceGen {
// header area
len = 1;
}
Anattrib attr = mProject.GetAnattrib(offset);
Debug.Assert(len > 0);
for (int i = offset; i < offset + len; i++) {
rs.Add(i);

View File

@ -501,8 +501,8 @@ namespace SourceGen {
/// <summary>
/// Creates an UndoableChange for a change to the project properties.
/// </summary>
/// <param name="oldNote">Current note.</param>
/// <param name="newNote">New note.</param>
/// <param name="oldProps">Old property set.</param>
/// <param name="newProps">New property set.</param>
/// <returns>Change record.</returns>
public static UndoableChange CreateProjectPropertiesChange(ProjectProperties oldProps,
ProjectProperties newProps) {

View File

@ -141,9 +141,14 @@ namespace SourceGen.WpfGui {
private Formatter mNumFormatter;
/// <summary>
/// Old symbol value. May be null.
/// Initial values for fields. May be null.
/// </summary>
private DefSymbol mOldSym;
private DefSymbol mInitialSym;
/// <summary>
/// Original symbol. May be null if this is a new symbol.
/// </summary>
private DefSymbol mOrigSym;
/// <summary>
/// List of existing symbols, for uniqueness check. The list will not be modified.
@ -173,19 +178,30 @@ namespace SourceGen.WpfGui {
/// Constructor, for editing a project or platform symbol.
/// </summary>
public EditDefSymbol(Window owner, Formatter formatter,
SortedList<string, DefSymbol> defList, DefSymbol defSym,
SortedList<string, DefSymbol> defList, DefSymbol origSym, DefSymbol initVals,
SymbolTable symbolTable)
: this(owner, formatter, defList, defSym, symbolTable, false, false) { }
: this(owner, formatter, defList, origSym, initVals, symbolTable, false, false) { }
/// <summary>
/// Constructor, for editing a local variable, or editing a project symbol with
/// the value field locked.
/// </summary>
/// <param name="owner">Parent window.</param>
/// <param name="formatter">Formatter object.</param>
/// <param name="defList">List of DefSymbols against which we test label uniqueness.</param>
/// <param name="origSym">Original symbol definition. The new label is allowed to
/// match this.</param>
/// <param name="initVals">Initial values for fields. This might be null, might be
/// the same as origSym, or might be the result of a previous unsaved edit.</param>
/// <param name="symbolTable">Full symbol table, for an extended uniqueness check
/// used for local variables (which must not clash with user labels).</param>
/// <param name="isVariable">Set true when editing a local variable table entry.</param>
/// <param name="lockValueAndType">Set true to prevents edits to the value and type.</param>
/// <remarks>
/// TODO(someday): disable the "constant" radio button unless CPU=65816.
/// </remarks>
public EditDefSymbol(Window owner, Formatter formatter,
SortedList<string, DefSymbol> defList, DefSymbol defSym,
SortedList<string, DefSymbol> defList, DefSymbol origSym, DefSymbol initVals,
SymbolTable symbolTable, bool isVariable, bool lockValueAndType) {
InitializeComponent();
Owner = owner;
@ -193,7 +209,8 @@ namespace SourceGen.WpfGui {
mNumFormatter = formatter;
mDefSymbolList = defList;
mOldSym = defSym;
mOrigSym = origSym;
mInitialSym = initVals;
mSymbolTable = symbolTable;
IsVariable = isVariable;
mReadOnlyValueAndType = lockValueAndType;
@ -215,24 +232,24 @@ namespace SourceGen.WpfGui {
}
private void Window_Loaded(object sender, RoutedEventArgs e) {
if (mOldSym != null) {
Label = mOldSym.GenerateDisplayLabel(mNumFormatter);
Value = mNumFormatter.FormatValueInBase(mOldSym.Value,
mOldSym.DataDescriptor.NumBase);
if (mOldSym.HasWidth) {
VarWidth = mOldSym.DataDescriptor.Length.ToString();
if (mInitialSym != null) {
Label = mInitialSym.GenerateDisplayLabel(mNumFormatter);
Value = mNumFormatter.FormatValueInBase(mInitialSym.Value,
mInitialSym.DataDescriptor.NumBase);
if (mInitialSym.HasWidth) {
VarWidth = mInitialSym.DataDescriptor.Length.ToString();
}
Comment = mOldSym.Comment;
Comment = mInitialSym.Comment;
if (mOldSym.IsConstant) {
if (mInitialSym.IsConstant) {
IsConstant = true;
} else {
IsAddress = true;
}
if (mOldSym.Direction == DefSymbol.DirectionFlags.Read) {
if (mInitialSym.Direction == DefSymbol.DirectionFlags.Read) {
IsReadChecked = true;
} else if (mOldSym.Direction == DefSymbol.DirectionFlags.Write) {
} else if (mInitialSym.Direction == DefSymbol.DirectionFlags.Write) {
IsWriteChecked = true;
} else {
IsReadChecked = IsWriteChecked = true;
@ -279,37 +296,17 @@ namespace SourceGen.WpfGui {
if (mDefSymbolList.TryGetValue(trimLabel, out DefSymbol existing)) {
// We found a match. See if we're just seeing the symbol we're editing.
// If there's no "original" symbol, then the fact that we matched anything
// means the label is not unique. Otherwise, we consider it unique if the
// label matches the original symbol.
//
// We mostly want to check the label, not the entire symbol, because otherwise
// things can go funny when multiple edits are done without flushing the data
// back to the symbol table. For example, when this is invoked from the
// Edit Project Symbol button in the instruction operand editor, the user might
// edit the comment field of an existing project symbol, hit OK here, then try
// to edit it again before closing the operand editor. In that case we get
// passed the edited DefSymbol, which no longer fully matches what's in the
// symbol table.
//
// We want to check the value as well, because of a weird case when the user
// edits an instruction operand with a user-modified symbol. For example, if
// the operand needs to be "FOO-1", so the user hand-edited the label to FOO.
// This allows the user to "create project symbol" with the symbol as the initial
// value, but the symbol would be for address FOO not FOO-1. (It would be best
// to disable "create project symbol" in this case.)
//
// TODO: we still don't handle the case where the user changes the label from
// FOO to FOO1 and then back to FOO without closing the instruction edit dialog.
// The problem is that we find a match for FOO in the symbol table without
// realizing that it was the original name before the edits began. To fix this
// we need to pass in the original label as well as the recently-edited symbol,
// and allow the new name to match either.
// If there's no "previous" symbol, then any match means the label is not unique.
// Otherwise, we consider it unique if the label and value match what they were
// when this edit dialog was opened.
//labelUnique = (existing == mOldSym);
labelUnique = mOldSym != null &&
(Asm65.Label.LABEL_COMPARER.Equals(existing.Label, mOldSym.Label) &&
existing.Value == mOldSym.Value);
// We only need to check the label. Since we're comparing the original
// symbol to the value from the symbol table, it should be a total match,
// but the other fields don't actually matter. It's safer to let the Symbol
// class comparison operators do the work though.
labelUnique = (existing == mOrigSym);
//labelUnique = mOrigSym != null &&
// Asm65.Label.LABEL_COMPARER.Equals(existing.Label, mOrigSym.Label);
} else {
labelUnique = true;
}
@ -376,7 +373,7 @@ namespace SourceGen.WpfGui {
bool valueUniqueValid = true;
if (IsVariable && valueValid && widthValid) {
foreach (KeyValuePair<string, DefSymbol> kvp in mDefSymbolList) {
if (kvp.Value != mOldSym &&
if (kvp.Value != mOrigSym &&
DefSymbol.CheckOverlap(kvp.Value, thisValue, thisWidth, symbolType)) {
valueUniqueValid = false;
break;

View File

@ -49,14 +49,19 @@ namespace SourceGen.WpfGui {
public int SymbolEditOffsetResult { get; private set; }
/// <summary>
/// Edited project property, or null if no changes were made.
/// Project symbol for this operand, when this dialog was opened. Will be null if
/// there was no matching project symbol. This is used as the "before" value for
/// updates to the project symbol set.
/// </summary>
public DefSymbol ProjectPropertyResult { get; private set; }
public DefSymbol OrigProjectSymbolResult { get; private set; }
/// <summary>
/// The project property that was modified, or null if none.
/// Edited project symbol, or null if no changes were made. This is used as the "after"
/// value for updates to the project symbol. This dialog is not allowed to delete
/// project symbols, so if this is null there's nothing to do.
/// </summary>
public DefSymbol PrevProjectPropertyResult { get; private set; }
public DefSymbol ProjectSymbolResult { get; private set; }
/// <summary>
/// Updated label.
@ -216,7 +221,7 @@ namespace SourceGen.WpfGui {
SymbolEditResult = mEditedLabel;
}
ProjectPropertyResult = mEditedProjectSymbol;
ProjectSymbolResult = mEditedProjectSymbol;
DialogResult = true;
}
@ -970,7 +975,7 @@ namespace SourceGen.WpfGui {
CreateEditProjectSymbolText = EDIT_PROJECT_SYMBOL;
mEditedProjectSymbol = (DefSymbol)firstProject;
PrevProjectPropertyResult = mEditedProjectSymbol;
OrigProjectSymbolResult = mEditedProjectSymbol;
} else {
ShowNarNoProjectMatch = true;
CreateEditProjectSymbolText = CREATE_PROJECT_SYMBOL;
@ -1019,20 +1024,25 @@ namespace SourceGen.WpfGui {
}
private void EditProjectSymbol_Click(object sender, RoutedEventArgs e) {
DefSymbol origSym = mEditedProjectSymbol;
if (origSym == null) {
DefSymbol initVals = mEditedProjectSymbol;
if (initVals == null) {
// Need to start with a symbol so we can set the value field.
string symName = "SYM";
if (!string.IsNullOrEmpty(SymbolLabel) &&
Asm65.Label.ValidateLabel(SymbolLabel)) {
symName = SymbolLabel;
}
origSym = new DefSymbol(symName, mOperandValue, Symbol.Source.Project,
initVals = new DefSymbol(symName, mOperandValue, Symbol.Source.Project,
Symbol.Type.ExternalAddr, FormatDescriptor.SubType.None);
}
// Edit the symbol, locking the value so it can only apply to this operand.
// We want to pass in the symbol as it was before we made any edits, so that
// the user can always change the label back to what it was when this dialog
// was first opened.
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter,
mProject.ProjectProps.ProjectSyms, origSym, null, false, true);
mProject.ProjectProps.ProjectSyms, OrigProjectSymbolResult, initVals, null,
false, true);
if (dlg.ShowDialog() != true) {
return;
}
@ -1218,9 +1228,12 @@ namespace SourceGen.WpfGui {
DefSymbol.DirectionFlags.ReadWrite, null, string.Empty);
}
// Unlike project symbols, we don't pass the original symbol value in. This is
// because we update a local copy of the LV table immediately, and pass that in
// for the uniqueness checks.
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter,
mEditedLvTable.GetSortedByLabel(), initialVar, mProject.SymbolTable,
true, true);
mEditedLvTable.GetSortedByLabel(), mEditedLocalVar, initialVar,
mProject.SymbolTable, true, true);
if (dlg.ShowDialog() == true) {
if (mEditedLocalVar != dlg.NewSym) {
// Integrate result. Future edits will start with this.

View File

@ -313,7 +313,7 @@ namespace SourceGen.WpfGui {
private void NewSymbolButton_Click(object sender, RoutedEventArgs e) {
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter, mWorkTable.GetSortedByLabel(),
null, mSymbolTable, true, false);
null, null, mSymbolTable, true, false);
dlg.ShowDialog();
if (dlg.DialogResult == true) {
Debug.WriteLine("ADD: " + dlg.NewSym);
@ -338,7 +338,7 @@ namespace SourceGen.WpfGui {
private void DoEditSymbol(DefSymbol defSym) {
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter, mWorkTable.GetSortedByLabel(),
defSym, mSymbolTable, true, false);
defSym, defSym, mSymbolTable, true, false);
dlg.ShowDialog();
if (dlg.DialogResult == true) {
// Label might have changed, so remove old before adding new.

View File

@ -656,8 +656,8 @@ namespace SourceGen.WpfGui {
}
private void NewSymbolButton_Click(object sender, RoutedEventArgs e) {
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter, mWorkProps.ProjectSyms, null,
null);
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter, mWorkProps.ProjectSyms,
null, null, null);
dlg.ShowDialog();
if (dlg.DialogResult == true) {
Debug.WriteLine("ADD: " + dlg.NewSym);
@ -693,7 +693,7 @@ namespace SourceGen.WpfGui {
private void DoEditSymbol(DefSymbol defSym) {
EditDefSymbol dlg = new EditDefSymbol(this, mFormatter, mWorkProps.ProjectSyms,
defSym, null);
defSym, defSym, null);
dlg.ShowDialog();
if (dlg.DialogResult == true) {
// Label might have changed, so remove old before adding new.
@ -886,7 +886,7 @@ namespace SourceGen.WpfGui {
}
private void MoveSingleItem(int selIndex, object selectedItem, int adj) {
string selected = (string)symbolFilesListBox.SelectedItem;
string selected = (string)selectedItem;
PlatformSymbolIdentifiers.Remove(selected);
PlatformSymbolIdentifiers.Insert(selIndex + adj, selected);
symbolFilesListBox.SelectedIndex = selIndex + adj;