Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Native: Multiple fixes#3479

Merged
nguerrera merged 2 commits into
masterfrom
unknown repository
Sep 26, 2015
Merged

Native: Multiple fixes#3479
nguerrera merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 26, 2015

067ea11 d9cf2d8
207d186 e348434
  • Removes unrequired dependencies from native src/Native/gen-buildsys-clang.sh.
    • It builds without llvm-* dependencies.
    • Cleanup unused code blocks.
    • Minor conditions and comment fixes.

@ghost ghost changed the title Native: Fixes unnecessary/wrong cast Native: Multiple fixes Sep 26, 2015
@stephentoub
Copy link
Copy Markdown
Member

cc: @nguerrera, @sokket

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why is this needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is for the check #ifdef BSD:
sys/param.h is available in all POSIX and BSD macro is only defined in BSD-backed OSes. (ref: http://sourceforge.net/p/predef/wiki/OperatingSystems/, https://technet.microsoft.com/en-us/library/bb463220.aspx)

@nguerrera
Copy link
Copy Markdown
Contributor

Thanks, @jasonwilliams200OK. I had two comments, but the rest looks good.

Comment thread src/Native/gen-buildsys-clang.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just found a third issue: Are we even using locate_llvm_exec or llvm_prefix anymore?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nice catch! I thought it is used to check llvm existence and early exit. Fixed by 9e22f86.

Peter added 2 commits September 26, 2015 21:39
Clang on FreeBSD caught this as an error.
* It builds without llvm-* dependencies.
* Minor conditions and comment fixes.
@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 26, 2015

Thanks for the review @nguerrera. I have fixed the issues and re-pushed.

Regarding the assert comment, do you want me to address it by checking id_t width vs. int32_t (using sizeof operator) as part of this PR?

@nguerrera
Copy link
Copy Markdown
Contributor

Nevermind the assert since it was already missing. (Aside: Note that sizeof() check isn't sufficient since it doesn't account for possible difference in signedness between int32_t and id_t.)

I'm going to make another change to make potentially narrowing conversions like that go through a helper that also avoids having to cmake configure every occurrence like this. It'll also help clean up some things in the crypto shim.

This LGTM once it gets through a successful CI.

nguerrera added a commit that referenced this pull request Sep 26, 2015
@nguerrera nguerrera merged commit e5c73c4 into dotnet:master Sep 26, 2015
@ghost ghost deleted the native branch September 26, 2015 23:53
@karelz karelz added this to the 1.0.0-rtm milestone Jan 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants