[flang-commits] [flang] [flang] Clean up some optional<bool> usage (PR #161925)
via flang-commits
flang-commits at lists.llvm.org
Fri Oct 3 16:05:03 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-openmp
Author: Peter Klausler (klausler)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/161925.diff
9 Files Affected:
- (modified) flang/docs/C++17.md (+5)
- (modified) flang/docs/C++style.md (+3-1)
- (modified) flang/include/flang/Semantics/type.h (+3-1)
- (modified) flang/lib/Evaluate/check-expression.cpp (+6-4)
- (modified) flang/lib/Evaluate/fold-logical.cpp (+11-13)
- (modified) flang/lib/Semantics/check-call.cpp (+10-7)
- (modified) flang/lib/Semantics/check-declarations.cpp (+2-3)
- (modified) flang/lib/Semantics/check-omp-structure.cpp (+1-1)
- (modified) flang/lib/Semantics/expression.cpp (+1-1)
``````````diff
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<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.
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<bool>`
+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<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> &,
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<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") {
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<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 {
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<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,
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,
``````````
</details>
https://github.com/llvm/llvm-project/pull/161925
More information about the flang-commits
mailing list