[PATCH] D79851: [Flang] Semantics for SELECT TYPE
Pete Steinfeld via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 3 14:20:41 PDT 2020
PeteSteinfeld requested changes to this revision.
PeteSteinfeld added a comment.
Thanks for working on this. There's still more to be done here.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:1
+//===-- lib/Semantics/check-select-type.cpp -------------------------------===//
+//
----------------
Does this file need to have `clang-format` run on it?
================
Comment at: flang/lib/Semantics/check-select-type.cpp:56
+ common::visitors{
+ [&](const parser::Default &)
+ -> std::optional<evaluate::DynamicType> {
----------------
The `&` is not needed for any of these variants.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:61
+ [&](const parser::TypeSpec &typeSpec)
+ -> std::optional<evaluate::DynamicType> {
+ return evaluate::DynamicType::From(typeSpec.declTypeSpec);
----------------
Specifying the return type is not necessary for this variant.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:91
+ common::visitors{
+ [&](const parser::Default &) { return true; },
+ [&](const parser::TypeSpec &typeSpec) {
----------------
The `&` is not needed.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:97
+ context_.Say(parser::FindSourceLocation(typeSpec),
+ "The type specification statement must have LEN type parameter as assumed"_err_en_US);
+ return false;
----------------
Please make this fit into 80 columns.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:119
+ }
+ bool PassesDerivedTypeChecks(const semantics::DerivedTypeSpec &derived,
+ parser::CharBlock sourceLoc) const {
----------------
Other methods are preceded by a blank line. Please insert one for consistency.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:124
+ context_.Say(sourceLoc,
+ "The type specification statement must have LEN type parameter as assumed"_err_en_US);
+ return false;
----------------
Please make this fit into 80 columns.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:134
+ }
+ // TODO: C1162
+ return true;
----------------
sameeranjoshi wrote:
> klausler wrote:
> > C1162 is important; please implement it. There can't be TYPE IS or CLASS IS cases in a SELECT TYPE that are not possible for the selector.
> I have a test already added in selecttype01.f90 in subroutine CheckC1162.
> If @PeteSteinfeld @klausler that LG, I think `IsExtensibleType` when passed with `parentDerived` would help in code implementation?
>
> Can we extract the ancestor chain of a particular type as well as the types which derive from it?
>
> Can you point some test file if present or the code file which could be already handling this?
>
I don't understand your first question about "code implementation".
In resolve-names.cpp, there's a function called `FindSymbol()` that searches though a chain of extended derived types. Also, take a look at the `Post()` function for `parser::DerivedTypeStmt`. It processes the `EXTENDS` clause of a derived type definition.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:206
+
+ // This has quadratic time, only runs in non-error cases
+ void ReportConflictingTypeCases() {
----------------
Peter's comment about this is that the normal case is that there are no errors. So why have this comment at all?
================
Comment at: flang/lib/Semantics/check-select-type.cpp:236
+ const auto &unResolvedSel{std::get<parser::Selector>(selectType.t)};
+ const auto *Selector{GetExprFromSelector(unResolvedSel)};
+ if (!Selector) {
----------------
Variable names should start with a lower-case letter. Please change this to `selector`. See flang//documentation/C++style.md for guidance.
================
Comment at: flang/lib/Semantics/check-select-type.cpp:73-83
+ const semantics::Symbol &typeSymbol{
+ derivedTypeSpec->typeSymbol()};
+ if (const semantics::Scope * scope{typeSymbol.scope()}) {
+ auto &mutablederivedTypeSpec{
+ *const_cast<semantics::DerivedTypeSpec *>(
+ derivedTypeSpec)};
+ auto &mutableConstScope{
----------------
sameeranjoshi wrote:
> klausler wrote:
> > Your code code has no comments anywhere to explain what you're doing, and they would be helpful here. It looks like you're using move semantics to suck the contents out of the derived type spec and transfer them to a new derived type spec in the scope of the original derived type spec. This seems likely to wreck the representation of the original type, and I can't tell why you want to do it.
> Ok, so wanted to extract the `Evaluate::DynamicType` from the `parser::DerivedTypeSpec`.
> The only way I could figure out was,
> step1:
> convert from `semantics::DerivedTypeSpec` to `semantics::DeclTypeSpec`
> step2:
> further I knew that `evaluate::DynamicType::From()` converts `semantics::DeclTypeSpec` to `Evaluate::DynamicType`
>
> Is there a better way you can suggest?
I'm not sure what the best answer here is. But the `std::move()` seems like a dangerous thing to do. I suggest looking around the rest of the files that do semantics checking for how they extract `Evaluate::DynamicType` objects and see if you can glean some wisdom from them.
================
Comment at: flang/lib/Semantics/resolve-names.cpp:5096
+ Say(association.selector.source,
+ "Selector '%s' in SELECT TYPE statements must be "
+ "polymorphic value"_err_en_US);
----------------
sameeranjoshi wrote:
> klausler wrote:
> > s/statements/statement/
> >
> > s/ value//
> Is this what you wanna say?
> replace statements => statement
> and remove "value"
>
> new message
> "Selector '%s' in SELECT TYPE statement must be polymorphic"
This looks good to me.
================
Comment at: flang/test/Semantics/selecttype01.f90:1
+! RUN: %B/test/Semantics/test_errors.sh %s %flang %t
+! Test for checking select type constraints,
----------------
The `RUN` command has changed. It's now
```
! RUN: %S/test_errors.sh %s %t %f18
```
================
Comment at: flang/test/Semantics/selecttype02.f90:1
+! RUN: %B/test/Semantics/test_errors.sh %s %flang %t
+module m1
----------------
The RUN command has changed. It's now
```
! RUN: %S/test_errors.sh %s %t %f18
```
================
Comment at: flang/test/Semantics/selecttype03.f90:1
+! RUN: %B/test/Semantics/test_errors.sh %s %flang %t
+! Test various conditions in C1158.
----------------
The RUN command has changed. It's now
```
! RUN: %S/test_errors.sh %s %t %f18
```
================
Comment at: flang/test/Semantics/selecttype03.f90:48
+!C)Associate with with vector subscript
+ select type(b => foo(1) )
+ type is (t1)
----------------
klausler wrote:
> The comment states that the test is trying to force an error by associating a selector with a designator that has a vector subscript, but that's not what's happening in the code.
There's a comment in selecttype01.f90 that says it tests c1158. But I couldn't find a test that tests vector subscripts.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79851/new/
https://reviews.llvm.org/D79851
More information about the llvm-commits
mailing list