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

Fix check on SSL, MBEDTLS, WINSSL exclusivity #818

Closed
wants to merge 2 commits into from
Closed

Fix check on SSL, MBEDTLS, WINSSL exclusivity #818

wants to merge 2 commits into from

Conversation

Jan-E
Copy link
Contributor

@Jan-E Jan-E commented May 19, 2016

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @BurningEnlightenment and @vszakats to be potential reviewers

@@ -118,6 +114,10 @@ USE_MBEDTLS = true
MBEDTLS = $(WITH_MBEDTLS)
!ENDIF

!IF ( "$(USE_SSL)"=="true" && "$(USE_WINSSL)"=="true" ) || ( "$(USE_SSL)"=="true" && "$(USE_MBEDTLS)"=="true" ) || ( "$(USE_MBEDTLS)"=="true" && "$(USE_WINSSL)"=="true" )
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pretty long line, I suggest you wrap it like so:

!IF "$(USE_SSL)"=="true" && "$(USE_WINSSL)"=="true" \
 || "$(USE_SSL)"=="true" && "$(USE_MBEDTLS)"=="true" \
 || "$(USE_MBEDTLS)"=="true" && "$(USE_WINSSL)"=="true"

Otherwise well done! :shipit:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, line wrapping like that does not work in Visual C makefiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW: the parentheses around the && pairs are once again gone in your suggestion. They really should be there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't checked the docs whether it is supposed to work with preprocessor directives, but it is supported for commands as documented here and I used it in practice without encountering any issues.

BTW: the parentheses around the && pairs are once again gone in your suggestion. They really should be there.

No they are unnecessary, because of a feature called operator precedence which is documented here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parentheses may be unnecessary, but for readability I would still leave them there.

The backslash followed by a line-end was indeed documented for commands I see now, even for VC9. But line-ends and Windows are messy stuff, so why not choose the safe solution? Better safe than sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine with me. Priority is that the building works.

Copy link
Member

@bagder bagder May 20, 2016

Choose a reason for hiding this comment

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

I (although I like to say we so that it doesn't sound so much like a dictatorship), like code that isn't terribly wide as it makes it easier to read so I'm for breaking the line up into several with backslash-continuations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With or without the parentheses? If you break it up the logic is clearer, but nevertheless...

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on that but would leave that to you who're writing this, but my general preference is to add parentheses to help readers to not have to dig out any forgotten precedence rules to understand the order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to the PR. Please merge.

@bagder bagder closed this in 6bdc609 May 20, 2016
@bagder
Copy link
Member

bagder commented May 20, 2016

thanks!

@Jan-E
Copy link
Contributor Author

Jan-E commented May 20, 2016

BTW: building on VC9, VC11, VC14 * x86, x64 confirm that the patch is OK.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants