Skip to content
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

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Apr 13, 2021

Context: #789

One issue with the proposal to flip the order of ApiXmlAdjuster and metadata 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.

@jpobst jpobst marked this pull request as ready for review April 15, 2021 19:47
{
var value = element.XGetAttribute (name);

if (int.TryParse (value, out var result))
Copy link
Member

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)

Copy link
Member

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);

Copy link
Contributor Author

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;
Copy link
Member

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"));
Copy link
Member

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.

@jonpryor jonpryor merged commit f4e68b5 into main Apr 22, 2021
@jonpryor jonpryor deleted the rework-fixups branch April 22, 2021 21:26
@jpobst jpobst added this to the 11.3 (16.10 / 8.10) milestone May 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants