This is an archive of the discontinued LLVM Phabricator instance.

[flang] Semantic checks for SELECT RANK
ClosedPublic

Authored by sameeranjoshi on Apr 22 2020, 3:23 AM.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Apr 22 2020, 3:23 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
DavidTruby accepted this revision.Apr 22 2020, 6:17 AM

LGTM but I'm not an expert on this code so wait for approval from someone else

flang/lib/Semantics/resolve-names.cpp
5111

Why does this need de-referencing and then referencing again?

This revision is now accepted and ready to land.Apr 22 2020, 6:17 AM
tskeith requested changes to this revision.Apr 27 2020, 1:59 PM
tskeith added inline comments.
flang/include/flang/Semantics/symbol.h
149

I think rank and rank_ are better names than associationRank and associationRank_.
You know it's the rank of an association because it is in an AssocEntityDetails, and that would be consistent with set_rank.

flang/lib/Evaluate/shape.cpp
461

Can you extract all of the copies of this code into a single function?

flang/lib/Semantics/check-select-rank.cpp
70

You could show where the previous DEFAULT is.

79

Same here.

116

I think GetExpr is a better name for this function. Nothing is being resolved.

124

You should be able to write this as:
std::visit([](const auto &x) { return GetExpr(x); }, selector.u);

Or maybe change GetExpr to operate on any union class as it does on wrapper classes.

flang/lib/Semantics/resolve-names.cpp
5147

Why do you need this Walk? Didn't it just get visited?

flang/lib/Semantics/symbol.cpp
290

I don't think this belongs in Symbol. It is only called in one place and it's not consistent with GetRank. You can get the rank of all kinds of symbols but you can set it on only one particular kind.

292

Details should be details.
Even better, use get<AssocEntityDetails>() and the check will be done automatically.

flang/test/Semantics/select-rank.f90
76

"default" -> "DEFAULT"

93

This message could say what the bad value is. If it's an expression it may not be obvious.

This revision now requires changes to proceed.Apr 27 2020, 1:59 PM
sameeranjoshi removed a subscriber: mgorny.

Review comments addressed.

Please mark comments as done when you have done something in response, and indicate what you did if it's not obvious.

flang/include/flang/Evaluate/shape.h
131–137

How about CreateShape?

flang/lib/Semantics/check-select-rank.cpp
115

Can you print *val instead of fetching and formatting expr?

tskeith requested changes to this revision.May 6 2020, 7:45 AM
This revision now requires changes to proceed.May 6 2020, 7:45 AM
sameeranjoshi marked 12 inline comments as done.
sameeranjoshi accepted this revision.May 6 2020, 6:37 PM
sameeranjoshi marked 3 inline comments as done.

Review comments addressed.

@peterklauslar @timkeith is there still anything which needs to be addressed to merge this PR?

flang/lib/Semantics/check-select-rank.cpp
115

Updated revision using *val.
The kind value vanishes when using *val. Only the rank value would be displayed.

116

Two same names were causing confusion, hence added a different name GetExprFromSelector.

sameeranjoshi marked an inline comment as done.
tskeith accepted this revision.May 6 2020, 7:22 PM
This revision is now accepted and ready to land.May 6 2020, 7:22 PM

@tskeith
Is that a green signal so that I can commit it, or do I still need to wait for some more reviewers to comment and accept?
Is @klausler ok with it?

@tskeith
Is that a green signal so that I can commit it, or do I still need to wait for some more reviewers to comment and accept?
Is @klausler ok with it?

It's approved, you can commit it. I'll clean it up here.

klausler marked an inline comment as done.May 7 2020, 12:19 PM
klausler added inline comments.
flang/lib/Semantics/resolve-names.cpp
5111

Because it's not a pointer.

DavidTruby added inline comments.May 7 2020, 12:32 PM
flang/lib/Semantics/resolve-names.cpp
5111

No.. and neither is the thing it is being assigned to? I don't understand how it not being a pointer is relevant.

klausler added inline comments.May 7 2020, 1:12 PM
flang/lib/Semantics/resolve-names.cpp
5111

name is a reference to a std::optional<parser::Name>.

*name is a reference to a parser::Name.

&*name is an rvalue pointer to a parser::Name.

association.name is an lvalue whose type is pointer to a const parser::Name.

association.name = &*name; assigns a pointer rvalue to a pointer lvalue.