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

Fix for issue #373 #408

Merged
merged 35 commits into from
Mar 8, 2021
Merged

Fix for issue #373 #408

merged 35 commits into from
Mar 8, 2021

Conversation

aaronjeline
Copy link
Collaborator

No description provided.

Comment on lines 661 to 665
for (auto It = Vars.begin(); It != Vars.end(); It++, Idx++) {
getQualString(Idx, S);
}

return S.str();
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this could generate i.e. "const const" if you have const int * const * a or something like that. So the function name is appropriate. But it seems to be used below as a single qualifier. Is that correct?

Copy link
Collaborator

@john-h-kastner john-h-kastner left a comment

Choose a reason for hiding this comment

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

Some code formatting/testing changes + one concern about a function.

@mattmccutchen-cci
Copy link
Member

@aaronjeline Let me know if you need help with the -base-dir constraints stuff. One thing I'll mention: my tests currently use typedefs as an example of a case that is not implemented, so now that you're implementing that case, you'll have to find a different example! 🙂 See here for some ideas or ask John.

typedef int * * const a;
//CHECK: typedef const _Ptr<_Ptr<int>> a;
void xxx(void) {
a b;
Copy link
Member

Choose a reason for hiding this comment

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

Since this rewrites const (or removes it), you can add a check here, maybe even with a comment on why it's ok to add/remove it.

@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Feb 8, 2021

I had an idea for a new test of the "3C generated changes to this file even though it is not allowed to write to the file" error. This code is simple but crazy enough that we may not have to update the test again for a while, if ever.

base_subdir/main.c:

void
#include "partial_defn.h"

partial_defn.h:

foo(int *x) {}

Then 3c -extra-arg=-I${PWD%/*} main.c -- in base_subdir trips the error nicely. 😁

Update: Since I have to make other changes to clang/test/3C/base_subdir/canwrite_constraints_typedef.c in #412 anyway, I'll go ahead and make the above change in #412. (Second update: Done.) That means if #412 goes in before this PR, this PR will not affect the "designed to fail" base dir test and you'll only have to add a typedef to the "designed to succeed" test. If this PR goes in before #412, you'll be able to copy the change to the "designed to fail" test from the draft of #412 into this PR.

mattmccutchen-cci added a commit that referenced this pull request Feb 8, 2021
Also change the unimplemented canWrite constraints case from a typedef
to a crazy #include in anticipation of #408.
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'm reviewing only the canWrite constraints portion of this PR. There are a few things I'd like to see added.

Copy link
Member

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

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

Nothing to add to open issues in the comments. Seems OK on quick glance, beyond this.

Aaron Eline and others added 2 commits February 19, 2021 12:04
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>
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.

The root-cause and canWrite stuff looks good to me now; thanks!

Copy link
Collaborator

@john-h-kastner john-h-kastner left a comment

Choose a reason for hiding this comment

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

I think all the substantial work is done for this PR. There are a few formatting issue I commented on, but looks good otherwise.

@aaronjeline aaronjeline requested a review from mwhicks1 March 5, 2021 19:31
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 think the parts I know about are OK modulo this comment.

Copy link
Collaborator

@john-h-kastner john-h-kastner left a comment

Choose a reason for hiding this comment

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

I think this is good to go. It should fix at least one of the compilation errors for Lua.

@mwhicks1
Copy link
Member

mwhicks1 commented Mar 8, 2021

Fantastic! Let's get it in there. :)

@john-h-kastner john-h-kastner merged commit 906138e into main Mar 8, 2021
@aaronjeline aaronjeline deleted the typedefs branch March 9, 2021 05:19
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.

5 participants