-
Notifications
You must be signed in to change notification settings - Fork 211
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
Avoid repeated global strings #1377
Conversation
Are these "global strings" guaranteed to be readonly? |
only when |
I missed that. I should reuse them when they are constant. Let me update the PR. |
dbd08f7
to
427ebea
Compare
Fixed! |
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.
How are we testing this?
The test file I added |
Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>
427ebea
to
7e82dae
Compare
|
||
|
||
constructor(address a, string memory _baseURI) | ||
{ |
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.
Does the contract need all those empty functions for this case?
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 can shorten the contract, but I left it as it is because it used to generate 1008 definitions of math overflow
.
What is your opinion in shortening it?
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.
Well, it's not the end of the world. I'll leave that up to you. I just thought that if in the end only 1 single math overflow
string instead of 1008 should be generated, the test code could be more to the point.
When debugging the issue #1367, I found out that we keep emitting repeated global strings. The LLVM IR for the contract pointed in such an issue had the string
math overflow,\0A
defined 1008 times.This PR DOES NOT solve #1367, but it is something that needed a fix.