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

Cleans up ICC options, and some build issues #1451

Merged
merged 1 commit into from
Feb 16, 2017
Merged

Conversation

zwoop
Copy link
Contributor

@zwoop zwoop commented Feb 15, 2017

This removes a number of the previous warning exclusions that we
we used to do. We then also have to clean up the core code in a
few places, sometimes looking slightly odd (e.g. the meaningless
const that we have in a few places).

@zwoop zwoop added Build work related to build configuration or environment Cleanup labels Feb 15, 2017
@zwoop zwoop added this to the 7.2.0 milestone Feb 15, 2017
@zwoop zwoop self-assigned this Feb 15, 2017
@atsci
Copy link

atsci commented Feb 15, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1449/ for details.

@atsci
Copy link

atsci commented Feb 15, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1555/ for details.

@atsci
Copy link

atsci commented Feb 15, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/120/ for details.

@atsci
Copy link

atsci commented Feb 15, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1451/ for details.

@atsci
Copy link

atsci commented Feb 15, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1558/ for details.

@atsci
Copy link

atsci commented Feb 15, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/123/ for details.

Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

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

👍

@@ -694,6 +695,7 @@ TS_INLINE char &IOBufferReader::operator[](int64_t i)
}

ink_release_assert(!"out of range");
return default_ret[0];
Copy link
Member

Choose a reason for hiding this comment

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

The real problem is ICC doesn't realize the assert never returns.

Although, I don't understand why static char zwoop = '0'; return zwoop; wouldn't work, rather than messing about with an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can try that.

@@ -46,7 +46,7 @@ signal_check_handler(int signal, signal_handler_t handler)
sigact = (void *)oact.sa_sigaction;
}

if (sigact != handler) {
if (sigact != (void *)handler) {
Copy link
Member

Choose a reason for hiding this comment

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

No C-style casts! static_cast<void*>(handler) although we should really rethink using void* in this context.

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 did look / try not using void *, but it turns out the sigaction is not the same type as the handler. So some casting would be needed.

And yeh, I'll change to amc style cast, only because you are the best, and currently sitting in the #1 spot on the ranking.

Copy link
Contributor Author

@zwoop zwoop Feb 16, 2017

Choose a reason for hiding this comment

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

Actually, I'm not gonna change this, because everything else in this file (in a many places) uses the C-style cast already.

This removes a number of the previous warning exclusions that we
we used to do. We then also have to clean up the core code in a
few places, sometimes looking slightly odd (e.g. the meaningless
const that we have in a few places).
@atsci
Copy link

atsci commented Feb 16, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1458/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1564/ for details.

@atsci
Copy link

atsci commented Feb 16, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/129/ for details.

@zwoop zwoop merged commit 6bdc94e into apache:master Feb 16, 2017
@zwoop zwoop deleted the ICC branch February 16, 2017 19:30
@zwoop zwoop modified the milestones: 7.1.0, 7.2.0 Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build work related to build configuration or environment Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants