-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Memory64] Replace int (used as ptr) with long in tests #14613
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ void *ThreadMain(void *arg) { | |
|
|
||
| pthread_t thread[NUM_THREADS]; | ||
|
|
||
| void CreateThread(int i) | ||
| void CreateThread(long i) | ||
| { | ||
| int rc = pthread_create(&thread[i], nullptr, ThreadMain, (void*)i); | ||
| assert(rc == 0); | ||
|
|
@@ -48,7 +48,7 @@ void mainn() { | |
|
|
||
| int main() { | ||
| // Create initial threads. | ||
| for(int i = 0; i < NUM_THREADS; ++i) { | ||
| for(long i = 0; i < NUM_THREADS; ++i) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can leave this as
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just trying to make the code work with as few casts as possible. Since the thread id is stored as a long/pointer, it seemed nicer to make this loop use |
||
| CreateThread(i); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,9 @@ int main(int argc, char **argv) { | |
| char *pc, *pc2; | ||
| assert(argc == 1); | ||
| pc = (char *)alloca(4+argc); | ||
| assert(((int)pc) % 4 == 0); | ||
| assert(((long)pc) % 4 == 0); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this case needed? Can't it just be removed?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was just trying to fix the existing test for 64-bit. It probably shouldn't be removed since this also still runs for 32-bit. |
||
| pc2 = (char *)alloca(4+argc); | ||
| assert(((int)pc2) % 4 == 0); | ||
| printf("z:%d*%d*%d*\n", (int)pc > 0, (int)pc, (int)pc2); | ||
| assert(((long)pc2) % 4 == 0); | ||
| printf("z:%d*%p*%p*\n", (long)pc > 0, pc, pc2); | ||
| return 0; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| #include <string.h> | ||
| #include <stdint.h> | ||
|
|
||
| int main() { | ||
| return (int)memchr("hello", 'z', 7); | ||
| return (int)(intptr_t)memchr("hello", 'z', 7); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be
%pwith no cast, no? Same wherever we want to printf a pointer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that be better.. I was just trying to fix the existing style which seems to like to cast to
int.411 results for
%ld(most of which "pointers", only 115 of which from PR) - might want to leave this improvement for another PR though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe lets land that first? That would make this PR a lot smaller. I can have a go at it if you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that produce a ton of conflicts with this PR?
I don't think we should mix trying to make things 64-bit compatible with general large scale cleanup of tests. I am sure if you go look at the tests you can find a lot of things to clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its good to do such cleanups when it becomes apparent that they are useful/needed. In this case doing the cleanup ahead of time will significantly reduce the size of, and churn generated by, this PR.
Yes it will conflict with this PR but it should be trivial to just run
git checkout --thiersto accept the upstream changes since we know we want to basically revert all the conflicting parts of this PR, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i.e. there should be no manual conflict resolution needed I think)