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

Makefile: fix for GCC 7+ and clang 4+ #2681

Closed
wants to merge 5 commits into from
Closed

Makefile: fix for GCC 7+ and clang 4+ #2681

wants to merge 5 commits into from

Conversation

choleraehyq
Copy link

@maysamyabandeh @IslamAbdelRahman PTAL

Fix #2672

Signed-off-by: Cholerae Hu huyingqian@pingcap.com

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Makefile Outdated
@@ -255,6 +255,19 @@ default: all
WARNING_FLAGS = -W -Wextra -Wall -Wsign-compare -Wshadow \
-Wno-unused-parameter

CCVERSION = $(shell cc -dumpversion)
Copy link
Contributor

Choose a reason for hiding this comment

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

cc -> $CXX
or ${CXX:-cc} though I have doubts that cc is right.

@facebook-github-bot
Copy link
Contributor

@choleraehyq updated the pull request - view changes

Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@facebook-github-bot
Copy link
Contributor

@choleraehyq updated the pull request - view changes

@siying
Copy link
Contributor

siying commented Aug 3, 2017

Is it possible to fix the code rather than suppress the warning?

@ajkr
Copy link
Contributor

ajkr commented Aug 3, 2017

@siying this should enable compiler support for allocating over-aligned types.

We still don't use new properly in older gcc versions. But I don't think it's critical as we over-align to protect against false cache-line sharing. So it should be a perf issue, not a correctness issue.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

Thanks!

Makefile Outdated

ifeq ($(CCNAME), clang)
ifeq ($(CCVERSION), 4*)
WARNING_FLAGS += -faligned-new
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put it in CXX_FLAGS instead as it's not really a warning flag?

Cholerae Hu added 3 commits August 4, 2017 09:03
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@facebook-github-bot
Copy link
Contributor

@choleraehyq updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@choleraehyq updated the pull request - view changes

@ajkr
Copy link
Contributor

ajkr commented Aug 4, 2017

Sorry our tools won't let me pull it unless the CLA is signed (https://code.facebook.com/cla).

@choleraehyq
Copy link
Author

Signed! @ajkr

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

None yet

5 participants