-
-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
Fix LT-21088: Yellow Box while parsing words after promoting category #188
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jtmaxwell3)
Src/LexText/ParserCore/M3ToXAmpleTransformer.cs
line 133 at r1 (raw file):
} private void GetUndefinedSlots(XElement element, ISet<string> undefinedSlots)
Could we do this without recursion?
Here's an alternative way of doing it that I believe may work. DescendantsAndSelf() gets a flat structure that can be simply iterated over. Unsure if the order is fine without recursion based on your last comment in this method.
Code snippet:
private void GetUndefinedSlots(XElement element, ISet<string> undefinedSlots)
{
// Iterate over all descendant elements in a flat structure.
foreach (XElement element in rootElement.DescendantsAndSelf())
{
// Record slots encountered.
if (element.Name == "PrefixSlots" || element.Name == "SuffixSlots")
{
undefinedSlots.Add((string)element.Attribute("dst"));
}
// Check and remove slots that are defined at this level.
if (element.Name == "AffixSlots")
{
foreach (XElement slot in element.Elements())
{
undefinedSlots.Remove((string)slot.Attribute("Id"));
}
}
}
}
I think that the flat version may get the wrong result because AffixSlots has to be processed after PrefixSlots and SuffixSlots. I would prefer to keep the recursive version because it is more obvious whether this constraint is preserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do two passes then? The recursive code feels less readable/easily understandable to me.
private void GetUndefinedSlots(XElement rootElement, ISet<string> undefinedSlots)
{
var allElements = rootElement.DescendantsAndSelf();
// First pass: Add slots encountered in PrefixSlots or SuffixSlots
foreach (XElement element in allElements)
{
if (element.Name == "PrefixSlots" || element.Name == "SuffixSlots")
{
undefinedSlots.Add((string)element.Attribute("dst"));
}
}
// Second pass: Remove any slots that are defined in AffixSlots elements
foreach (XElement element in allElements)
{
if (element.Name == "AffixSlots")
{
foreach (XElement slot in element.Elements())
{
undefinedSlots.Remove((string)slot.Attribute("Id"));
}
}
}
}
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jtmaxwell3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jtmaxwell3)
Src/LexText/ParserCore/M3ToXAmpleTransformer.cs
line 118 at r1 (raw file):
return; // Add undefined slots to AffixSlots. foreach (XElement elem in templateElem.Elements())
We can get rid of the outer foreach loop by grabbing the first element with "AffixSlots" as its name.
We could also get rid of the inner foreach loop with LINQ, although that change is arguably beneficial.
Code snippet:
XElement affixSlotsElem = templateElem.Element("AffixSlots");
if (affixSlotsElem == null)
return;
affixSlotsElem.Add(CreateAffixSlotElements(undefinedSlots));
// Separate helper method for readability
private IEnumerable<XElement> CreateAffixSlotElements(IEnumerable<string> slotIds)
{
return slotIds.Select(slotId => new XElement("MoInflAffixSlot", new XAttribute("Id", slotId)));
}
Two flat passes gets the wrong result. We are trying to determine whether there is a variable that is used but has no definition in scope. So we have to use recursion to compute scope. I don't understand your second suggestion, so I can't verify if it is correct. I find LINQ code hard to read. I don't see what is wrong with my original code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Just wanting to avoid nested foreach loops & an if statement if possible. For my second suggestion, we can leave out the LINQ and still remove the outer foreach loop.
XElement affixSlotsElem = templateElem.Element("AffixSlots");
if (affixSlotsElem == null)
return;
foreach (string slotId in undefinedSlots)
{
XElement slot = new XElement("MoInflAffixSlot");
slot.SetAttributeValue("Id", slotId);
affixSlotsElem.Add(slot);
}
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jtmaxwell3)
I removed the extra for loop and made the documentation clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jtmaxwell3)
I fixed LT-21088 by locally defining slots that are in the wrong scope. This keeps XAmple from crashing, but it may or may not parse templates that have slots in the wrong scope correctly. Andy thinks that the right solution is for users to define the scopes of the slots correctly. I'd like to report the problem to the user somehow but I don't see an easy way to do that from ParserCore.
This change is