Skip to content

Closes #791: Remove duplicated check table in MultiTypeSymbolTable #792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mhmerrill merged 1 commit into from
May 7, 2021
Merged

Closes #791: Remove duplicated check table in MultiTypeSymbolTable #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 28 additions & 92 deletions src/MultiTypeSymbolTable.chpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,7 @@ module MultiTypeSymbolTable
}

proc regName(name: string, userDefinedName: string) throws {

// check to see if name is defined
if (!tab.contains(name)) {
mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"regName: undefined symbol: %s".format(name));
throw getErrorWithContext(
msg=unknownSymbolError("regName", name),
lineNumber=getLineNumber(),
routineName=getRoutineName(),
moduleName=getModuleName(),
errorClass="UnknownSymbolError");
}
checkTable(name, "regName");

// check to see if userDefinedName is already defined, with in-place modification, this will be an error
if (registry.contains(userDefinedName)) {
Expand Down Expand Up @@ -76,25 +65,13 @@ module MultiTypeSymbolTable
}

proc unregName(name: string) throws {

// check to see if name is defined
if !tab.contains(name) {
mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"unregName: undefined symbol: %s ".format(name));
throw getErrorWithContext(
msg=unknownSymbolError("regName", name),
lineNumber=getLineNumber(),
routineName=getRoutineName(),
moduleName=getModuleName(),
errorClass="UnknownSymbolError");
checkTable(name, "unregName");
if registry.contains(name) {
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"Unregistering symbol: %s ".format(name));
} else {
if registry.contains(name) {
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"Unregistering symbol: %s ".format(name));
} else {
mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"The symbol %s is not registered".format(name));
}
mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"The symbol %s is not registered".format(name));
}

registry -= name; // take name out of registry
Expand Down Expand Up @@ -199,32 +176,17 @@ module MultiTypeSymbolTable
:returns: bool indicating whether the deletion occurred
*/
proc deleteEntry(name: string): bool throws {
if tab.contains(name) {
if !registry.contains(name) {
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"Deleting unregistered entry: %s".format(name));
tab.remove(name);
return true;
} else {
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"Skipping registered entry: %s".format(name));
return false;
}
checkTable(name, "deleteEntry");
if !registry.contains(name) {
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"Deleting unregistered entry: %s".format(name));
tab.remove(name);
return true;
} else {
if registry.contains(name) {
mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"Registered entry is not in SymTab: %s".format(name));
} else {
Comment on lines 185 to -217
Copy link
Member Author

@stress-tess stress-tess May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition in deleteEntry is one area that I was unsure about. This case of "registered object that's not in the symbol table" is no longer captured and logged since the generic "in symbol table check" is done before checking in registry. I could switch the order and add the logging back in if people want. My thinking was this shouldn't happen and the code is cleaner without multiple cases

Copy link
Collaborator

@glitch glitch May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand this correctly: In both implementations if the item is not in the SymTab we're still going to throw an error. So the difference here is whether we log additional information about the missing item (whether it was registered or not).

So my (rhetorical) question is what would we do with that additional information. Is it going to help us determine what happened / what led to the item being missing? I'm not sure, I think I'm leaning towards no it's not really helpful, but I'm still a bit of a newb here.

If we decided that extra logging information is useful, I'd probably wrap the checkTable call in a try/catch, log the registry info and then re-throw the error.

mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"Unregistered entry is not in SymTab: %s".format(name));
}
throw getErrorWithContext(
msg=unknownSymbolError(pname="deleteEntry", sname=name),
lineNumber=getLineNumber(),
routineName=getRoutineName(),
moduleName=getModuleName(),
errorClass="UnknownSymbolError");
}
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"Skipping registered entry: %s".format(name));
return false;
}
}

/*
Expand All @@ -247,24 +209,14 @@ module MultiTypeSymbolTable
:throws: `unkownSymbolError(name)`
*/
proc lookup(name: string): borrowed GenSymEntry throws {
if (!tab.contains(name)) {
throw getErrorWithContext(
msg=unknownSymbolError(pname="lookup", sname=name),
lineNumber=getLineNumber(),
routineName=getRoutineName(),
moduleName=getModuleName(),
errorClass="UnknownSymbolError");
} else {
mtLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"found symbol: %s".format(name));
return tab.getBorrowed(name);
}
checkTable(name, "lookup");
return tab.getBorrowed(name);
}

/*
checks to see if a symbol is defined if it is not it throws an exception
*/
proc check(name: string, calling_func="check") throws {
proc checkTable(name: string, calling_func="check") throws {
if (!tab.contains(name)) {
mtLogger.error(getModuleName(),getRoutineName(),getLineNumber(),
"undefined symbol: %s".format(name));
Expand Down Expand Up @@ -307,16 +259,9 @@ module MultiTypeSymbolTable
proc dump(name:string): string throws {
if name == "__AllSymbols__" {
return "%jt".format(this);
} else if (tab.contains(name)) {
return "%jt %jt".format(name, tab.getReference(name));
} else {
throw getErrorWithContext(
msg=unknownSymbolError(pname="dump",sname=name),
lineNumber=getLineNumber(),
routineName=getRoutineName(),
moduleName=getModuleName(),
errorClass="UnknownSymbolError");
}
}
checkTable(name, "dump");
return "%jt %jt".format(name, tab.getReference(name));
}

/*
Expand Down Expand Up @@ -379,7 +324,7 @@ module MultiTypeSymbolTable
var i = 0;
for name in infoList {
i+=1;
check(name);
checkTable(name);
entries[i] = formatEntry(name, tab.getBorrowed(name));
}
return entries;
Expand Down Expand Up @@ -411,20 +356,11 @@ module MultiTypeSymbolTable
:returns: s (string) containing info
*/
proc attrib(name:string):string throws {
checkTable(name, "attrib");
var s:string;
if (tab.contains(name)) {
s = "%s %s %t %t %t %t".format(name, dtype2str(tab.getBorrowed(name).dtype),
s = "%s %s %t %t %t %t".format(name, dtype2str(tab.getBorrowed(name).dtype),
tab.getBorrowed(name).size, tab.getBorrowed(name).ndim,
tab.getBorrowed(name).shape, tab.getBorrowed(name).itemsize);
}
else {
throw getErrorWithContext(
msg=unknownSymbolError("attrib",name),
lineNumber=getLineNumber(),
routineName=getRoutineName(),
moduleName=getModuleName(),
errorClass="UnknownSymbolError");
}
return s;
}

Expand All @@ -443,7 +379,7 @@ module MultiTypeSymbolTable
:returns: s (string) containing the array data
*/
proc datastr(name: string, thresh:int): string throws {
check(name, "datastr");
checkTable(name, "datastr");
var u: borrowed GenSymEntry = tab.getBorrowed(name);
if (u.dtype == DType.UNDEF || u.dtype == DType.UInt8) {
var s = unrecognizedTypeError("datastr",dtype2str(u.dtype));
Expand All @@ -469,7 +405,7 @@ module MultiTypeSymbolTable
:returns: s (string) containing the array data
*/
proc datarepr(name: string, thresh:int): string throws {
check(name, "datarepr");
checkTable(name, "datarepr");
var u: borrowed GenSymEntry = tab.getBorrowed(name);
if (u.dtype == DType.UNDEF || u.dtype == DType.UInt8) {
var s = unrecognizedTypeError("datarepr",dtype2str(u.dtype));
Expand Down
52 changes: 26 additions & 26 deletions src/SegmentedMsg.chpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ module SegmentedMsg {
var (objtype, segName, valName) = payload.splitMsgToTuple(3);

// check to make sure symbols defined
st.check(segName);
st.check(valName);
st.checkTable(segName);
st.checkTable(valName);

var rname = st.nextName();
smLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
Expand Down Expand Up @@ -102,8 +102,8 @@ module SegmentedMsg {
payload.splitMsgToTuple(6);

// check to make sure symbols defined
st.check(segName);
st.check(valName);
st.checkTable(segName);
st.checkTable(valName);

var json = jsonToPdArray(valStr, 1);
var val = json[json.domain.low];
Expand Down Expand Up @@ -158,8 +158,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
idStr, kpStr, lStr, jsonStr) = payload.splitMsgToTuple(10);

// check to make sure symbols defined
st.check(segName);
st.check(valName);
st.checkTable(segName);
st.checkTable(valName);

smLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),
"cmd: %s subcmd: %s objtype: %t valtype: %t".format(
Expand Down Expand Up @@ -266,8 +266,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
var (objtype, segName, valName) = payload.splitMsgToTuple(3);

// check to make sure symbols defined
st.check(segName);
st.check(valName);
st.checkTable(segName);
st.checkTable(valName);

select objtype {
when "str" {
Expand Down Expand Up @@ -349,8 +349,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
var pn = Reflection.getRoutineName();

// check to make sure symbols defined
st.check(args[1]);
st.check(args[2]);
st.checkTable(args[1]);
st.checkTable(args[2]);

select objtype {
when "str" {
Expand Down Expand Up @@ -390,8 +390,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
var pn = Reflection.getRoutineName();

// check to make sure symbols defined
st.check(args[1]);
st.check(args[2]);
st.checkTable(args[1]);
st.checkTable(args[2]);

select objtype {
when "str" {
Expand Down Expand Up @@ -450,8 +450,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
var pn = Reflection.getRoutineName();

// check to make sure symbols defined
st.check(args[1]);
st.check(args[2]);
st.checkTable(args[1]);
st.checkTable(args[2]);

var newSegName = st.nextName();
var newValName = st.nextName();
Expand Down Expand Up @@ -518,10 +518,10 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
= payload.splitMsgToTuple(9);

// check to make sure symbols defined
st.check(lsegName);
st.check(lvalName);
st.check(rsegName);
st.check(rvalName);
st.checkTable(lsegName);
st.checkTable(lvalName);
st.checkTable(rsegName);
st.checkTable(rvalName);

select (ltype, rtype) {
when ("str", "str") {
Expand Down Expand Up @@ -583,8 +583,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
= payload.splitMsgToTuple(6);

// check to make sure symbols defined
st.check(segName);
st.check(valName);
st.checkTable(segName);
st.checkTable(valName);

var json = jsonToPdArray(encodedVal, 1);
var value = json[json.domain.low];
Expand Down Expand Up @@ -628,10 +628,10 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
testValName, invertStr) = payload.splitMsgToTuple(7);

// check to make sure symbols defined
st.check(mainSegName);
st.check(mainValName);
st.check(testSegName);
st.check(testValName);
st.checkTable(mainSegName);
st.checkTable(mainValName);
st.checkTable(testSegName);
st.checkTable(testValName);

var invert: bool;
if invertStr == "True" {invert = true;
Expand Down Expand Up @@ -672,8 +672,8 @@ proc segmentedPeelMsg(cmd: string, payload: string, st: borrowed SymTab): MsgTup
var (objtype, segName, valName) = payload.splitMsgToTuple(3);

// check to make sure symbols defined
st.check(segName);
st.check(valName);
st.checkTable(segName);
st.checkTable(valName);

var rname = st.nextName();
select (objtype) {
Expand Down