[flang] Clean up some optional<bool> usage #161925
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-openmp Author: Peter Klausler (klausler) ChangesAudit 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:
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,
|
eugeneepshteyn
approved these changes
Oct 5, 2025
DharuniRAcharya
pushed a commit
to DharuniRAcharya/llvm-project
that referenced
this pull request
Oct 13, 2025
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Audit the use of std::optional 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.