[PATCH] D90697: [Flang][OpenMP 4.5] Add semantic check for OpenMP Reduction Clause

[Valentin Clement via Phabricator via llvm-commits]

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


================
Comment at: flang/lib/Semantics/check-directive-structure.h:284
+}
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+bool DirectiveStructureChecker<D, C, PC,
----------------
Can you add a blank line here? The rest of the file has blank lines between functions. 


================
Comment at: flang/lib/Semantics/check-directive-structure.h:286
+bool DirectiveStructureChecker<D, C, PC,
+    ClauseEnumSize>::CheckPrivateReductionVariable(Symbol *symbol) {
+  if (!GetContext().reductionSymbols.empty()) {
----------------
Maybe I miss the point here but the function itself does not check for any private attribute on the symbol or smith like that. It just check whether the symbol is in the list of `reductionSymbol` which can be populated differently. 


================
Comment at: flang/lib/Semantics/check-directive-structure.h:296
+}
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckNoBranching(
----------------
Add a blank line here. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:407
+  const auto &definedOp{std::get<0>(x.t)};
+  // const auto &opr{std::get<parser::DefinedOperator>(definedOp.u)};
+  bool ok = false;
----------------
Should be removed? 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:412
+          [&](const parser::DefinedOperator
+                  &dOpr) { //:DefinedOpName &defName) {},
+            const auto &intrinsicOp{
----------------
Comment should be removed? 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:313
+                        if (!name.symbol) {
+                         //Resolving the symbol value for the procedure name
+                         //in reduction clause.
----------------
Indent is wrong. See clang-format message below. 


================
Comment at: flang/test/Semantics/omp-reduction08.f90:6
+program omp_reduction
+  !DEF: /omp_reduction/i ObjectEntity INTEGER(4)
+  integer i
----------------
Should you use `test_symbols.sh` if you want to check this kind of information? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697