IronOS icon indicating copy to clipboard operation
IronOS copied to clipboard

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

Open ia opened this issue 3 months ago • 5 comments

  • 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 avatar Mar 16 '24 08:03 ia

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.

ia avatar May 10 '24 09:05 ia

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

Ralim avatar May 14 '24 11:05 Ralim

@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?

discip avatar May 14 '24 19:05 discip

@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.

ia avatar May 14 '24 21:05 ia

@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.

discip avatar May 15 '24 07:05 discip