Fix check on SSL, MBEDTLS, WINSSL exclusivity - #818
Conversation
|
By analyzing the blame information on this pull request, we identified @BurningEnlightenment and @vszakats to be potential reviewers |
| MBEDTLS = $(WITH_MBEDTLS) | ||
| !ENDIF | ||
|
|
||
| !IF ( "$(USE_SSL)"=="true" && "$(USE_WINSSL)"=="true" ) || ( "$(USE_SSL)"=="true" && "$(USE_MBEDTLS)"=="true" ) || ( "$(USE_MBEDTLS)"=="true" && "$(USE_WINSSL)"=="true" ) |
There was a problem hiding this comment.
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! 
There was a problem hiding this comment.
AFAIK, line wrapping like that does not work in Visual C makefiles
There was a problem hiding this comment.
BTW: the parentheses around the && pairs are once again gone in your suggestion. They really should be there.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fine with me. Priority is that the building works.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
With or without the parentheses? If you break it up the logic is clearer, but nevertheless...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added this to the PR. Please merge.
|
thanks! |
|
BTW: building on VC9, VC11, VC14 * x86, x64 confirm that the patch is OK. |
4 participants
See https://curl.haxx.se/mail/lib-2016-05/0129.html