diff --git a/flang/docs/C++17.md b/flang/docs/C++17.md index b20364b03e060..f36110af86847 100644 --- a/flang/docs/C++17.md +++ b/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` is commonly used to denote a "tri-state" +logical value that might be unknown. +Please try to avoid implicit casts of `std::optional` to `bool`, +and use `.value_or(default)` or `.has_value()` instead as appropriate. diff --git a/flang/docs/C++style.md b/flang/docs/C++style.md index 04579130aa7cb..cbb96f15eb5f1 100644 --- a/flang/docs/C++style.md +++ b/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` +to avoid a common pitfall or the appearance of having fallen into it.) #### Classes 1. Define POD structures with `struct`. diff --git a/flang/include/flang/Semantics/type.h b/flang/include/flang/Semantics/type.h index 3bd638b89053d..87583a088fd3e 100644 --- a/flang/include/flang/Semantics/type.h +++ b/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; diff --git a/flang/lib/Evaluate/check-expression.cpp b/flang/lib/Evaluate/check-expression.cpp index 8931cbe485ac2..dda7427c46223 100644 --- a/flang/lib/Evaluate/check-expression.cpp +++ b/flang/lib/Evaluate/check-expression.cpp @@ -1298,10 +1298,12 @@ std::optional IsContiguous(const A &x, FoldingContext &context, std::optional 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 IsContiguous(const Expr &, diff --git a/flang/lib/Evaluate/fold-logical.cpp b/flang/lib/Evaluate/fold-logical.cpp index 449c316802d6a..457b2f6dd20b8 100644 --- a/flang/lib/Evaluate/fold-logical.cpp +++ b/flang/lib/Evaluate/fold-logical.cpp @@ -799,22 +799,20 @@ Expr> 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 knownContiguous; if (auto *expr{args[0]->UnwrapExpr()}) { - if (auto contiguous{IsContiguous(*expr, context)}) { - warnContiguous(); - return Expr{*contiguous}; - } + knownContiguous = IsContiguous(*expr, context); } else if (auto *assumedType{args[0]->GetAssumedTypeDummy()}) { - if (auto contiguous{IsContiguous(*assumedType, context)}) { - warnContiguous(); - return Expr{*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{*knownContiguous}; } } } else if (name == "is_iostat_end") { diff --git a/flang/lib/Semantics/check-call.cpp b/flang/lib/Semantics/check-call.cpp index 81c53aaf9e339..e4d2a0d220c12 100644 --- a/flang/lib/Semantics/check-call.cpp +++ b/flang/lib/Semantics/check-call.cpp @@ -185,7 +185,8 @@ static void CheckCharacterActual(evaluate::Expr &actual, } else if (static_cast(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(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 { diff --git a/flang/lib/Semantics/check-declarations.cpp b/flang/lib/Semantics/check-declarations.cpp index 75934243b7916..ea5e2c095d31a 100644 --- a/flang/lib/Semantics/check-declarations.cpp +++ b/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 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, diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index c0c41c1eed89d..d65a89e768466 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/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, diff --git a/flang/lib/Semantics/expression.cpp b/flang/lib/Semantics/expression.cpp index fc268886c5feb..e04a3ea1bc4eb 100644 --- a/flang/lib/Semantics/expression.cpp +++ b/flang/lib/Semantics/expression.cpp @@ -2317,7 +2317,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,