Can someone help me convince apache-thrift accept this PR which fix LDC build
Vladimir Panteleev
thecybershadow.lists at gmail.com
Thu Jun 24 20:15:04 UTC 2021
On Wednesday, 23 June 2021 at 20:12:46 UTC, mw wrote:
>
> https://github.com/apache/thrift/pull/2404
I think the pull request needs work, though the reviewer didn't
do a good job of explaining why.
1. It's not clear what causes the error message exactly. How
could `llvm_bswap` conflict with `nativeToBigEndian`? They have
different names. There should be an explanation for how the
conflict arose, why it happens only with LDC, and how your
proposed change fixes it.
2. It's not explained what the conflict problem has anything to
do with OpenSSL. Your pull request description makes no mention
about OpenSSL, yet most of the changes in the diff are concerning
it one way or the other.
3. It's not explained what the OpenSSL changes are even needed
for. "Fixing LDC" is not a sufficient explanation, they need to
explain what exactly is broken, why it is broken, what is being
changed, and how the change fixes the problem.
4. The OpenSSL changes really should be in a completely separate
pull request.
5. If for whatever reason the OpenSSL changes cannot be in a
separate pull request, then it should be explained why.
6. There should also be a justification for why the problem is
being fixed by making a change (or changes) in Apache Thrift, and
not in LDC on the D OpenSSL bindings etc.
All answers to these questions should be included in commit
messages or the pull request description.
I suggest closing that PR, and opening a new PR (or PRs) which
address the above.
More information about the Digitalmars-d
mailing list