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

[NFC] Explicitly set dialects to usePropertiesForAttributes=0 in preparation for https://reviews.llvm.org/D158581 (flipping the default to 1) to land. #124

Closed
wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link
Contributor

@copybara-service copybara-service bot commented Aug 23, 2023

[NFC] Explicitly set dialects to usePropertiesForAttributes=0 in preparation for https://reviews.llvm.org/D158581 (flipping the default to 1) to land.

This allows us to switch dialects to use properties one by one.

@copybara-service copybara-service bot changed the title [NFC] Prepare for https://reviews.llvm.org/D158581 (switching the default for usePropertiesForAttributes). [NFC] Explicitly set dialects to usePropertiesForAttributes=0 in preparation for https://reviews.llvm.org/D158581 (flipping the default to 1) to land. Aug 23, 2023
@copybara-service copybara-service bot force-pushed the test_559375240 branch 3 times, most recently from 964f63e to bb2b735 Compare August 24, 2023 12:36
@copybara-service copybara-service bot closed this Aug 24, 2023
@copybara-service copybara-service bot deleted the test_559375240 branch August 24, 2023 14:47
@AlexanderViand-Intel
Copy link
Collaborator

AlexanderViand-Intel commented Aug 29, 2023

Since this came up in #123

Also see the very timely PR #124 which seems to opt us out of a migration to use properties as the default. If you're interested in digging into how to use properties instead of attributes, feel free to flip that flag back after it lands.

From my understanding "inherent attributes" (i.e., attributes that are part of the OP's definition and not something tagged on as part of an analysis/transformation) should really switch over to properties (for various reasons described in the properties RFC) so imho we should either not merge this, or merge this but then flip the flag back as soon as we can.

EDIT: I'm actually somewhat confused by what's going on with this PR, the copybara bot seems to be going a bit mad here?

@asraa
Copy link
Collaborator

asraa commented Aug 29, 2023

or merge this but then flip the flag back as soon as we can.

I think so too. I'm a little fuzzy still on the cases for when not to use a property, so I might need some help understanding the tradeoffs from the RFC. I can file an issue to flip the flag back and perform some op attribute migrations.

the copybara bot seems to be going a bit mad here?

It's really weird but copybara bot, after a PR is approved (by one of us internally), closes the PR and merges the commit to the main branch directly (9ec8628)

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

2 participants