-
Notifications
You must be signed in to change notification settings - Fork 52
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
[generator] Separate metadata fixup step from parsing step. #822
Conversation
{ | ||
var value = element.XGetAttribute (name); | ||
|
||
if (int.TryParse (value, out var result)) |
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.
One possible concern here is that this uses the current culture info to parse value
: https://docs.microsoft.com/en-us/dotnet/api/system.int32.tryparse?view=net-5.0#System_Int32_TryParse_System_String_System_Int32__
The
s
parameter is parsed using the formatting information in a NumberFormatInfo object initialized for the current system culture.
We may want to instead use:
int.TryParse (value, NumberStyles.Integer, CurrentCulture.InvariantCulture, out var result)
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.
Alternate idea: why don't we use the explicit conversion from XAttribute
to int?
operator?
public static int? XGetInt32Attribute (this XElement element, string name) => (int?) element.Attribute (name);
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.
This seems to throw an exception on invalid input:
System.FormatException
HResult=0x80131537
Message=Input string was not in a correct format.
Source=System.Private.CoreLib
StackTrace:
at System.Number.ThrowOverflowOrFormatException(ParsingStatus status, TypeCode type)
at System.Number.ParseInt32(ReadOnlySpan`1 value, NumberStyles styles, NumberFormatInfo info)
at System.Int32.Parse(String s, NumberStyles style, IFormatProvider provider)
at System.Xml.XmlConvert.ToInt32(String s)
at System.Xml.Linq.XAttribute.op_Explicit(XAttribute attribute)
// BG4302 | ||
Report.LogCodedError (Report.ErrorAddNodeInvalidXPath, e, metaitem, path); | ||
} | ||
break; |
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.
Indentation.
gens.AddRange (ParsePackage (elem, options)); | ||
break; | ||
case "enum": | ||
var sym = new EnumSymbol (elem.XGetAttribute ("name")); |
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.
I wonder if we should capture elem.XGetAttribute("name")
into a name
variable, so that we don't call it twice.
Context: #789
One issue with the proposal to flip the order of
ApiXmlAdjuster
andmetadata
in #789 is that the "apply metadata" step is intertwined with the "parse API xml document into POCO's" step. This means that the adjuster step cannot be inserted between them as the code is currently written.This PR separates the two steps so that they can be reordered in the future.
Additionally it refactors the metadata fixup step so that it can be unit tested easier by moving it to
Java.Interop.Tools.Generator
. Unit tests for applying metadata have been added.