[PATCH] D78424: [flang] Added Semantic Checks for 2 Data constraints and fixed the semantic errors in 3 test cases

[Anchu Rajendran S via Phabricator via llvm-commits]

anchu-rajendran marked 2 inline comments as done.
anchu-rajendran added inline comments.


================
Comment at: flang/lib/Semantics/check-data.cpp:73
+            "Rightmost data object pointer '%s' must not be subscripted"_err_en_US,
+            lastSymbol.name().ToString());
+      }
----------------
tskeith wrote:
> Do you need to continue after an error is reported or can you just return false? The latter would simplify the code.
I have returned false than reporting the errors. Thank you!


================
Comment at: flang/lib/Semantics/check-data.cpp:107
+    DataVarChecker subscriptChecker{context_, source_};
+    subscriptChecker.RestrictPointer();
     return std::visit(
----------------
tskeith wrote:
> Why do you need a new DataVarChecker here rather than continuing to use the existing one?
Subscripts can have any kind of `DataRef`. If a subscript is a%b%c(i), i think it is better to have tests on this `DataRef` separately so functions like `CheckFirstSymbol` will be called on `a` and say if `a` is a dummy argument, it reports an error. 

For eg., consider `a%b(c%d)` is the data object being analyzed, where `b` is an array. In this case,  when execution reaches checks on subscript, `this.isFirstSymbolChecked_` will be `true` and `this.hasSubscript_` will be `true`. For any checks that need to be done on subscript , I think it is better to set these variables for each subscript separately as it can mark the beginning of a `DataRef`. However I cannot find an use case where this checker will be used to report more errors (as the subscript needs to be a constant). I shall remove this part of code if it not expected to catch more semantic errors.


================
Comment at: flang/lib/Semantics/tools.cpp:752
+bool IsAutomaticObject(const Symbol &symbol) {
+  if (symbol.IsDummy()) {
+    return false;
----------------
klausler wrote:
> What about pointers and allocatables and functions and statement functions and construct entities and probably other types of symbol?  I think your function can return `true` for them, and that seems wrong if the symbol isn't a local object.
If it is a dummy, the function returns `false`. The only cases it returns true are (i) if length of a character is given by a non-constant expression (ii) the arrays with atleast one of the bounds is a non-constant expression. Else, the conservative value returned is `false`. Standard defines automatic object as 
"nondummy data object with a type parameter or array bound that depends on the value of a specification-expr that is not a constant expression " 
As kind parameter is always a constant expression, i did checks on only the length parameter.

For the cases you pointed out, I did check for pointers & allocatables and I saw it returns false. @klausler, Can you hint me an example code  that would make this not behave as expected?


================
Comment at: flang/test/Semantics/data04.f90:42
+      !ERROR: Data object part 'b' must not be an automatic object
+      DATA b(0) /1/
+      f = i *1024
----------------
klausler wrote:
> Please extend these tests to include cases where the dummy argument (or whatever) is not the whole designator; add subscripts, substring references, components, &c.
lines 88, 116, 119, 140 in the same file cover the cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78424