[PATCH] D90241: [openmp][openacc] Check for duplicate clauses for directive

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: llvm/test/TableGen/directive3.td:30
+// CHECK: error: Clause TDLC_ClauseA already defined on directive TDL_DirA
+// CHECK: error: One or more clauses are defined multiple times on directive TDL_DirA
----------------
Nit: Is this second error giving any additional information? Is this twin errors so that we get all occurrences of errors rather than fail at the first one?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:151-162
   const auto &DirectiveLanguages =
       Records.getAllDerivedDefinitions("DirectiveLanguage");
 
   if (DirectiveLanguages.size() != 1) {
     PrintError("A single definition of DirectiveLanguage is needed.");
     return;
   }
----------------
Is this code from 151 to 162 similar for all occurrences of HasDuplicateClausesInDirectives? If so would it be better to pull it into a function and use that function?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:119
+    VersionedClause VerClause{C};
+    if (CrtClauses.find(VerClause.getClause().getName()) == CrtClauses.end()) {
+      CrtClauses.insert(VerClause.getClause().getName());
----------------
clementval wrote:
> kiranchandramohan wrote:
> > Wouldn't this lead to an O(N^2) loop. Can we do better?
> `CrtClauses` is a `StringSet` implemented on the top of `StringMap`. The lookup should be constant (O(1)) so inside the loop we should be O(N). 
Ahh, OK. Completely missed that.
Nit: Would using insert and checking the return value be better than using find and insert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90241