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

[Peter Klausler via Phabricator via llvm-commits]

klausler requested changes to this revision.
klausler added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Evaluate/check-expression.cpp:60
+  bool operator()(const Component &component) const {
+    if (IsNamedConstant(component.GetFirstSymbol())) {
+      return true;
----------------
The logic here is wrong.  Just recurse on the base (`return (*this)(component.base());`).  If the base is an array element reference with non-constant subscripts, the component reference isn't a constant.


================
Comment at: flang/lib/Semantics/check-data.cpp:61
+    const Symbol &lastSymbol{component.GetLastSymbol()};
+    if (!isRightMostComponent) {
+      if (IsPointer(lastSymbol)) { // C877
----------------
This code doesn't implement the C877 constraint correctly.  A pointer is allowed in a DATA statement object only if it is the **entire** rightmost part-ref.  Your code would accept subscripts or a substring reference that was applied to a pointer component (`A%PTR(1)(2:3)`).



================
Comment at: flang/lib/Semantics/check-data.cpp:128
   }
+  bool IsInBlankCommon(const Symbol &symbol) {
+    if (FindCommonBlockContaining(symbol)) {
----------------
This member function doesn't use any state from the class; it could be made `static`, but it would be even better if it were in lib/Semantics/tools.cpp as it will probably be useful in other cases.


================
Comment at: flang/lib/Semantics/check-data.cpp:148
   bool hasSubscript_{false};
+  bool isRightMostComponent{true};
 };
----------------
Please follow the naming conventions.


================
Comment at: flang/lib/Semantics/tools.cpp:733
 
+bool IsAutomaticArray(const Symbol &symbol) {
+  if (symbol.IsObjectArray()) {
----------------
This code would return `true` for an explicit-shape dummy argument array with non-constant bounds, yes?  Such objects are not "automatic".  I think that you need a better name for this predicate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78424