rpc: Add missing #include

domob1812 commented at 11:13 am on February 18, 2019: contributor

bd0dbe8763fc3029cf96531c9ccaba280b939445 introduced a dependency of rpc/util.h on RPCErrorCode, defined in rpc/protocol.h. The latter file is only included from rpc/util.cpp, though. This commit fixes the missing include, by moving the #include of rpc/protocol.h to rpc/util.h.

Add missing #include.

bd0dbe8763fc3029cf96531c9ccaba280b939445 introduced a dependency of
rpc/util.h on RPCErrorCode, defined in rpc/protocol.h.  The latter file
is only included from rpc/util.cpp, though.  This commit fixes the
missing include, by moving the #include of rpc/protocol.h to
rpc/util.h.

39e20fc54f

fanquake added the label RPC/REST/ZMQ on Feb 18, 2019

DrahtBot commented at 1:25 pm on February 18, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15427 (Add support for descriptors to utxoupdatepsbt by sipa)
#15288 (Remove wallet -> node global function calls by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 5:30 pm on February 18, 2019: member

Why is this specific change needed if master compiles? I mean, there’s probably a lot of other missing includes right? Have you used iwyu?

domob1812 commented at 7:02 am on February 19, 2019: contributor

Why is this specific change needed if master compiles? I mean, there’s probably a lot of other missing includes right? Have you used iwyu?

Master compiles because apparently rpc/util.h is only included together with rpc/protocol.h anyway, so it is fine in that situation. I personally noticed it because of tinkering with the code in a separate effort, where this is apparently not the case.

I think this is a clear bug with a straight-forward fix, so even if it is not strictly necessary to make master compile, it is good to apply.

promag commented at 7:27 am on February 19, 2019: member

Sorry but I don’t see this as a bug fix.

domob1812 commented at 12:41 pm on February 19, 2019: contributor

Sorry but I don’t see this as a bug fix.

Why not? It is clearly a coding mistake to rely on symbols from a header that is not included. This is even explicitly stated in the developer notes:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

That said, I agree that this is of course not a very important fix - but it is also trivial and straight-forward, so has hardly any costs either. But if you think it is not useful to merge simple patches like this, I’m happy to close the PR instead (I’ve fixed the issue for my own projects already, so I don’t need it).

MarcoFalke commented at 1:49 pm on February 19, 2019: member

It would be nice to have a tool to check for those, otherwise it seems like a one-off fix

domob1812 commented at 2:54 pm on February 19, 2019: contributor

It would be nice to have a tool to check for those, otherwise it seems like a one-off fix

I agree, but I don’t have such a tool. As mentioned above, I found this issue because it broke some code I’m working with (based on Bitcoin Core), and I thought it would be useful to fix this, once found, upstream.

promag commented at 3:02 pm on February 19, 2019: member

That’s my point, there’s no bug or code to fix. If you submit a pull that needs this “fixed” then let’s review it there.

MarcoFalke commented at 3:21 pm on February 19, 2019: member

There’d be iwyu, but it isn’t a trivial process to set up: #15442

domob1812 commented at 6:53 am on February 20, 2019: contributor

That’s my point, there’s no bug or code to fix. If you submit a pull that needs this “fixed” then let’s review it there.

There is code to fix, since the current code is simply not correct and just compiles by accident. It also violates the style explicitly described in the developer notes as noted above.

But if you don’t think it is worthwhile to push a fix like this, then let’s just close the PR. But to be honest, I don’t understand what the potential downside of a trivial change like this is (even if the benefits are limited as well from a practical point of view).

practicalswift commented at 7:19 pm on February 20, 2019: contributor

utACK 39e20fc54ff1b57d6e046fc1215e2c2f7f89a97e

Solves a problem for @domob1812: no need to fight this fix.

laanwj commented at 8:42 am on February 21, 2019: member

I mean this opens the flood-gates to a torrent of similar PRs, which doesn’t really solve a problem with any known compiler, so mild NACK from me.

domob1812 commented at 2:23 pm on February 21, 2019: contributor

I mean this opens the flood-gates to a torrent of similar PRs, which doesn’t really solve a problem with any known compiler, so mild NACK from me.

I understand if that’s the general view. I agree that this PR has a very small (although positive) impact. Once found, I wanted to contribute this fix back upstream, and expected it to be trivially to review and quick to merge, thus I didn’t see any downsides. But given how controversial this seems to be, it is obviously wasting more developer attention and time than I expected it to do.

If there is a general “bar” for not accepting too-small PRs (even if positive), perhaps that should be defined explicitly and stated somewhere in the developer notes?

As for this particular PR - I think it would be nice to merge it, as it is an improvement (although very small). But the real issue that led me to this on another project is obviously not relevant for Bitcoin’s decisions, and already fixed. So I don’t need it merged if you’d rather not set a precedent. Feel free to close in that case.

laanwj commented at 6:12 pm on February 21, 2019: member

Yea as said I’m not strongly against this, and when this can be automatically checked by some tool I’d be in favor of doing that. It’s just that doing this include by include, seems inefficient, so I’d prefer if you group multiple similar changes, that’s all.

If there is a general “bar” for not accepting too-small PRs (even if positive), perhaps that should be defined explicitly and stated somewhere in the developer notes?

We do have this line in contributing.md:

Trivial pull requests or pull requests that refactor the code with no clear benefits may be immediately closed by the maintainers to reduce unnecessary workload on reviewing.

I mean “no clear benefits” can be arguable in this case. And if it helps you with whatever upstream project I’ll utACK this for this time. But please don’t make a habit out of it.

utACK 39e20fc54ff1b57d6e046fc1215e2c2f7f89a97e

fanquake commented at 2:35 am on February 22, 2019: member

utACK 39e20fc

Hopefully discussion in #15442 might result in something automated to keep on top of these changes.

MarcoFalke added the label Refactoring on Feb 22, 2019

MarcoFalke removed the label RPC/REST/ZMQ on Feb 22, 2019

MarcoFalke renamed this:
~~Add missing #include~~
rpc: Add missing #include
on Feb 22, 2019

MarcoFalke referenced this in commit f3f9c1de19 on Feb 22, 2019

MarcoFalke merged this on Feb 22, 2019

MarcoFalke closed this on Feb 22, 2019

domob1812 commented at 8:22 am on February 23, 2019: contributor

Thanks everyone! I’m sorry for wasting everyone’s time, I didn’t intend that - thought it would just be quick to review and merge, as the change is trivial. I’ll not submit things like that in the future.

domob1812 deleted the branch on Feb 23, 2019

deadalnix referenced this in commit dc38fa1b73 on May 28, 2020

PastaPastaPasta referenced this in commit 4c014306b5 on Aug 16, 2021

PastaPastaPasta referenced this in commit 1ccb57648a on Aug 16, 2021

PastaPastaPasta referenced this in commit b45386011a on Aug 18, 2021

DrahtBot locked this on Dec 16, 2021

rpc: Add missing #include #15435

Conflicts