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

Remove register keyword according to ISO C++17 for fixing related warning #1893

Merged
merged 8 commits into from
May 22, 2024

Conversation

ia
Copy link
Collaborator

@ia ia commented Mar 16, 2024

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?
    Fix warning from compiler.

  • What is the current behavior?
    Pinecil build generates a lot of messages with the same type of warning.

  • What is the new behavior (if this is a feature change)?
    register keyword is removed from source/Core/BSP/Pinecil/Vendor/NMSIS/Core/Include/core_feature_base.h.

  • Other information:
    Not an C++ expert but as far as I understand, here is the situation:

  • ISO C++17 standard is used while compiling Pinecil build;
  • the standard tells that register keyword has no any effect (my guess is that now it's up to compiler's decision whether or not to save the variable value in CPU register);
  • some variables declared with register keyword in core_feature_base.h;
  • plus, there is -Wregister flag for the build;
  • hence, the build output log is overwhelmed with a lot of warnings like:
core_feature_base.h: warning: ISO C++17 does not allow 'register' storage class specifier [-Wregister]
|   register int32_t result;

I didn't cross the checklist though since I haven't tried the build on the hardware to make sure that it doesn't break anything on the device but since the keyword has no meaning I guess it should be ok.

@ia ia self-assigned this Mar 23, 2024
@ia ia requested a review from Ralim March 23, 2024 09:52
@ia
Copy link
Collaborator Author

ia commented May 10, 2024

Dear @Ralim . I understand that you are very busy these days so sorry to bother you. But once you will have a bit of some free time, could you, please, briefly look through this little PR and tell me what you think? Much appreciated for your time & experience as always, thanks.

Dear @discip , could you, please, very briefly check & test this build on Pinecil V1 (if you have any) just to make sure that there is no any undefined behavior with this little change? As always, much appreciated for your QA help.

Thank you, guys.

@Ralim
Copy link
Owner

Ralim commented May 14, 2024

I have no issue with the PR; I'll test on hardware this week (nag me if I dont get back to you)

@discip
Copy link
Collaborator

discip commented May 14, 2024

@ia
Sorry for the late answer.
I was able to briefly test this on the Pinecil-2 & on the S60P but unfortunately not yet on the TS80P.

So far nothing seems to be broken.

Is there anything particular to look out for?

@ia
Copy link
Collaborator Author

ia commented May 14, 2024

@ia Sorry for the late answer.

No-no, don't even worry.

I was able to briefly test this on the Pinecil-2 & on the S60P

Thanks!

but unfortunately not yet on the TS80P.

Yeah, sorry about that... I saw your comments from another discussion: it seems that yours is half bricked now. I'm sincerely really sorry to hear that :(
I wish I could help you but I don't have any experience with unbricking/reflashing TS80P through SWD myself yet.

So far nothing seems to be broken.

Really appreciate your help!

Is there anything particular to look out for?

Well, this particular change goes for Pinecil V1 only so if you can check just basic OK behavior on Pinecil V1 then it would be awesome. If not - that's fine, don't worry.

@discip
Copy link
Collaborator

discip commented May 15, 2024

@ia

Yeah, sorry about that...

Thank you! 👍

Well, this particular change goes for Pinecil V1 only so if you can check just basic OK behavior on Pinecil V1 then it would be awesome. If not - that's fine, don't worry.

I'll let you know as soon as I get my hands on it.

@Ralim Ralim merged commit f112908 into Ralim:dev May 22, 2024
17 checks passed
@Ralim
Copy link
Owner

Ralim commented May 22, 2024

Tested on two units here and works perfectly ❤️
Thank you for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants