Who is to blame for this range based for over a reference to temporary

No, it is never bad form to break out of any loop early unless you are aware of an invariant that will always break early. I have seen loops process the first element only:

for (auto item : itemCollection) {
  // do the 95% stuff
  break;
}

This code should never be used, unfortunately I have seen it in production code several times in the past. Of course this prompted a code review.

Anyway, this does not appear to be your case. I would still reconsider the design of your algorithm though. Why break out of the loop just because one element somewhere in the container fails a check? Why not skip that element? Maybe looking at the big picture will reveal a better way of doing whatever you are trying to do. It is difficult for me to give more guidance without more of the code and the overall design.

I think it is a poor strategy to make Derived_1::Impl derive from Base::Impl.

The main purpose of using the Pimpl idiom is to hide the implementation details of a class. By letting Derived_1::Impl derive from Base::Impl, you've defeated that purpose. Now, not only does the implementation of Base depend on Base::Impl, the implementation of Derived_1 also depends on Base::Impl.

Is there a better solution?

That depends on what trade-offs are acceptable to you.

Solution 1

Make Impl classes totally independent. This will imply that there will be two pointers to Impl classes -- one in Base and another one in Derived_N.

class Base {

   protected:
      Base() : pImpl{new Impl()} {}

   private:
      // It's own Impl class and pointer.
      class Impl { };
      std::shared_ptr<Impl> pImpl;

};

class Derived_1 final : public Base {
   public:
      Derived_1() : Base(), pImpl{new Impl()} {}
      void func_1() const;
   private:
      // It's own Impl class and pointer.
      class Impl { };
      std::shared_ptr<Impl> pImpl;
};

Solution 2

Expose the classes only as handles. Don't expose the class definitions and implementations at all.

Public header file:

struct Handle {unsigned long id;};
struct Derived1_tag {};
struct Derived2_tag {};

Handle constructObject(Derived1_tag tag);
Handle constructObject(Derived2_tag tag);

void deleteObject(Handle h);

void fun(Handle h, Derived1_tag tag);
void bar(Handle h, Derived2_tag tag); 

Here's quick implementation

#include <map>

class Base
{
   public:
      virtual ~Base() {}
};

class Derived1 : public Base
{
};

class Derived2 : public Base
{
};

namespace Base_Impl
{
   struct CompareHandle
   {
      bool operator()(Handle h1, Handle h2) const
      {
         return (h1.id < h2.id);
      }
   };

   using ObjectMap = std::map<Handle, Base*, CompareHandle>;

   ObjectMap& getObjectMap()
   {
      static ObjectMap theMap;
      return theMap;
   }

   unsigned long getNextID()
   {
      static unsigned id = 0;
      return ++id;
   }

   Handle getHandle(Base* obj)
   {
      auto id = getNextID();
      Handle h{id};
      getObjectMap()[h] = obj;
      return h;
   }

   Base* getObject(Handle h)
   {
      return getObjectMap()[h];
   }

   template <typename Der>
      Der* getObject(Handle h)
      {
         return dynamic_cast<Der*>(getObject(h));
      }
};

using namespace Base_Impl;

Handle constructObject(Derived1_tag tag)
{
   // Construct an object of type Derived1
   Derived1* obj = new Derived1;

   // Get a handle to the object and return it.
   return getHandle(obj);
}

Handle constructObject(Derived2_tag tag)
{
   // Construct an object of type Derived2
   Derived2* obj = new Derived2;

   // Get a handle to the object and return it.
   return getHandle(obj);
}

void deleteObject(Handle h)
{
   // Get a pointer to Base given the Handle.
   //
   Base* obj = getObject(h);

   // Remove it from the map.
   // Delete the object.
   if ( obj != nullptr )
   {
      getObjectMap().erase(h);
      delete obj;
   }
}

void fun(Handle h, Derived1_tag tag)
{
   // Get a pointer to Derived1 given the Handle.
   Derived1* obj = getObject<Derived1>(h);
   if ( obj == nullptr )
   {
      // Problem.
      // Decide how to deal with it.

      return;
   }

   // Use obj
}

void bar(Handle h, Derived2_tag tag)
{
   Derived2* obj = getObject<Derived2>(h);
   if ( obj == nullptr )
   {
      // Problem.
      // Decide how to deal with it.

      return;
   }

   // Use obj
}

Pros and Cons

With the first approach, you can construct Derived classes in the stack. With the second approach, that is not an option.

With the first approach, you incur the cost of two dynamic allocations and deallocations for constructing and destructing a Derived in the stack. If you construct and destruct a Derived object from the heap you, incur the cost of one more allocation and deallocation. With the second approach, you only incur the cost of one dynamic allocation and one deallocation for every object.

With the first approach, you get have the ability to use virtual member function is Base. With the second approach, that is not an option.

My suggestion

I would go with the first solution so I can use the class hierarchy and virtual member functions in Base even though it is a little bit more expensive.