[flang] Use reference_wrapper in vectors and sets

Convert some CharBlock references to values

Replace more pointers with reference wrappers

Restore object references that were converted to value semantics in an earlier commit

Use Reference<> in Scope

Fix new component iterator

Document pitfall that bit me

final tweaks before rebasing and merging

Rebasing

Original-commit: flang-compiler/f18@87874af
Reviewed-on: flang-compiler/f18#788

flang/documentation/C++style.md

@@ -159,10 +159,14 @@ class.
 clear.  Consider reworking any code that uses `malloc()` or a (non-placement)
 `operator new`.
 See the section on Pointers below for some suggested options.
-1. Use references for `const` arguments; prefer `const` references to values for
-all but small types that are trivially copyable (e.g., use `const std::string &`
-and `int`).  Use non-`const` pointers for output arguments.  Put output arguments
-last (_pace_ the standard C library conventions for `memcpy()` & al.).
+1. When defining argument types, use values when object semantics are
+not required and the value is small and copyable without allocation
+(e.g., `int`);
+use `const` or rvalue references for larger values (e.g., `std::string`);
+use `const` references to rather than pointers to immutable objects;
+and use non-`const` references for mutable objects, including "output" arguments
+when they can't be function results.
+Put such output arguments last (_pace_ the standard C library conventions for `memcpy()` & al.).
 1. Prefer `typename` to `class` in template argument declarations.
 1. Prefer `enum class` to plain `enum` wherever `enum class` will work.
 We have an `ENUM_CLASS` macro that helps capture the names of constants.
@@ -206,18 +210,29 @@ data in this project.
 Some of these are standard C++ language and library features,
 while others are local inventions in `lib/common`:
 * Bare pointers (`Foo *p`): these are obviously nullable, non-owning,
-undefined when uninitialized, shallowly copyable, reassignable, and almost
-never the right abstraction to use in this project.
+undefined when uninitialized, shallowly copyable, reassignable, and often
+not the right abstraction to use in this project.
+But they can be the right choice to represent an optional
+non-owning reference, as in a function result.
+Use the `DEREF()` macro to convert a pointer to a reference that isn't
+already protected by an explicit test for null.
 * References (`Foo &r`, `const Foo &r`): non-nullable, not owning,
 shallowly copyable, and not reassignable.
 References are great for invisible indirection to objects whose lifetimes are
 broader than that of the reference.
-(Sometimes when a class data member should be a reference, but we also need
-reassignability, it will be declared as a pointer, and its accessor
-will be defined to return a reference.)
+Take care when initializing a reference with another reference to ensure
+that a copy is not made because only one of the references is `const`;
+this is a pernicious C++ language pitfall!
 * Rvalue references (`Foo &&r`): These are non-nullable references
 *with* ownership, and they are ubiquitously used for formal arguments
 wherever appropriate.
+* `std::reference_wrapper<>`: non-nullable, not owning, shallowly
+copyable, and (unlike bare references) reassignable, so suitable for
+use in STL containers and for data members in classes that need to be
+copyable or assignable.
+* `common::Reference<>`: like `std::reference_wrapper<>`, but also supports
+move semantics, member access, and comparison for equality; suitable for use in
+`std::variant<>`.
 * `std::unique_ptr<>`: A nullable pointer with ownership, null by default,
 not copyable, reassignable.
 F18 has a helpful `Deleter<>` class template that makes `unique_ptr<>`
@@ -240,14 +255,17 @@ of that standard feature is prohibitive.
 
 A feature matrix:
 
-| pointer              | nullable | default null | owning | reassignable | copyable          | undefined type ok? |
-| -------              | -------- | ------------ | ------ | ------------ | --------          | ------------------ |
-| `*p`                 | yes      | no           | no     | yes          | shallowly         | yes                |
-| `&r`                 | no       | n/a          | no     | no           | shallowly         | yes                |
-| `unique_ptr<>`       | yes      | yes          | yes    | yes          | no                | yes, with work     |
-| `shared_ptr<>`       | yes      | yes          | yes    | yes          | shallowly         | no                 |
-| `Indirection<>`      | no       | n/a          | yes    | yes          | optionally deeply | yes, with work     |
-| `CountedReference<>` | yes      | yes          | yes    | yes          | shallowly         | no                 |
+| indirection           | nullable | default null | owning | reassignable | copyable          | undefined type ok? |
+| -----------           | -------- | ------------ | ------ | ------------ | --------          | ------------------ |
+| `*p`                  | yes      | no           | no     | yes          | shallowly         | yes                |
+| `&r`                  | no       | n/a          | no     | no           | shallowly         | yes                |
+| `&&r`                 | no       | n/a          | yes    | no           | shallowly         | yes                |
+| `reference_wrapper<>` | no       | n/a          | no     | yes          | shallowly         | yes                |
+| `Reference<>`         | no       | n/a          | no     | yes          | shallowly         | yes                |
+| `unique_ptr<>`        | yes      | yes          | yes    | yes          | no                | yes, with work     |
+| `shared_ptr<>`        | yes      | yes          | yes    | yes          | shallowly         | no                 |
+| `Indirection<>`       | no       | n/a          | yes    | yes          | optionally deeply | yes, with work     |
+| `CountedReference<>`  | yes      | yes          | yes    | yes          | shallowly         | no                 |
 
 ### Overall design preferences
 Don't use dynamic solutions to solve problems that can be solved at

flang/lib/common/reference.h

@@ -0,0 +1,62 @@
+// Copyright (c) 2019, NVIDIA CORPORATION.  All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Implements a better std::reference_wrapper<> template class with
+// move semantics, equality testing, and member access.
+// Use Reference<A> in place of a real A& reference when assignability is
+// required; safer than a bare pointer because it's guaranteed to not be null.
+
+#ifndef FORTRAN_COMMON_REFERENCE_H_
+#define FORTRAN_COMMON_REFERENCE_H_
+#include <type_traits>
+namespace Fortran::common {
+template<typename A> class Reference {
+public:
+  using type = A;
+  Reference(type &x) : p_{&x} {}
+  Reference(const Reference &that) : p_{that.p_} {}
+  Reference(Reference &&that) : p_{that.p_} {}
+  Reference &operator=(const Reference &that) {
+    p_ = that.p_;
+    return *this;
+  }
+  Reference &operator=(Reference &&that) {
+    p_ = that.p_;
+    return *this;
+  }
+
+  // Implicit conversions to references are supported only for
+  // const-qualified types in order to avoid any pernicious
+  // creation of a temporary copy in cases like:
+  //   Reference<type> ref;
+  //   const Type &x{ref};  // creates ref to temp copy!
+  operator std::conditional_t<std::is_const_v<type>, type &, void>() const
+      noexcept {
+    if constexpr (std::is_const_v<type>) {
+      return *p_;
+    }
+  }
+
+  type &get() const noexcept { return *p_; }
+  type *operator->() const { return p_; }
+  type &operator*() const { return *p_; }
+  bool operator==(Reference that) const { return *p_ == *that.p_; }
+  bool operator!=(Reference that) const { return *p_ != *that.p_; }
+
+private:
+  type *p_;  // never null
+};
+template<typename A> Reference(A &)->Reference<A>;
+}
+#endif

flang/lib/common/unwrap.h

@@ -17,6 +17,7 @@
 
 #include "indirection.h"
 #include "reference-counted.h"
+#include "reference.h"
 #include <memory>
 #include <optional>
 #include <type_traits>
@@ -126,6 +127,11 @@ struct UnwrapperHelper {
         [](const auto &x) -> std::add_const_t<A> * { return Unwrap<A>(x); }, u);
   }
 
+  template<typename A, typename B>
+  static auto Unwrap(const Reference<B> &ref) -> Constify<A, B> * {
+    return Unwrap<A>(*ref);
+  }
+
   template<typename A, typename B, bool COPY>
   static auto Unwrap(const Indirection<B, COPY> &p) -> Constify<A, B> * {
     return Unwrap<A>(*p);

flang/lib/evaluate/call.cc

@@ -27,8 +27,8 @@ ActualArgument::ActualArgument(common::CopyableIndirection<Expr<SomeType>> &&v)
 ActualArgument::ActualArgument(AssumedType x) : u_{x} {}
 ActualArgument::~ActualArgument() {}
 
-ActualArgument::AssumedType::AssumedType(const semantics::Symbol &symbol)
-  : symbol_{&symbol} {
+ActualArgument::AssumedType::AssumedType(const Symbol &symbol)
+  : symbol_{symbol} {
   const semantics::DeclTypeSpec *type{symbol.GetType()};
   CHECK(
       type != nullptr && type->category() == semantics::DeclTypeSpec::TypeStar);
@@ -110,7 +110,7 @@ int ProcedureDesignator::Rank() const {
   return 0;
 }
 
-const semantics::Symbol *ProcedureDesignator::GetInterfaceSymbol() const {
+const Symbol *ProcedureDesignator::GetInterfaceSymbol() const {
   if (const Symbol * symbol{GetSymbol()}) {
     if (const auto *details{
             symbol->detailsIf<semantics::ProcEntityDetails>()}) {
@@ -149,7 +149,7 @@ const Component *ProcedureDesignator::GetComponent() const {
 const Symbol *ProcedureDesignator::GetSymbol() const {
   return std::visit(
       common::visitors{
-          [](const Symbol *sym) { return sym; },
+          [](SymbolRef symbol) { return &*symbol; },
           [](const common::CopyableIndirection<Component> &c) {
             return &c.value().GetLastSymbol();
           },
@@ -162,7 +162,7 @@ std::string ProcedureDesignator::GetName() const {
   return std::visit(
       common::visitors{
           [](const SpecificIntrinsic &i) { return i.name; },
-          [](const Symbol *sym) { return sym->name().ToString(); },
+          [](const Symbol &symbol) { return symbol.name().ToString(); },
           [](const common::CopyableIndirection<Component> &c) {
             return c.value().GetLastSymbol().name().ToString();
           },

flang/lib/evaluate/call.h

@@ -20,6 +20,7 @@
 #include "formatting.h"
 #include "type.h"
 #include "../common/indirection.h"
+#include "../common/reference.h"
 #include "../parser/char-block.h"
 #include "../semantics/attr.h"
 #include <optional>
@@ -48,24 +49,27 @@ extern template class Fortran::common::Indirection<
 
 namespace Fortran::evaluate {
 
+using semantics::Symbol;
+using SymbolRef = common::Reference<const Symbol>;
+
 class ActualArgument {
 public:
   // Dummy arguments that are TYPE(*) can be forwarded as actual arguments.
   // Since that's the only thing one may do with them in Fortran, they're
   // represented in expressions as a special case of an actual argument.
   class AssumedType {
   public:
-    explicit AssumedType(const semantics::Symbol &);
+    explicit AssumedType(const Symbol &);
     DEFAULT_CONSTRUCTORS_AND_ASSIGNMENTS(AssumedType)
-    const semantics::Symbol &symbol() const { return *symbol_; }
+    const Symbol &symbol() const { return symbol_; }
     int Rank() const;
     bool operator==(const AssumedType &that) const {
-      return symbol_ == that.symbol_;
+      return &*symbol_ == &*that.symbol_;
     }
     std::ostream &AsFortran(std::ostream &) const;
 
   private:
-    const semantics::Symbol *symbol_;
+    SymbolRef symbol_;
   };
 
   explicit ActualArgument(Expr<SomeType> &&);
@@ -91,7 +95,7 @@ class ActualArgument {
     }
   }
 
-  const semantics::Symbol *GetAssumedTypeDummy() const {
+  const Symbol *GetAssumedTypeDummy() const {
     if (const AssumedType * aType{std::get_if<AssumedType>(&u_)}) {
       return &aType->symbol();
     } else {
@@ -141,17 +145,17 @@ struct SpecificIntrinsic {
 struct ProcedureDesignator {
   EVALUATE_UNION_CLASS_BOILERPLATE(ProcedureDesignator)
   explicit ProcedureDesignator(SpecificIntrinsic &&i) : u{std::move(i)} {}
-  explicit ProcedureDesignator(const semantics::Symbol &n) : u{&n} {}
+  explicit ProcedureDesignator(const Symbol &n) : u{n} {}
   explicit ProcedureDesignator(Component &&);
 
   // Exactly one of these will return a non-null pointer.
   const SpecificIntrinsic *GetSpecificIntrinsic() const;
-  const semantics::Symbol *GetSymbol() const;  // symbol or component symbol
+  const Symbol *GetSymbol() const;  // symbol or component symbol
 
   // Always null if the procedure is intrinsic.
   const Component *GetComponent() const;
 
-  const semantics::Symbol *GetInterfaceSymbol() const;
+  const Symbol *GetInterfaceSymbol() const;
 
   std::string GetName() const;
   std::optional<DynamicType> GetType() const;
@@ -161,7 +165,7 @@ struct ProcedureDesignator {
   std::ostream &AsFortran(std::ostream &) const;
 
   // TODO: When calling X%F, pass X as PASS argument unless NOPASS
-  std::variant<SpecificIntrinsic, const semantics::Symbol *,
+  std::variant<SpecificIntrinsic, SymbolRef,
       common::CopyableIndirection<Component>>
       u;
 };

flang/lib/evaluate/constant.h

@@ -18,12 +18,20 @@
 #include "formatting.h"
 #include "type.h"
 #include "../common/default-kinds.h"
+#include "../common/reference.h"
 #include <map>
 #include <ostream>
 #include <vector>
 
+namespace Fortran::semantics {
+class Symbol;
+}
+
 namespace Fortran::evaluate {
 
+using semantics::Symbol;
+using SymbolRef = common::Reference<const Symbol>;
+
 // Wraps a constant value in a class templated by its resolved type.
 // This Constant<> template class should be instantiated only for
 // concrete intrinsic types and SomeDerived.  There is no instance
@@ -191,8 +199,8 @@ class Constant<Type<TypeCategory::Character, KIND>> : public ConstantBounds {
 };
 
 class StructureConstructor;
-using StructureConstructorValues = std::map<const semantics::Symbol *,
-    common::CopyableIndirection<Expr<SomeType>>>;
+using StructureConstructorValues =
+    std::map<SymbolRef, common::CopyableIndirection<Expr<SomeType>>>;
 
 template<>
 class Constant<SomeDerived>

flang/lib/evaluate/expression.cc

@@ -155,7 +155,7 @@ bool StructureConstructor::operator==(const StructureConstructor &that) const {
 DynamicType StructureConstructor::GetType() const { return result_.GetType(); }
 
 const Expr<SomeType> *StructureConstructor::Find(
-    const Symbol *component) const {
+    const Symbol &component) const {
   if (auto iter{values_.find(component)}; iter != values_.end()) {
     return &iter->second.value();
   } else {
@@ -165,7 +165,7 @@ const Expr<SomeType> *StructureConstructor::Find(
 
 StructureConstructor &StructureConstructor::Add(
     const Symbol &symbol, Expr<SomeType> &&expr) {
-  values_.emplace(&symbol, std::move(expr));
+  values_.emplace(symbol, std::move(expr));
   return *this;
 }
 

flang/lib/evaluate/expression.h

@@ -756,7 +756,7 @@ class StructureConstructor {
     return values_.end();
   }
 
-  const Expr<SomeType> *Find(const Symbol *) const;  // can return null
+  const Expr<SomeType> *Find(const Symbol &) const;  // can return null
 
   StructureConstructor &Add(const semantics::Symbol &, Expr<SomeType> &&);
   int Rank() const { return 0; }