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

Reduce the amount of CPP for attribute grammar support #257

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

Ericson2314
Copy link
Collaborator

We previously had a lot of #ifdef HAPPY_BOOTSTRAP to support mangling with and without attribute grammar support. Now that module is broken up so that we need less.

This hopefully makes the code easier to understand and maintain.

We previously had a lot of `#ifdef HAPPY_BOOTSTRAP` to support mangling
with and without attribute grammar support. Now that module is broken up
so that we need less.

This hopefully makes the code easier to understand and maintain.
description: Optimize the implementation of happy using a pre-built happy
description:
Optimize the implementation of happy using a pre-built happy,
and add support for attribute grammars.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should mention the other behavioral thing this flag does.

Copy link
Member

Choose a reason for hiding this comment

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

I would have the flag the other way round, so it would then be "Build a version of happy for boot-strapping. The generated binary will not have support for attribute grammars."
See #256 (comment).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's going to be confusing either way because bootstrap can be interpreted either as "bootstrapped" or "for bootstrapping", which are opposite in meaning.

Any ideas for a less ambiguous name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well that is a preexisting issue; I am happy to change it but seems like it should be done separately. Does all the new things from this PR look good?

@@ -63,7 +65,7 @@ library
other-modules:
Happy.Frontend.ParseMonad
Happy.Frontend.ParseMonad.Class
Happy.Frontend.AttrGrammar
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was actually already conditionally used, and should thus go below.

@Ericson2314 Ericson2314 merged commit c194d8c into master Oct 2, 2023
31 checks passed
@Ericson2314 Ericson2314 deleted the less-cpp-for-attr-gammar branch October 2, 2023 16:14
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.

None yet

3 participants