[PATCH] D82067: [flang] addition of Coarray and some header files

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

I have a few comments/questions.

Could this have been better as more than one patch? Box, symbolmap in one, RTBuilder in another, and corray in another?



================
Comment at: flang/include/flang/Lower/Support/BoxValue.h:176-177
+
+/// Used for triple notation (array slices)
+using RangeBoxValue = std::tuple<mlir::Value, mlir::Value, mlir::Value>;
+
----------------
RangeBoxValue is not used.
Also, is the name a misnomer since it has nothing to do with the abstract box classes?


================
Comment at: flang/lib/Lower/Coarray.cpp:15-16
+#include "flang/Lower/Coarray.h"
+#include "RTBuilder.h"
+#include "SymbolMap.h"
+#include "flang/Lower/Bridge.h"
----------------
Are these two headers needed here?


================
Comment at: flang/lib/Lower/RTBuilder.h:28
+// List the runtime headers we want to be able to dissect
+#include "../../runtime/io-api.h"
+
----------------
should this header be in the include directory?


================
Comment at: flang/lib/Lower/SymbolMap.h:129
+        common::visitors{[&](const FullDim &box) {
+                           assert(dim < box.lbounds.size());
+                           return box.lbounds[dim];
----------------
https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/include/llvm/ADT/SmallVector.h#L149

Isn't this assert part of the SmallVector implementation?


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

https://reviews.llvm.org/D82067