Skip to content

Eliminated warnings in Hrs3300#206

Merged
JF002 merged 2 commits intoInfiniTimeOrg:developfrom
nscooling:Hrs3300_SetGain_remove_warnings
Feb 14, 2021
Merged

Eliminated warnings in Hrs3300#206
JF002 merged 2 commits intoInfiniTimeOrg:developfrom
nscooling:Hrs3300_SetGain_remove_warnings

Conversation

@nscooling
Copy link
Copy Markdown
Contributor

The .cpp file was emitting indentation warnings for WriteRegister as the while loop wasn't wrapped in a block.

Also removed the unnecessary static associated with the constexpr for

  constexpr uint8_t maxGain = 64U;

and added the U just for completeness.

@Avamander
Copy link
Copy Markdown
Collaborator

Avamander commented Feb 12, 2021

I'm not mandating anything, but did you use the clang-tidy configuration (in the develop branch by now) to detect this? Also, is it formatted using clang-format configuration? It's just that I want to know if they have been useful.

@nscooling
Copy link
Copy Markdown
Contributor Author

I'm building using docker and I don't think that's setup to use clang-tidy at the moment. I'm pretty sure these are just standard GCC warnings.

Clang-tidy is great, especially most of the modernise warnings. I haven't tried out the confit file you've setup yet. I'm just getting back into the swing of things after a couple of weeks not being able to contribute. Cppcheck is also useful.

@petterhs
Copy link
Copy Markdown
Contributor

There is already #182 suggesting some of these changes🙂

@nscooling
Copy link
Copy Markdown
Contributor Author

There is already #182 suggesting some of these changes🙂

Sorry I'd missed these. I'd suggest it's always better to do a PR per file change. I know it's a minor thing but it makes merging simpler. But great minds think alike 😉 or maybe we're both showing our OCD 🤔

@JF002 JF002 merged commit 3a1ccdb into InfiniTimeOrg:develop Feb 14, 2021
@nscooling nscooling deleted the Hrs3300_SetGain_remove_warnings branch March 6, 2021 17:31
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.

4 participants