[PATCH] D83363: [openmp] Use switch in isAllowedClauseForDirective instead of multiple if

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: llvm/test/TableGen/directive1.td:120
+// IMPL-NEXT:        }
+// IMPL-NEXT:      } break;
+// IMPL-NEXT:      default:
----------------
In most examples I've seen, the outer `case` would be formatted as:

```
case TDLD_dira: {
  . . .
  break;
}
```

clang-format puts the `{` on the same line as the `case`.

grep shows some code putting `break;` on the same line as the `}` and some code putting it on the line before.  However, I did at least find [[ http://llvm.org/docs/WritingAnLLVMBackend.html#instruction-selector | one example in LLVM docs ]] showing the way I'm suggesting above.

Alternatively, [[ http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return | as in this example ]], couldn't those braces be dropped given that there are no local declarations?


================
Comment at: llvm/test/TableGen/directive1.td:124
+// IMPL-NEXT:    }
+// IMPL-NEXT:    llvm_unreachable("Invalid Tdl Directive kind");
 // IMPL-NEXT:  }
----------------
The unreachable message doesn't make sense given the `default` in the directive switch.  If that switch covers all directives, `default` isn't needed anyway.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:255
     const auto &RequiredClauses = D->getValueAsListOfDefs("requiredClauses");
-    GenerateTestForAllowedClauses(RequiredClauses, OS, DirectiveName,
+    GenerateCaseForAllowedClauses(RequiredClauses, OS, DirectiveName,
                                   DirectivePrefix, ClausePrefix);
----------------
Maybe `GenerateCaseForVersionedClauses` given that it's not just `allowedClauses`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83363