Merge rust-bitcoin/rust-bitcoin#1670: `hash_newtype` cleanup

8ccfb412c1 Improve documentation of `hash_newtype!` (Martin Habovstiak)
58876e2be9 Remove unused macro (Martin Habovstiak)

Pull request description:

  Removed unused macro and improved documentation to address review of #1659 - see commits. I also added a note about recursion.

ACKs for top commit:
  apoelstra:
    ACK 8ccfb412c1
  tcharding:
    ACK 8ccfb412c1

Tree-SHA512: 3b4b0c4ffc8a5166619110d9dcb51affd5cafbb2af84a55dd540a815e4702514d99c71dc1c54aca27fb91970e7e7189d1dffb4f7da7951b0f71336ef6f32d30b

hashes/src/util.rs

@@ -121,11 +121,11 @@ macro_rules! engine_input_impl(
 /// }
 /// ```
 ///
-/// You can use any valid visibility specifier in place of `pub` or you can leave it out if you
-/// don't want the type or its field to be private.
+/// You can use any valid visibility specifier in place of `pub` or you can omit either or both, if
+/// you want the type or its field to be private.
 ///
-/// Whether the hash is reversed or not depends on the inner type. However you can override it like
-/// this:
+/// Whether the hash is reversed or not when displaying depends on the inner type. However you can
+/// override it like this:
 ///
 /// ```
 /// # use bitcoin_hashes::{hash_newtype, sha256};
@@ -156,6 +156,40 @@ macro_rules! engine_input_impl(
 ///     struct Newtype2(hash160::Hash);
 /// }
 /// ```
+///
+/// Note: the macro is internally recursive. If you use too many attributes (> 256 tokens) you may
+/// hit recursion limit. If you have so many attributes for a good reason, just raising the limit
+/// should be OK. Note however that attribute-processing part has to use [TT muncher] which has
+/// quadratic complexity, so having many attributes may blow up compile time. This should be rare.
+///
+/// [TT muncher]: https://danielkeep.github.io/tlborm/book/pat-incremental-tt-munchers.html
+///
+// Ever heard of legendary comments warning developers to not touch the code? Yep, here's another
+// one. The following code is written the way it is for some specific reasons. If you think you can
+// simplify it, I suggest spending your time elsewhere.
+//
+// If you looks at the code carefully you might ask these questions:
+//
+// * Why are attributes using `tt` and not `meta`?!
+// * Why are the macros split like that?!
+// * Why use recursion instead of `$()*`?
+//
+// None of these are here by accident. For some reason unknown to me, if you accept an argument to
+// macro with any fragment specifier other than `tt` it will **not** match any of the rules
+// requiring a specific token. Yep, I tried it, I literally got error that `hash_newtype` doesn't
+// match `hash_newtype`. So all input attributes must be `tt`.
+//
+// Originally I wanted to define a bunch of macros that would filter-out hash_type attributes. Then
+// I remembered (by seeing compiler error) that calling macros is not allowed inside attributes.
+// And no, you can't bypass it by calling a helper macro and passing "output of another macro" into
+// it. The whole macro gets passed, not the resulting value. So we have to generate the entire
+// attributes. And you can't just place an attribute-producing macro above struct - they are
+// considered separate items. This is not C.
+//
+// Thus struct is generated in a separate macro together with attributes. And since the macro needs
+// attributes as the input and I didn't want to create confusion by using `#[]` syntax *after*
+// struct, I opted to use `{}` as a separator. Yes, a separator is required because an attribute
+// may be composed of multiple token trees - that's the point of "double repetition".
 #[macro_export]
 macro_rules! hash_newtype {
     ($($(#[$($type_attrs:tt)*])* $type_vis:vis struct $newtype:ident($(#[$field_attrs:tt])* $field_vis:vis $hash:path);)+) => {
@@ -280,6 +314,16 @@ macro_rules! hash_newtype {
     };
 }
 
+// Generates the struct only (no impls)
+//
+// This is a separate macro to make it more readable and have a separate interface that allows for
+// two groups of type attributes: processed and not-yet-processed ones (think about it like
+// computation via recursion). The macro recursively matches unprocessed attributes, popping them
+// one at a time and either ignoring them (`hash_newtype`) or appending them to the list of
+// processed attributes to be added to the struct.
+//
+// Once the list of not-yet-processed attributes is empty the struct is generated with processed
+// attributes added.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_struct {
@@ -307,13 +351,14 @@ macro_rules! hash_newtype_struct {
     };
 }
 
-#[doc(hidden)]
-#[macro_export]
-macro_rules! hash_newtype_our_attrs {
-    (hash_newtype($($attr:tt)*)) => { $($attr)* };
-    ($($ignore:tt)*) => {};
-}
-
+// Extracts `hash_newtype(forward)` and `hash_newtype(backward)` attributes if any and turns them
+// into bool, defaulting to `DISPLAY_BACKWARD` of the wrapped type if the attribute is omitted.
+//
+// Once an appropriate attribute is found we pass the remaining ones into another macro to detect
+// duplicates/conflicts and report an error.
+//
+// FYI, no, we can't use a helper macro to first filter all `hash_newtype` attributes. We would be
+// attempting to match on macros instead. So we must write `hashe_newtype` in each branch.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_get_direction {
@@ -323,6 +368,9 @@ macro_rules! hash_newtype_get_direction {
     ($hash:ty, #[$($ignore:tt)*]  $($others:tt)*) => { $crate::hash_newtype_get_direction!($hash, $($others)*) };
 }
 
+// Reports an error if any of the attributes is `hash_newtype($direction)`.
+//
+// This is used for detection of duplicates/conflicts, see the macro above.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_forbid_direction {
@@ -338,6 +386,11 @@ macro_rules! hash_newtype_forbid_direction {
     };
 }
 
+// Checks (at compile time) that all `hash_newtype` attributes are known.
+//
+// An unknown attribute could be a typo that could cause problems - e.g. wrong display direction if
+// it's missing. To prevent this, we call this macro above. The macro produces nothing unless an
+// unknown attribute is found in which case it produces `compile_error!`.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_known_attrs {