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

[onert/frontend] Revisit traininfo metadata name #12410

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Jan 5, 2024

This PR adds 'const' keyword to the traininfo metadata name.
It also adds 'extern' keyword to follow one-definition-rule.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn sseung.youn@samsung.com

related issue : #11692

@zetwhite zetwhite added the PR/ready for review It is ready to review. Please review it. label Jan 5, 2024
@zetwhite zetwhite requested review from a team and removed request for a team January 5, 2024 08:12
@zetwhite zetwhite added PR/NO MERGE Please don't merge. I'm still working on this :) and removed PR/ready for review It is ready to review. Please review it. labels Jan 5, 2024
@zetwhite zetwhite changed the title [onert/frontend] Add const to traininfo metadata name [onert/frontend] Revisit traininfo metadata name Jan 5, 2024
Comment on lines 30 to 31
static constexpr char *const TRAININFO_METADATA_NAME = "CIRCLE_TRAINING";
extern const char *const TRAININFO_METADATA_NAME;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I was on the next step (next PR), I found an error :

ONE/runtime/onert/frontend/circle_traininfo/include/traininfo_loader.h:30:56: 
error: ISO C++ forbids converting a string constant to ‘char*’ [-Werror=write-strings]
   30 | static constexpr char *const TRAININFO_METADATA_NAME = "CIRCLE_TRAINING";
      |                                                        ^~~~~~~~~~~~~~~~~

It looks like constexpr char* doesn't mean that char-array(string) content is constant.
So, For clearness, Let's just remove constexpr and use const instead.


I refered stack-overflow https://stackoverflow.com/questions/30561104/const-constexpr-char-vs-constexpr-char.
I tried to find an official C++ document about this, But it's hard to find a clear explanation about this.

Copy link
Contributor

@ragmani ragmani Jan 5, 2024

Choose a reason for hiding this comment

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

Could you add a comment like below?

// TODO Change to inline variables since C++17

Copy link
Contributor Author

@zetwhite zetwhite Jan 5, 2024

Choose a reason for hiding this comment

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

updated!

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@zetwhite zetwhite added PR/ready for review It is ready to review. Please review it. approval: 2 Require at least 2 approvals and removed PR/NO MERGE Please don't merge. I'm still working on this :) labels Jan 5, 2024
@zetwhite zetwhite requested review from a team and ragmani January 5, 2024 09:50
This PR adds 'const' keyword to the traininfo metadata name.
It also adds 'extern' keyword to follow one-definition-rule.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn <sseung.youn@samsung.com>
Copy link
Contributor

@YongseopKim YongseopKim left a comment

Choose a reason for hiding this comment

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

LGTM and I have a question.

by zetwhite

This PR adds 'const' keyword to the traininfo metadata name.
It also adds 'extern' keyword to follow one-definition-rule.

Where can I find the reference to one-definition-rule?

@zetwhite
Copy link
Contributor Author

zetwhite commented Jan 8, 2024

Where can I find the reference to one-definition-rule?

@YongseopKim
Ah, I didn't put the reference about one-definition-rule.
I put things below :

  • cpp-reference
    Here, You could find an official document about one-definition-rule

  • best way to declare and define global variable : link-from-stackoveflow
    Since the official document is a bit broad, I recommend reading this note (I found this on StackOverflow)
    This one is not highly reliable as an official document, but easy to catch this PR's changes.

Copy link
Contributor

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

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

LGTM

@hseok-oh hseok-oh merged commit c925d8a into Samsung:master Jan 8, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants