Page MenuHomePhabricator

[flang] Fix non-deterministic line output function
ClosedPublic

Authored by ijan1 on Aug 24 2021, 4:53 AM.

Details

Summary

The evaluation order for the | operator is undefined
(in contrast to the short-circuiting || operator). The arguments are
stored in variables to force a specific evaluation order.

A test in D107575 relies on this change.

Diff Detail

Event Timeline

ijan1 created this revision.Aug 24 2021, 4:53 AM
ijan1 requested review of this revision.Aug 24 2021, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2021, 4:53 AM
kiranchandramohan added a subscriber: klausler.

LGTM. Please wait for approval from @klausler or @Meinersbur.

flang/lib/Semantics/check-declarations.cpp
1155

Nit: You probably do not need "&" in the capture list since you have explicitly captured the variables (opName, specific, proc) required.

This revision is now accepted and ready to land.Aug 24 2021, 5:46 AM
kiranchandramohan edited reviewers, added: klausler; removed: clementval.Aug 24 2021, 5:48 AM
klausler added inline comments.Aug 24 2021, 9:11 AM
flang/lib/Semantics/check-declarations.cpp
1156

We use modern C++ braced initialization in the front-end, as a rule, for better error detection.

You don't need parentheses on a return statement.

Meinersbur added inline comments.Aug 24 2021, 9:29 AM
flang/lib/Semantics/check-declarations.cpp
1155

specific and proc should be captured by-reference, at they are in the function arguments.

Even better, I'd convert all captures into lambda arguments, such that one can see at the call site what data the check depends on.

Meinersbur edited the summary of this revision. (Show Details)Aug 24 2021, 9:32 AM
Meinersbur added inline comments.Aug 24 2021, 9:34 AM
flang/lib/Semantics/check-declarations.cpp
1158

With both argument having been evaluated, it is now possible to use the logical or (both arguments are bool).

ijan1 updated this revision to Diff 368631.Aug 25 2021, 7:11 AM

Updating to use braced initialisation, removed parentheses in the return statement and converted the captures into lambda parameters.

Please use braced initialization for the lambda as well.

Meinersbur added inline comments.Aug 26 2021, 12:26 PM
flang/lib/Semantics/check-declarations.cpp
1156

Nothing is implicitly captured.

ijan1 updated this revision to Diff 369876.Sep 1 2021, 1:01 AM
ijan1 marked 2 inline comments as done.

Using braced initialisation for the lambda.

klausler accepted this revision.Sep 1 2021, 8:55 AM

Looks great to me now; thank you for the patch.

This revision was automatically updated to reflect the committed changes.
ijan1 marked an inline comment as done.
ijan1 added a comment.Sep 2 2021, 12:36 PM

Commenting so that my inline comment shows.

flang/lib/Semantics/check-declarations.cpp
1156

I thought I had left a comment but it seems like it was unsubmitted. The this pointer is being captured here.

Meinersbur added inline comments.Sep 4 2021, 4:48 PM
flang/lib/Semantics/check-declarations.cpp
1156

OK. Could have used [this], it is always captured by value even with [&]. No need to change after it having been committed.