[PATCH] RFC: fail-fast iterators for DenseMap

[Duncan P. N. Exon Smith]

> On 2015 Feb 28, at 23:46, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> 
> Address Chandler's review.
> 
> - no "ADT" in the file or class names.
> - changed the vocabulary to "Handle" and "Host"
> - suffixed the class names with "Base" and change the DenseMap implementation to use inheritance over composition.
> - handled the NDEBUG-only-ness directly in the header file.  Once this was locked down, I could not justify the additional complexity from the template specialization, so I removed that too.
> 
> 
> http://reviews.llvm.org/D7931

This is much cleaner than the original implementation.  Nice!  I have a
few nitpicks:

> Index: include/llvm/ADT/EpochTracker.h
> ===================================================================
> --- /dev/null
> +++ include/llvm/ADT/EpochTracker.h
> @@ -0,0 +1,65 @@
> +//===- llvm/ADT/EpochTracker.h - ADT epoch tracking-*- C++ -*--------------===//

Missing a space here ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

Also, don't we usually dump the C++ mode at the end of the line?

    //===- llvm/ADT/EpochTracker.h - ADT epoch tracking -------------*- C++ -*-===//


> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file defines the EpochTrackerHostBase and EpochTrackerHandleBase
> +// classes.  These can be used to write iterators that are fail-fast when LLVM
> +// is built with asserts enabled.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef LLVM_ADT_EPOCH_TRACKER_H
> +#define LLVM_ADT_EPOCH_TRACKER_H
> +
> +namespace llvm {
> +
> +#ifdef NDEBUG
> +
> +// These classes are no-op in a build without asserts.

This should probably just be part of the class documentation for each
class.

> +
> +class EpochTrackerHostBase {

I think the NDEBUG should go here (inside the class), and you should add
some short doxygen to the class.

> +public:
> +  void bumpEpoch() {}
> +};
> +
> +class EpochTrackerHandleBase {

(You'd obviously need a separate NDEBUG here.)

> +public:
> +  EpochTrackerHandleBase(const EpochTrackerHostBase *) {}

I'd probably mark this `explicit`.

I think you might need this as well:

    EpochTrackerHandleBase() {}

> +  bool isHandleInSync() { return true; }
> +};
> +
> +#else
> +
> +class EpochTrackerHostBase {
> +  uint64_t Epoch;
> +
> +public:
> +  EpochTrackerHostBase() : Epoch(0){};

This semi-colon ~~~~~~~~~~~~~~~~~~~~~~~~^ will cause problems with some
compilers.  Also, `clang-format` would put a space before the `{}`.

> +  void bumpEpoch() { Epoch++; }

Not that it really matters here, but `++Epoch` is canonical.

> +  ~EpochTrackerHostBase() { bumpEpoch(); }

Is this to catch use-after-frees?  Might be worth a doxygen comment.

> +  friend class EpochTrackerHandleBase;
> +};
> +
> +class EpochTrackerHandleBase {
> +  const uint64_t *HostEpochAddress;
> +  uint64_t EpochAtCreation;
> +
> +public:
> +  EpochTrackerHandleBase() : HostEpochAddress(nullptr), EpochAtCreation(-1) {}

I'd prefer `UINT64_MAX` over `-1` here.

> +
> +  EpochTrackerHandleBase(const EpochTrackerHostBase *Host)
> +      : HostEpochAddress(&Host->Epoch), EpochAtCreation(Host->Epoch) {}

(This should also be `explicit` if you change the NDEBUG version.)

> +
> +  bool isHandleInSync() const { return *HostEpochAddress == EpochAtCreation; }
> +};
> +
> +#endif // !defined(NDEBUG)
> +
> +} // namespace llvm
> +
> +#endif