JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
563 stars 118 forks source link

Replace recursive method calls with iterative processing. #362

Closed bryanboettcher closed 1 year ago

bryanboettcher commented 1 year ago

Addresses #361

jeremydmiller commented 1 year ago

@bryanboettcher You changed a lot more code than I was expecting. Why did you eliminate so much of the validation logic in there?

bryanboettcher commented 1 year ago

@bryanboettcher You changed a lot more code than I was expecting. Why did you eliminate so much of the validation logic in there?

The validation logic in each case previously would test "as/null", and if that check failed, would throw an InvalidCastException. The replacement code instead is using the As<T> extension method, which will itself throw an InvalidCastException. The same exception should be thrown just with an extra frame on the stack trace for the extension method.

In other instances, like ArrayAssignmentFrame, the Next tests & assignment were moved into the calling method to unwind the traversal.

I'm not the author of the changes, that's another member of our team at work who's now out on vacation -- but he should be back 11/14/22 and I can ask him any additional questions then.

jeremydmiller commented 1 year ago

It didn't just change how the casting logic was working, it changed actual functionality. Probably points at some missing tests too that this didn't fail the build.

bryanboettcher commented 1 year ago

Huh, alright. We'll take another look at it and update the PR. I'll convert to draft for now.

bryanboettcher commented 1 year ago

Marking this ready-for-review again. I believe the feedback is a misunderstanding. The chained calls to "Next" were all centralized inside of FuncResolverDefinition.