Skip to content

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 5, 2017

Nit: We expect to find primitives/transaction.h locally in src/.

@jtimon
Copy link
Contributor

jtimon commented Jul 5, 2017

ACK 0357ce0 (trivial review)

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jul 5, 2017

All aboard the obvious-ACK train :).

utACK.

@fanquake
Copy link
Member

fanquake commented Jul 6, 2017

utACK 0357ce0

@meshcollider
Copy link
Contributor

utACK 0357ce0

@laanwj
Copy link
Member

laanwj commented Jul 6, 2017

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

With quotes should be relative?

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

@jtimon
Copy link
Contributor

jtimon commented Jul 7, 2017

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 practicalswift changed the title Use quoted form when including primitives/transaction.h Use quoted form when including primitives/transaction.h and wallet/wallet.h Jul 26, 2017
@practicalswift
Copy link
Contributor Author

Added a commit fixing the same thing when importing wallet/wallet.h :-) Introduced in 9737572.

@laanwj
Copy link
Member

laanwj commented Aug 15, 2017

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 (#10976 (comment)) and I think that'd be superior.

@laanwj laanwj closed this Aug 15, 2017
@practicalswift
Copy link
Contributor Author

Makes sense! :-)

@practicalswift practicalswift deleted the transaction-h branch April 10, 2021 19:32
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants