Skip to content

Enable extra warnings (-Wextra) #384

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Feb 17, 2017
Merged

Conversation

rigtorp
Copy link
Contributor

@rigtorp rigtorp commented Sep 1, 2016

Enable extra warnings and fix those warnings.

@rigtorp
Copy link
Contributor Author

rigtorp commented Sep 20, 2016

@tmontgomery What do you think about this patch?

@@ -4,4 +4,5 @@
"http://www.puppycrawl.com/dtds/suppressions_1_0.dtd">
<suppressions>
<suppress files=".*generated.*" checks="."/>
<suppress files="CppGenerator.java" checks="MethodLength"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not something we want to suppress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to split up the constructor codegen function for no good reason. Should I do something like this:

void generateConstructor() {
  generatePart1();
  generatePart2();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is best to break up large methods into logical blocks so they can be reasoned about.

@ksergey
Copy link
Contributor

ksergey commented Dec 5, 2016

a looking forward 80c53b6 in next release

@rigtorp
Copy link
Contributor Author

rigtorp commented Jan 16, 2017

@tmontgomery @mjpt777 I've updated this PR to ignore -Wtype-limits in wrapForEncode instead of the more complicated change i previously had.

This PR is a superset of #421

@rigtorp
Copy link
Contributor Author

rigtorp commented Feb 11, 2017

@tmontgomery @mjpt777 Any thoughts on this?

@tmontgomery tmontgomery merged commit 569d5f6 into aeron-io:master Feb 17, 2017
@tmontgomery
Copy link
Contributor

Sorry this took so long. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants