Use quoted form when including primitives/transaction.h and wallet/wallet.h #10752

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:transaction-h changing 2 files +2 −2

practicalswift commented at 9:22 PM on July 5, 2017: contributor

Nit: We expect to find primitives/transaction.h locally in src/.
jtimon commented at 10:07 PM on July 5, 2017: contributor

ACK 0357ce0 (trivial review)
TheBlueMatt commented at 10:07 PM on July 5, 2017: member

All aboard the obvious-ACK train :).

utACK.
fanquake added the label Refactoring on Jul 6, 2017
fanquake commented at 12:21 AM on July 6, 2017: member

utACK 0357ce0
meshcollider commented at 12:28 AM on July 6, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/10752/commits/0357ce0740e5fc168b408f8fd55aa7ba5531a082
laanwj commented at 2:29 PM on July 6, 2017: member
Nit: We expect to find primitives/transaction.h locally in src/.

utACK as it matches the common convention better.

Though sometimes I wonder whether this convention is a good idea. include <> searches relative to the include paths, include "" relative to the directory of the current source file, then the include paths. We know primitives/ is relative to the source root of the repository, so it doesn't really need to look anywhere else. Especially not if it is already in primitives/, e.g.
- when primitives/block.h includes "primitives/transaction.h", then it makes it look in src/primitives/primitives
- when src/wallet/feebumper.h includes "primitives/transaction.h", it makes it look in src/wallet/primitives!
So with regard to unambiguity, this is a change for the worse.

in src/wallet/feebumper.h:8 in 0357ce0740 outdated

   4 | @@ -5,7 +5,7 @@
   5 |  #ifndef BITCOIN_WALLET_FEEBUMPER_H
   6 |  #define BITCOIN_WALLET_FEEBUMPER_H
   7 |  
   8 | -#include <primitives/transaction.h>
   9 | +#include "primitives/transaction.h"

promag commented at 9:54 AM on July 7, 2017:

With quotes should be relative?

#include "../primitives/transaction.h"

jtimon commented at 7:28 PM on July 7, 2017: contributor

The advantage of using #include "primitives/transaction.h" instead of simply #include "transaction.h" is that one day we may want to search and replace all #include "primitives/transaction.h" say, to a new directory. If you moved primitives/block.o to the same directory at the same time it would be fine though.
practicalswift force-pushed on Jul 26, 2017
practicalswift renamed this:
~~Use quoted form when including primitives/transaction.h~~
Use quoted form when including primitives/transaction.h and wallet/wallet.h
on Jul 26, 2017
practicalswift commented at 3:14 PM on July 26, 2017: contributor

Added a commit fixing the same thing when importing wallet/wallet.h :-) Introduced in 97375727b8f3b7d26c7c813630a6139005b5c5c9.
practicalswift force-pushed on Jul 27, 2017
Use quoted form when including wallet.h 6bfce90431
Use quoted form when including transaction.h a9ea61afa5
practicalswift force-pushed on Aug 3, 2017
laanwj commented at 7:40 AM on August 15, 2017: member

If you don't mind, I'm going to close this. There has been talk of moving to absolute inclusion (e.g. #include <>) instead on the longer run (https://github.com/bitcoin/bitcoin/pull/10976#issuecomment-322397113) and I think that'd be superior.
laanwj closed this on Aug 15, 2017
practicalswift commented at 7:45 AM on August 15, 2017: contributor

Makes sense! :-)
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#11053 refactor: Make all #includes relative to project root