[flang] Clean up some optional<bool> usage (#161925)

Audit the use of std::optional<bool> as a tri-state logical value in
flang, correct a couple cases that need ".value_or()", add some explicit
".has_value()" calls, and document the possible pitfalls.

Author: klausler

flang/docs/C++17.md

@@ -153,3 +153,8 @@ signifies a successful recognition and absence denotes a failed parse.
 It is used in data structures in place of nullable pointers to
 avoid indirection as well as the possible confusion over whether a pointer
 is allowed to be null.
+
+`std::optional<bool>` is commonly used to denote a "tri-state"
+logical value that might be unknown.
+Please try to avoid implicit casts of `std::optional<bool>` to `bool`,
+and use `.value_or(default)` or `.has_value()` instead as appropriate.

flang/docs/C++style.md

@@ -205,11 +205,13 @@ contents, and it is assumed that the contents are present, validate that
 assumption by using `x.value()` instead.
 1. We use `c_str()` rather than `data()` when converting a `std::string`
 to a `const char *` when the result is expected to be NUL-terminated.
-1. Avoid explicit comparisions of pointers to `nullptr` and tests of
+1. Avoid explicit comparisons of pointers to `nullptr` and tests of
 presence of `optional<>` values with `.has_value()` in the predicate
 expressions of control flow statements, but prefer them to implicit
 conversions to `bool` when initializing `bool` variables and arguments,
 and to the use of the idiom `!!`.
+(But please use `.has_value()` or `.value_or()` with `optional<bool>`
+to avoid a common pitfall or the appearance of having fallen into it.)
 
 #### Classes
 1. Define POD structures with `struct`.

flang/include/flang/Semantics/type.h

@@ -120,7 +120,9 @@ class ParamValue {
   bool IsEquivalentInInterface(const ParamValue &that) const {
     return (category_ == that.category_ &&
         expr_.has_value() == that.expr_.has_value() &&
-        (!expr_ || evaluate::AreEquivalentInInterface(*expr_, *that.expr_)));
+        (!expr_ ||
+            evaluate::AreEquivalentInInterface(*expr_, *that.expr_)
+                .value_or(false)));
   }
   std::string AsFortran() const;
 

flang/lib/Evaluate/check-expression.cpp

@@ -1304,10 +1304,12 @@ std::optional<bool> IsContiguous(const A &x, FoldingContext &context,
 std::optional<bool> IsContiguous(const ActualArgument &actual,
     FoldingContext &fc, bool namedConstantSectionsAreContiguous,
     bool firstDimensionStride1) {
-  auto *expr{actual.UnwrapExpr()};
-  return expr &&
-      IsContiguous(
-          *expr, fc, namedConstantSectionsAreContiguous, firstDimensionStride1);
+  if (auto *expr{actual.UnwrapExpr()}) {
+    return IsContiguous(
+        *expr, fc, namedConstantSectionsAreContiguous, firstDimensionStride1);
+  } else {
+    return std::nullopt;
+  }
 }
 
 template std::optional<bool> IsContiguous(const Expr<SomeType> &,

flang/lib/Evaluate/fold-logical.cpp

@@ -799,22 +799,20 @@ Expr<Type<TypeCategory::Logical, KIND>> FoldIntrinsicFunction(
     }
   } else if (name == "is_contiguous") {
     if (args.at(0)) {
-      auto warnContiguous{[&]() {
-        if (auto source{args[0]->sourceLocation()}) {
-          context.Warn(common::UsageWarning::ConstantIsContiguous, *source,
-              "is_contiguous() is always true for named constants and subobjects of named constants"_warn_en_US);
-        }
-      }};
+      std::optional<bool> knownContiguous;
       if (auto *expr{args[0]->UnwrapExpr()}) {
-        if (auto contiguous{IsContiguous(*expr, context)}) {
-          warnContiguous();
-          return Expr<T>{*contiguous};
-        }
+        knownContiguous = IsContiguous(*expr, context);
       } else if (auto *assumedType{args[0]->GetAssumedTypeDummy()}) {
-        if (auto contiguous{IsContiguous(*assumedType, context)}) {
-          warnContiguous();
-          return Expr<T>{*contiguous};
+        knownContiguous = IsContiguous(*assumedType, context);
+      }
+      if (knownContiguous) {
+        if (*knownContiguous) {
+          if (auto source{args[0]->sourceLocation()}) {
+            context.Warn(common::UsageWarning::ConstantIsContiguous, *source,
+                "is_contiguous() is always true for named constants and subobjects of named constants"_warn_en_US);
+          }
         }
+        return Expr<T>{*knownContiguous};
       }
     }
   } else if (name == "is_iostat_end") {

flang/lib/Semantics/check-call.cpp

@@ -185,7 +185,8 @@ static void CheckCharacterActual(evaluate::Expr<evaluate::SomeType> &actual,
                 } else if (static_cast<std::size_t>(actualOffset->offset()) >=
                         actualOffset->symbol().size() ||
                     !evaluate::IsContiguous(
-                        actualOffset->symbol(), foldingContext)) {
+                        actualOffset->symbol(), foldingContext)
+                        .value_or(false)) {
                   // If substring, take rest of substring
                   if (*actualLength > 0) {
                     actualChars -=
@@ -598,7 +599,8 @@ static void CheckExplicitDataArg(const characteristics::DummyDataObject &dummy,
             context.IsEnabled(
                 common::LanguageFeature::ContiguousOkForSeqAssociation) &&
             actualLastSymbol &&
-            evaluate::IsContiguous(*actualLastSymbol, foldingContext)};
+            evaluate::IsContiguous(*actualLastSymbol, foldingContext)
+                .value_or(false)};
         if (actualIsArrayElement && actualLastSymbol &&
             !dummy.ignoreTKR.test(common::IgnoreTKR::Contiguous)) {
           if (IsPointer(*actualLastSymbol)) {
@@ -663,7 +665,8 @@ static void CheckExplicitDataArg(const characteristics::DummyDataObject &dummy,
               } else if (static_cast<std::size_t>(actualOffset->offset()) >=
                       actualOffset->symbol().size() ||
                   !evaluate::IsContiguous(
-                      actualOffset->symbol(), foldingContext)) {
+                      actualOffset->symbol(), foldingContext)
+                      .value_or(false)) {
                 actualElements = 1;
               } else if (auto actualSymType{evaluate::DynamicType::From(
                              actualOffset->symbol())}) {
@@ -1566,10 +1569,10 @@ static bool CheckElementalConformance(parser::ContextualMessages &messages,
                 ") corresponding to dummy argument #" + std::to_string(index) +
                 " ('" + dummy.name + "')"};
             if (shape) {
-              auto tristate{evaluate::CheckConformance(messages, *shape,
-                  *argShape, evaluate::CheckConformanceFlags::None,
-                  shapeName.c_str(), argName.c_str())};
-              if (tristate && !*tristate) {
+              if (!evaluate::CheckConformance(messages, *shape, *argShape,
+                      evaluate::CheckConformanceFlags::None, shapeName.c_str(),
+                      argName.c_str())
+                      .value_or(true)) {
                 return false;
               }
             } else {

flang/lib/Semantics/check-declarations.cpp

@@ -1984,9 +1984,8 @@ bool CheckHelper::CheckDistinguishableFinals(const Symbol &f1,
   const Procedure *p1{Characterize(f1)};
   const Procedure *p2{Characterize(f2)};
   if (p1 && p2) {
-    std::optional<bool> areDistinct{characteristics::Distinguishable(
-        context_.languageFeatures(), *p1, *p2)};
-    if (areDistinct.value_or(false)) {
+    if (characteristics::Distinguishable(context_.languageFeatures(), *p1, *p2)
+            .value_or(false)) {
       return true;
     }
     if (auto *msg{messages_.Say(f1Name,

flang/lib/Semantics/check-omp-structure.cpp

@@ -5085,7 +5085,7 @@ void OmpStructureChecker::CheckWorkdistributeBlockStmts(
 }
 
 void OmpStructureChecker::CheckIfContiguous(const parser::OmpObject &object) {
-  if (auto contig{IsContiguous(context_, object)}; contig && !*contig) {
+  if (!IsContiguous(context_, object).value_or(true)) { // known discontiguous
     const parser::Name *name{GetObjectName(object)};
     assert(name && "Expecting name component");
     context_.Say(name->source,

flang/lib/Semantics/expression.cpp

@@ -2329,7 +2329,7 @@ MaybeExpr ExpressionAnalyzer::CheckStructureConstructor(
               auto checked{CheckConformance(messages, *componentShape,
                   *valueShape, CheckConformanceFlags::RightIsExpandableDeferred,
                   "component", "value")};
-              if (checked && *checked && GetRank(*componentShape) > 0 &&
+              if (checked.value_or(false) && GetRank(*componentShape) > 0 &&
                   GetRank(*valueShape) == 0 &&
                   (IsDeferredShape(*symbol) ||
                       !IsExpandableScalar(*converted, foldingContext,