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

Small changes made according to static analyzer results #42

Merged
merged 10 commits into from
Sep 11, 2018
Merged

Small changes made according to static analyzer results #42

merged 10 commits into from
Sep 11, 2018

Conversation

fuzun
Copy link
Contributor

@fuzun fuzun commented Aug 30, 2018

I did not want to to change type of feature variables since it seems that they are getting accessed directly by other projects but could not find any other solution. Making them bool would probably break portability. Other option might be defining TRUE as -1 then modifying related parts.

Feature variable returning functions (Get...FeaturesEnumValue) are kept intact as automatic casting should not cause any problem. And because variables are all aligned, there should be no warning because of this casting in compilers other than VS. (I did not test though).

…and 'false' & IsBitSet()

It seems that ' : 1' alignments cause signed integers to be either -1 or 0. While -1 is true and 0 is false reverse might not be always correct when true is defined 1.

Maybe change feature variables to bool ?
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot
Copy link

CLAs look good, thanks!

@gchatelet
Copy link
Collaborator

gchatelet commented Sep 7, 2018

So unfortunately this PR is changing the API and may break client code.
I'm willing to pull everything in but the int -> unsigned intmodification (we would also need to modify the other platforms arm, aarch64, power ...). Can you revert this part?

@fuzun fuzun closed this Sep 8, 2018
@fuzun fuzun reopened this Sep 8, 2018
@fuzun
Copy link
Contributor Author

fuzun commented Sep 8, 2018

I have realized that bit_utils.h is internal so made some changes there instead of structs. Still not the best but I could not think any other solution without changing structures.

Copy link
Collaborator

@gchatelet gchatelet left a comment

Choose a reason for hiding this comment

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

Let's set 58a232c aside for now. I have to think this through. The rest of the PR is welcome though.

include/internal/bit_utils.h Outdated Show resolved Hide resolved
@fuzun
Copy link
Contributor Author

fuzun commented Sep 11, 2018

Yeah it is definitely not the best way since something like x == TRUE can result in absolute mess if TRUE is used instead of true mistakenly.

I could not think any other high level change to fix this so I agree that this commit should be dropped in favor of a potential lower level change later.

I'm reverting this commit now.

@gchatelet
Copy link
Collaborator

Thx a lot for your patience and for contributing to cpu_features.
Can you open a bug about the bool correctness issue so I keep it on my radar?

@gchatelet gchatelet merged commit 7863534 into google:master Sep 11, 2018
@fuzun
Copy link
Contributor Author

fuzun commented Sep 11, 2018

Sure!
Issue: #43

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.

None yet

3 participants