Skip to content

Take the latest tool-base and bring a handful of improvements#316

Merged
yevhenii-nadtochii merged 11 commits intomasterfrom
some-improvements
Jan 21, 2025
Merged

Take the latest tool-base and bring a handful of improvements#316
yevhenii-nadtochii merged 11 commits intomasterfrom
some-improvements

Conversation

@yevhenii-nadtochii
Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii commented Jan 16, 2025

This PR does the following:

  1. Takes the latest tool-base to pick up tool-base #111.
  2. Makes the whole Expression hierarchy covariant. Previously, only some child members declared it.
  3. FieldDeclaration is actually Statement.

Remove nullability of Member properties

I'm not 100% sure about this change, but personally don't foresee any problems.

I suggest moving null safety checks from the usage site to the declaration site of Member.context and typeSystem properties. Making them non-nullable is desired for the following reasons:

  1. I have already met the third JavaRenderer that has such a line public fun typeSystem(): TypeSystem = typeSystem!!.
  2. !! leads to a naked NPE while ISE from the declaration site would bring an error message.
  3. Anyway, each line of code within renderers (and, probably, members) is meant to be executed during the codegen.
  4. We have select() method in Member, and it just throws when used without the injected codegen context, but could have returned null as properties do.

@yevhenii-nadtochii yevhenii-nadtochii self-assigned this Jan 16, 2025
@codecov
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.57%. Comparing base (44912fa) to head (47e5f06).
Report is 12 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #316      +/-   ##
============================================
+ Coverage     76.46%   76.57%   +0.10%     
- Complexity      550      551       +1     
============================================
  Files           186      186              
  Lines          4105     4107       +2     
  Branches        380      378       -2     
============================================
+ Hits           3139     3145       +6     
+ Misses          835      834       -1     
+ Partials        131      128       -3     

@yevhenii-nadtochii yevhenii-nadtochii marked this pull request as ready for review January 16, 2025 14:42
@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

@yevhenii-nadtochii yevhenii-nadtochii changed the title Take the latest tool-base and bring a handful of small improvements Take the latest tool-base and bring a handful of improvements Jan 16, 2025
@armiol
Copy link
Collaborator

armiol commented Jan 16, 2025

@yevhenii-nadtochii The reason for returning 255 and not 1 is exactly the commonality of the latter. We want to distinguish the case in which we made a deliberate choice to stop the machine and the cases in which some third-party code (or JVM itself) has exited due to their particular reasons.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@yevhenii-nadtochii LGTM except for the exit code change.

* Obtains a [FieldAccess] to the [field] of this message.
*/
public fun Expression<out Message>.field(field: Field): FieldAccess =
public fun Expression<Message>.field(field: Field): FieldAccess =
Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as it allows you to use this code in Validation, looks logical.

e.printStackTrace(this)
println("```")
}
exitProcess(-1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment on the PR level.

@yevhenii-nadtochii
Copy link
Contributor Author

@armiol PTAL

Copy link
Collaborator

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

LGTM

@yevhenii-nadtochii yevhenii-nadtochii merged commit 8b53e14 into master Jan 21, 2025
8 checks passed
@yevhenii-nadtochii yevhenii-nadtochii deleted the some-improvements branch January 21, 2025 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants