Skip to content

Conversation

@obastemur
Copy link
Collaborator

andro-chakra

@obastemur
Copy link
Collaborator Author

@dotnet-bot test Windows x86_release please

@digitalinfinity
Copy link
Contributor

Very cool! @EdMaurer FYI

/******************* Compiler-specific glue *******************************/

#ifndef _MSC_VER
#define FEATURE_PAL_SXS 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleanup.

#endif // !_MSC_VER

#if defined(_MSC_VER) || defined(__llvm__)
#define DECLSPEC_ALIGN(x) __declspec(align(x))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused- __declspec(align) is a VC++ concept, no? Wasn't the ifdef that was there correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We use either Clang or VC++.

#define DECLSPEC_NORETURN PAL_NORETURN

#ifndef _MSC_VER
#define __assume(x) (void)0
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand the changes to this file- is the idea basically that this file should never be built with VC++? Otherwise, for instance, this change breaks prefast, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the idea basically that this file should never be built with VC++?

Yes

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

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

Generally, LGTM- just have questions about the changes in Pal.h, which appear to be more than just android-specific changes

@obastemur
Copy link
Collaborator Author

@digitalinfinity Thanks for the review!!

@chakrabot chakrabot merged commit ea6b954 into chakra-core:master Dec 29, 2016
chakrabot pushed a commit that referenced this pull request Dec 29, 2016
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.

4 participants