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: change hello_world cmake compiler use -std=c++17 #116

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

weedge
Copy link

@weedge weedge commented Mar 21, 2024

use libgemma util/app.h this function gcpp::LoaderArgs loader(argc, argv) , Want to pass const char** to argv

@@ -37,7 +37,7 @@ std::vector<int> tokenize(
return tokens;
}

int main(int argc, char** argv) {
int main(int argc, const char** argv) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we use const here? char** or char*[] is standard, see 3.6.1 in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf.

Copy link
Author

@weedge weedge Mar 21, 2024

Choose a reason for hiding this comment

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

argv from command params just read, so want use const char**, want use libgemma util/app.h this function gcpp::LoaderArgs loader(argc, argv) i want to cast const char** to char** , like this char** nonConstPtr = const_cast<char*>(constPtr);; but this command argv is immutable

Copy link
Author

Choose a reason for hiding this comment

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

Why do we use const here? char** or char*[] is standard, see 3.6.1 in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf.

use main(int argc, char** argv) it's ok~

@@ -38,7 +38,7 @@ std::vector<int> tokenize(
}

int main(int argc, char** argv) {
gcpp::LoaderArgs loader(argc, argv);
gcpp::LoaderArgs loader(argc, (const char**)argv);
Copy link
Member

Choose a reason for hiding this comment

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

I still do not understand how this change is helpful? :) Const is generally helpful, but for historical reasons we are stuck with non-const for args. It seems like casting is worse than the safety benefit.
We can keep the c++17 change, but let's revert the const or at least move to a separate PR to discuss further :)

Copy link
Author

Choose a reason for hiding this comment

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

yep~ just keep the c++17 change

@weedge weedge changed the title fix: change hello_world cmake compiler use -std=c++17 && argv add const fix: change hello_world cmake compiler use -std=c++17 Mar 22, 2024
@jan-wassenberg jan-wassenberg added the copybara-import Trigger Copybara for merging pull requests label Mar 22, 2024
@jan-wassenberg
Copy link
Member

We have an issue with the copybara import mechanism. Working on it.

@jan-wassenberg jan-wassenberg added copybara-import Trigger Copybara for merging pull requests and removed copybara-import Trigger Copybara for merging pull requests labels Mar 26, 2024
@jan-wassenberg
Copy link
Member

Hi @weedge, sorry this has taken so long. Would you mind rebasing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
copybara-import Trigger Copybara for merging pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants