DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Fix build error caused by -Werror=class-memaccess #823

Closed
hooddanielc wants to merge 2 commits from fix-werror-class-memaccess into master
pull from: fix-werror-class-memaccess
merge into: DaddyFrosty:master
Conversation 6 Commits 2 Files Changed 1 +3 -1
hooddanielc commented 2018-10-07 15:30:43 -07:00 (Migrated from github.com)
Copy Link

OS: Arch Linux
GCC Version: gcc (GCC) 8.2.1 20180831
Clang Version: 6.0.1 (tags/RELEASE_601/final)

Build Log Before Fix:

command: buck build //:yoga

Not using buckd because watchman isn't installed.
yoga/Yoga.cpp: In function ‘void YGZeroOutLayoutRecursivly(YGNodeRef)’:
yoga/Yoga.cpp:1854:51: error: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct YGLayout’; use assignment or value-initialization instead [-Werror=class-memaccess]
   memset(&(node->getLayout()), 0, sizeof(YGLayout));
                                                   ^
In file included from yoga/YGNode.h:11,
                 from yoga/Utils.h:9,
                 from yoga/Yoga.cpp:13:
yoga/YGLayout.h:12:8: note: ‘struct YGLayout’ declared here
 struct YGLayout {
        ^~~~~~~~
cc1plus: all warnings being treated as errors
Build failed: Command failed with exit code 1.
stderr: yoga/Yoga.cpp: In function ‘void YGZeroOutLayoutRecursivly(YGNodeRef)’:
yoga/Yoga.cpp:1854:51: error: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct YGLayout’; use assignment or value-initialization instead [-Werror=class-memaccess]
   memset(&(node->getLayout()), 0, sizeof(YGLayout));
                                                   ^
In file included from yoga/YGNode.h:11,
                 from yoga/Utils.h:9,
                 from yoga/Yoga.cpp:13:
yoga/YGLayout.h:12:8: note: ‘struct YGLayout’ declared here
 struct YGLayout {
        ^~~~~~~~
cc1plus: all warnings being treated as errors
    When running <c++ preprocess_and_compile>.
    When building rule //:yoga#compile-Yoga.cpp.o9b5477b5,default.
Parsing buck files: finished in 0.8 sec (100%)
Building: finished in 2.2 sec (100%) 10/10 jobs, 1 updated
  Total time: 3.3 sec

Build Log After Fix

command: buck build //:yoga

Not using buckd because watchman isn't installed.
Parsing buck files: finished in 0.8 sec (100%)
Building: finished in 0.6 sec (100%) 1/1 jobs, 0 updated
  Total time: 1.6 sec

All tests are passing

OS: Arch Linux GCC Version: gcc (GCC) 8.2.1 20180831 Clang Version: 6.0.1 (tags/RELEASE_601/final) Build Log Before Fix: command: `buck build //:yoga` ``` Not using buckd because watchman isn't installed. yoga/Yoga.cpp: In function ‘void YGZeroOutLayoutRecursivly(YGNodeRef)’: yoga/Yoga.cpp:1854:51: error: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct YGLayout’; use assignment or value-initialization instead [-Werror=class-memaccess] memset(&(node->getLayout()), 0, sizeof(YGLayout)); ^ In file included from yoga/YGNode.h:11, from yoga/Utils.h:9, from yoga/Yoga.cpp:13: yoga/YGLayout.h:12:8: note: ‘struct YGLayout’ declared here struct YGLayout { ^~~~~~~~ cc1plus: all warnings being treated as errors Build failed: Command failed with exit code 1. stderr: yoga/Yoga.cpp: In function ‘void YGZeroOutLayoutRecursivly(YGNodeRef)’: yoga/Yoga.cpp:1854:51: error: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct YGLayout’; use assignment or value-initialization instead [-Werror=class-memaccess] memset(&(node->getLayout()), 0, sizeof(YGLayout)); ^ In file included from yoga/YGNode.h:11, from yoga/Utils.h:9, from yoga/Yoga.cpp:13: yoga/YGLayout.h:12:8: note: ‘struct YGLayout’ declared here struct YGLayout { ^~~~~~~~ cc1plus: all warnings being treated as errors When running <c++ preprocess_and_compile>. When building rule //:yoga#compile-Yoga.cpp.o9b5477b5,default. Parsing buck files: finished in 0.8 sec (100%) Building: finished in 2.2 sec (100%) 10/10 jobs, 1 updated Total time: 3.3 sec ``` Build Log After Fix command: `buck build //:yoga` ``` Not using buckd because watchman isn't installed. Parsing buck files: finished in 0.8 sec (100%) Building: finished in 0.6 sec (100%) 1/1 jobs, 0 updated Total time: 1.6 sec ``` All tests are passing
facebook-github-bot (Migrated from github.com) reviewed 2018-10-22 04:30:10 -07:00
facebook-github-bot (Migrated from github.com) left a comment
Copy Link

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

passy has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D10486023).
davidaurelio (Migrated from github.com) requested changes 2018-10-25 09:07:00 -07:00
davidaurelio (Migrated from github.com) left a comment
Copy Link

Thanks for helping to make Yoga better.

I have a specific concern with using static_cast<>, and I believe that it isn’t necessary at all.

The memset call is probably a relict from the times when Yoga was pure C.

Thanks for helping to make Yoga better. I have a specific concern with using `static_cast<>`, and I believe that it isn’t necessary at all. The `memset` call is probably a relict from the times when Yoga was pure C.
yoga/Yoga.cpp
@@ -1851,7 +1851,9 @@ static bool YGNodeFixedSizeSetMeasuredDimensions(
}
davidaurelio (Migrated from github.com) commented 2018-10-25 09:05:28 -07:00
Copy Link

while you are at it, please leave the place in a better state. static_cast<> sidesteps the type system where we can just leverage default constructor + assignment.

  node->getLayout() = {};
while you are at it, please leave the place in a better state. `static_cast<>` sidesteps the type system where we can just leverage default constructor + assignment. ```suggestion node->getLayout() = {}; ```
hooddanielc (Migrated from github.com) reviewed 2018-10-25 23:26:47 -07:00
yoga/Yoga.cpp
@@ -1851,7 +1851,9 @@ static bool YGNodeFixedSizeSetMeasuredDimensions(
}
hooddanielc (Migrated from github.com) commented 2018-10-25 23:26:47 -07:00
Copy Link

👍 I agree this is much better. When doing so, test results break because YGLayout::YGLayout() initializes dimensions({YGUndefined, YGUndefined}) instead of dimensions({0, 0}). After setting dimensions to zero after assignment, tests are passing.

  node->getLayout() = {};
  node->setLayoutDimension(0, 0);
  node->setLayoutDimension(0, 1);
:+1: I agree this is much better. When doing so, test results break because `YGLayout::YGLayout()` initializes `dimensions({YGUndefined, YGUndefined})` instead of `dimensions({0, 0})`. After setting dimensions to zero after assignment, tests are passing. ```suggestion node->getLayout() = {}; node->setLayoutDimension(0, 0); node->setLayoutDimension(0, 1); ```
woehrl01 (Migrated from github.com) reviewed 2018-10-26 01:59:00 -07:00
yoga/Yoga.cpp
@@ -1851,7 +1851,9 @@ static bool YGNodeFixedSizeSetMeasuredDimensions(
}
woehrl01 (Migrated from github.com) commented 2018-10-26 01:59:00 -07:00
Copy Link

@hooddanielc, @davidaurelio If the test results brake I think this looks like a bug of the constructor of YGLayout mustn't this be initialized to {0,0} per default in the ctor ?

@hooddanielc, @davidaurelio If the test results brake I think this looks like a bug of the constructor of `YGLayout` mustn't this be initialized to `{0,0}` per default in the ctor ?
hooddanielc (Migrated from github.com) reviewed 2018-10-26 17:04:28 -07:00
yoga/Yoga.cpp
@@ -1851,7 +1851,9 @@ static bool YGNodeFixedSizeSetMeasuredDimensions(
}
hooddanielc (Migrated from github.com) commented 2018-10-26 17:04:28 -07:00
Copy Link

@woehrl01 That is what I thought at first, then I found some history in comments above YGUndefined definition.

/** Large positive number signifies that the property(float) is undefined.
 *Earlier we used to have YGundefined as NAN, but the downside of this is that
 *we can't use -ffast-math compiler flag as it assumes all floating-point
 *calculation involve and result into finite numbers. For more information
 *regarding -ffast-math compiler flag in clang, have a look at
 *https://clang.llvm.org/docs/UsersManual.html#cmdoption-ffast-math
 **/
#define YGUndefined 10E20F

I think the alternatives of using default constructor or changing the constructor may be a breaking change. Could we add a new constructor to YGLayout? Does the C++ delegate constructor allow us to override part of that constructors initialization list?

@woehrl01 That is what I thought at first, then I found some history in comments above `YGUndefined` definition. ``` /** Large positive number signifies that the property(float) is undefined. *Earlier we used to have YGundefined as NAN, but the downside of this is that *we can't use -ffast-math compiler flag as it assumes all floating-point *calculation involve and result into finite numbers. For more information *regarding -ffast-math compiler flag in clang, have a look at *https://clang.llvm.org/docs/UsersManual.html#cmdoption-ffast-math **/ #define YGUndefined 10E20F ``` I think the alternatives of using default constructor or changing the constructor may be a breaking change. Could we add a new constructor to `YGLayout`? Does the C++ delegate constructor allow us to override part of that constructors initialization list?
facebook-github-bot (Migrated from github.com) reviewed 2018-10-29 06:54:47 -07:00
facebook-github-bot (Migrated from github.com) left a comment
Copy Link

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

passy has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D10486023).

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
davidaurelio
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#823
No description provided.
Delete Branch "fix-werror-class-memaccess"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?