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

[Inderjeet via Phabricator via llvm-commits]

inderjeet-hcl marked 4 inline comments as done.
inderjeet-hcl added a comment.

@sameeranjoshi, Kindly find reply inline and let me know if it seems fine.



================
Comment at: flang/lib/Semantics/check-select-type.cpp:84
+                    !selectorType_.IsUnlimitedPolymorphic()) { // C1162
+                  context_.Say(stmt.source,
+                      "If selector is not Unlimited Polymorphic, "
----------------
sameeranjoshi wrote:
> 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.
Kindly refer following case when line number information is not output in error message. 
In case of intrinsic types 'integer','character' etc. parser::FindSourceLocation(typespec) is returning empty:

[root at localhost Github_compiler]# f18 selecttype.f90 
error: The type specification statement must have LEN type parameter as assumed
f18: semantic errors in selecttype.f90
[root at localhost Github_compiler]# cat -n selecttype.f90 
     1	class(*),allocatable :: cptr
     2	
     3	select type(cptr)
     4	 !ERROR: The type specification statement must have LEN type parameter as assumed
     5	 type is(character) !<-- assumed length-type
     6	end select
     7	end



================
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 "
----------------
sameeranjoshi wrote:
> 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
Thanks for sharing document reference. I will update it as follows:
                  context_.Say(stmt.source,
                      "If selector is not unlimited polymorphic, "
                      "intrinsic type specification must not be specified "
                      "in type guard statement"_err_en_US);



================
Comment at: flang/lib/Semantics/check-select-type.cpp:100
                       *derived, parser::FindSourceLocation(typeSpec));
                 }
                 return false;
----------------
sameeranjoshi wrote:
> How about adding an `else if ` statement here?
> ```
> if (){
> ...
> } else if (!selectorType_.IsUnlimitedPolymorphic() && spec->AsIntrinsic()) {  // C1162
>   ...
> }
> ```
I think either way is fine. I added 'if' condition at the beginning considering  if type specification is of intrinsic type then that should be the first check to be performed.


================
Comment at: flang/test/Semantics/selecttype04.f90:1
+type base
+ integer :: ii
----------------
sameeranjoshi wrote:
> 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?
I agree, I will update test program selecttype01.f90 to add tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84290