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

Clean up AttributeUtils #1470

Merged
merged 6 commits into from
Nov 21, 2022
Merged

Clean up AttributeUtils #1470

merged 6 commits into from
Nov 21, 2022

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Nov 11, 2022

While trying to use the Attribute syntax I found that the AttributeUtils could use some cleanups. In particular, this change removes the grammar rule AttrParmValue and instead simply uses Literal. Since some of the type checking done required new functions for checking literals, I also decided to move all functions regarding expressions to a new class ExpressionUtils. And while doing so I also realized that AstUtils, ExpressionUtils, and AttributeUtils woulb probably better located in the org.lflang.ast subpackage. I realize that this last change might be causing conflicts with other large outstanding PRs. Feel free to revert the last commit if it seems too intrusive.

@cmnrd cmnrd added the enhancement Enhancement of existing feature label Nov 11, 2022
@cmnrd cmnrd force-pushed the cleanup-attributes branch from b6b619d to 88125f7 Compare November 11, 2022 15:55
@oowekyala
Copy link
Collaborator

I understand where this is coming from, but maybe we should wait to move these classes. The AST nodes themselves don't live in the ast package -maybe they should be moved there as well. I'm partly saying this because #1441 is still in flight as you know, and I want it to avoid the fate of #544.

I suggest we should have a ticket about planned package restructurings, and do everything in bulk as a planned effort when that achieves a critical mass.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Nov 11, 2022

OK, we could add it as an item to #1276.

It looks like the tests do not pass anyway, I will probably only get to fix them end of next week.

@lhstrh
Copy link
Member

lhstrh commented Nov 11, 2022

This is cool, but I'd prefer to defer merging this into master until fed-gen is merged. We're getting pummeled with conflicts otherwise, making it even harder to get it ready to merge.

@cmnrd cmnrd force-pushed the cleanup-attributes branch from f84644d to 0042b99 Compare November 17, 2022 13:33
Co-authored-by: Clément Fournier <[email protected]>
@cmnrd
Copy link
Collaborator Author

cmnrd commented Nov 17, 2022

I force pushed a more minimalist version today, that only cleans up the attributes but avoids additional refactoring.

@cmnrd cmnrd merged commit 2eb3687 into master Nov 21, 2022
@cmnrd cmnrd deleted the cleanup-attributes branch November 21, 2022 07:36
@lhstrh lhstrh added refactoring Code quality enhancement and removed enhancement Enhancement of existing feature labels Jan 26, 2023
@petervdonovan petervdonovan changed the title Cleanup AttributeUtils Clean up AttributeUtils Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants