Skip to content

Insert itypes correctly for constant sized arrays #702

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

Merged
merged 16 commits into from
Sep 10, 2021

Conversation

john-h-kastner
Copy link
Collaborator

This fixes a rewriting error observed in libjpeg.

Given this declaration converted with 3c -itypes-for-extern -alltypes

int foo[10];

The output incorrectly moved the length before the identifier.

int [10] foo : itype(int _Checked[10]);

A similar issue can be observed without -itypes-for-extern

void test(int a[10]) {
  a = 1;
}

Which converted to a valid declaration, but the array type declaration was transformed into a pointer.

void test(int *a : itype(int _Checked[10])) {
  a = 1;
}

These examples now correctly rewrite to itypes int foo[10] : itype(int _Checked[10]).

@aaronjeline aaronjeline self-requested a review September 8, 2021 19:06
Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

I'd be curious if the other mkString change in #657 interacts in some way with what you are trying to achieve here and might indicate that a different approach might be preferable for this PR. Maybe one of us can look into that later. I'm now satisfied that this isn't the case. I thought the int [10] foo : itype(int _Checked[10]) problem might be related to the change I made to processing of arrays in mkString, but it sounds like that problem actually arises from the order of operations in DeclRewriter::buildItypeDecl.

@mattmccutchen-cci mattmccutchen-cci changed the title Insert itypes correcly for constant sized arrays Insert itypes correctly for constant sized arrays Sep 8, 2021
Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

"Request changes" just to make sure you consider my (not fully investigated) idea about getRewritableOriginalTy. We can dismiss my review if we determine that idea won't work.

Copy link
Collaborator

@aaronjeline aaronjeline left a comment

Choose a reason for hiding this comment

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

Looks good to me. mkString remains a complicated function but we don't know the way around that and that's not the point of this pr

Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

Thanks for figuring out the qtyToStr thing! I'm glad that there was a functionality benefit (as well as a design benefit) to offset the extra work. It's probably possible to clean up more of the getRewritableOriginalTy-related code now, but that's outside the scope of this PR; I may file a follow-up issue later.

The parts of this PR that I understand (:slightly_smiling_face:) look good to me, modulo the following comments. But I assume Aaron has reviewed the other parts.

Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

Two more nits. Thanks for your patience.

john-h-kastner and others added 3 commits September 10, 2021 14:42
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
@john-h-kastner john-h-kastner merged commit 5f70035 into main Sep 10, 2021
mattmccutchen-cci added a commit that referenced this pull request Sep 28, 2021
I'm including this because the code fix was similar to a fix that was
originally in the multi-decl overhaul before the same fix was needed in
#702 and I was
aware of the issue from item 4 of
#703.
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