[PATCH] D84290: Fix for the scenario when type guard has intrinsic type specification and Selector is NOT unlimited Polymorphic.

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi added a comment.

In D84290#2165893 <https://reviews.llvm.org/D84290#2165893>, @inderjeet-hcl wrote:

> Additional compilation error will come in selecttype01.f90 and symbol11.f90  test cases and it is expected with the patch.   Also I am uploading patch file to add "RUN: line" in newly added test case selecttype04.f90.
>
> 1. selecttype01.f90:  Addition error will come at line 122 and 130 selecttype01.f90:122:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer) ^^^^^^^^^^^^^^^^^
>
>   selecttype01.f90:130:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer) ^^^^^^^^^^^^^^^^^
> 2. symbol11.f90:  Addition error will come at line 74. symbol11.f90:74:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer(kind=8)) ^^^^^^^^^^^^^^^^^^^^^^^^^
>
>   F12384219: testcasefix.patch <https://reviews.llvm.org/F12384219>


Can you add checks with `!ERROR:` in the respective files?
Also I never saw someone sending a `.patch` file for review separately in llvm-project, you need to include the changes with the revision itself.



================
Comment at: flang/lib/Semantics/check-select-type.cpp:84
+                    !selectorType_.IsUnlimitedPolymorphic()) { // C1162
+                  context_.Say(stmt.source,
+                      "If selector is not Unlimited Polymorphic, "
----------------
I tried to minimize the use of passing `stmt` as a parameter by using `parser::FindSourceLocation(typeSpec)` it was strange that I couldn't see the source location information printed.

Does `parser::FindSourceLocation` fail in some scenarios @PeteSteinfeld ?
If that's the case using `stmt` is completely fine here.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:85
+                  context_.Say(stmt.source,
+                      "If selector is not Unlimited Polymorphic, "
+                      "intrinsic type specification must not be specified "
----------------
Why are `u` and `p` capitalized?
Read more on error message to user guideline:
https://github.com/llvm/llvm-project/blob/master/flang/documentation/C%2B%2Bstyle.md#error-messages


================
Comment at: flang/lib/Semantics/check-select-type.cpp:100
                       *derived, parser::FindSourceLocation(typeSpec));
                 }
                 return false;
----------------
How about adding an `else if ` statement here?
```
if (){
...
} else if (!selectorType_.IsUnlimitedPolymorphic() && spec->AsIntrinsic()) {  // C1162
  ...
}
```


================
Comment at: flang/test/Semantics/selecttype04.f90:1
+type base
+ integer :: ii
----------------
There's already a [[ https://github.com/llvm/llvm-project/blob/5bb742b10dafd595223172ae985687765934ebe9/flang/test/Semantics/selecttype01.f90#L153 | file ]] with a test for C1162, can you add these tests in it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84290