[PATCH] D97964: [flang][OpenMP] Add semantic check for occurrence of multiple list items in aligned clause for simd directive

[Tim Keith via Phabricator via llvm-commits]

tskeith added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:610
+    // A list-item cannot appear in more than one aligned clause
+    std::set<std::string> alignedVars = {};
+    bool foundErr = false;
----------------
kiranchandramohan wrote:
> I think alignedVars{} is preferred.
The default initialization for a set is empty, so just `std::set<std::string> alignedVars;` is best.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:619
+      for (auto const &var : alignedNameList) {
+        if (alignedVars.count(var.ToString()) == 1) {
+          context_.Say(
----------------
kiranchandramohan wrote:
> Would it be better use the symbol corresponding to the name? And use a SymbolSet to store the symbols associated with the variables?
> 
> @tskeith In general what is preferable, comparing names of variables or symbols associated with them?
You can have different symbols with the same name, so these aren't equivalent. In this case, I agree that SymbolSet seems appropriate.

Even when you want a set of names, it's better to save them as `parser::CharBlock` rather than making new `std::string` objects.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:625
+          foundErr = true;
+          break;
+        }
----------------
Why not just return here to simplify the logic?


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

https://reviews.llvm.org/D97964