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

BASE: Convert version strings to const objects. #3268

Merged
merged 1 commit into from Nov 1, 2021
Merged

BASE: Convert version strings to const objects. #3268

merged 1 commit into from Nov 1, 2021

Conversation

@carlo-bramini
Copy link
Contributor

@carlo-bramini carlo-bramini commented Aug 12, 2021

The following variables:

gScummVMVersion
gScummVMBuildDate
gScummVMVersionDate
gScummVMFullVersion
gScummVMFeatures

are declared as pointers to strings but there is no need to do so.
They can be simply declared as const strings, since they will be never modified when SCUMMVM runs.

@bluegr
Copy link
Member

@bluegr bluegr commented Aug 12, 2021

I fail to see the benefit of this change... why is the proposed code better?

Loading

@carlo-bramini
Copy link
Contributor Author

@carlo-bramini carlo-bramini commented Aug 13, 2021

I fail to see the benefit of this change... why is the proposed code better?

The difference is that in the first case you have a string and a pointer, while in the second case you have just the string.
For example, in this code:
const char *my_string = "foo";
the my_string identifier is a pointer that points to a location that stores the string "foo".
Instead:
const char my_string[] = "foo";
the my_string identifier is directly the string "foo".
In the first case, keep in mind that the strings are not const objects actually.
If you wanted to do so, the expression needs to be changed to:
const char * const my_string = "foo";
but in my opinion there is no need to keep also a pointer for the purpose that those strings have.
Of course, this is a minor fix, I just noticed this for pure case since I have seen that the strings were not into the .rodata section.

Loading

@sev-
Copy link
Member

@sev- sev- commented Sep 2, 2021

This is kind of an unusual change, but for the aesthetical purity, it is OK... However, the commit log messages are not in conformance with our commit guidelines (https://wiki.scummvm.org/index.php?title=Commit_Guidelines). Could you please add the prefix "BASE: " in front of the commits? And also, please squash the commits into one, since one of them is not compilable.

Loading

The following variables:

gScummVMVersion
gScummVMBuildDate
gScummVMVersionDate
gScummVMFullVersion
gScummVMFeatures

are declared as pointers to strings but there is no need to do so.
They can be simply declared as const strings, since they will be never modified when SCUMMVM runs.

Exports version variables as const strings
@carlo-bramini carlo-bramini changed the title Convert version strings to const objects. BASE: Convert version strings to const objects. Sep 2, 2021
@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

Thanks, merging

Loading

@sev- sev- merged commit 6c76906 into scummvm:master Nov 1, 2021
7 checks passed
Loading
@carlo-bramini carlo-bramini deleted the fix-const-1 branch Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants