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

[Valentin Clement via Phabricator via llvm-commits]

clementval added a comment.

Thanks for the review. I just updated the patch.



================
Comment at: llvm/test/TableGen/directive1.td:120
+// IMPL-NEXT:        }
+// IMPL-NEXT:      } break;
+// IMPL-NEXT:      default:
----------------
jdenny wrote:
> 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?
I guess we can drop the braces and then it's clear where the `break;` is going. 


================
Comment at: llvm/test/TableGen/directive1.td:124
+// IMPL-NEXT:    }
+// IMPL-NEXT:    llvm_unreachable("Invalid Tdl Directive kind");
 // IMPL-NEXT:  }
----------------
jdenny wrote:
> 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.
Will remove it.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:255
     const auto &RequiredClauses = D->getValueAsListOfDefs("requiredClauses");
-    GenerateTestForAllowedClauses(RequiredClauses, OS, DirectiveName,
+    GenerateCaseForAllowedClauses(RequiredClauses, OS, DirectiveName,
                                   DirectivePrefix, ClausePrefix);
----------------
jdenny wrote:
> Maybe `GenerateCaseForVersionedClauses` given that it's not just `allowedClauses`?
Yeah might be better and less confusing in the future. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83363