[PATCH] D79851: [Flang] Semantics for SELECT TYPE

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi marked 12 inline comments as done and an inline comment as not done.
sameeranjoshi added a comment.
sameeranjoshi added inline comments.


================
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{
----------------
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?


================
Comment at: flang/lib/Semantics/check-select-type.cpp:119
+              if (const semantics::DerivedTypeSpec *
+                  derived{x.derivedTypeSpec}) {
+                return ChecksOnDerivedTypeSpec(
----------------
Given that derived is checked for null value, I will go with reference.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:139
+  }
+  struct TypeCase {
+    explicit TypeCase(const parser::Statement<parser::TypeGuardStmt> &s,
----------------
klausler wrote:
> Is `struct TypeCase` nested in `class TypeCaseValues`?
Yes, the similar way select-case has `struct Case` inside `class CaseValues`.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:178
+      std::string result;
+      llvm::raw_string_ostream bs{result};
+      if (this->guardType()) {
----------------
klausler wrote:
> You're just coping a single string to a stream to get the same string back.
Why is it used in select case?
Does `AsFortran` in `evaluate::Constant`  template it do something more ?


================
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);
----------------
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"


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5103
+                   *association.selector.expr)) { // C1158
+      Say(association.selector.source,
+          "Neither 'associate-name =>' "
----------------
klausler wrote:
> This error message is unclear.  Explain why the expression cannot be used with an associate-name.
Probably because selector has a vector subscript.
`Neither 'associate-name =>' nor any subobject must appear in selector, if it has a vector subscript"

looks good?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79851/new/

https://reviews.llvm.org/D79851