This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP][NFC][2/2] Reorder OmpStructureChecker and simplify it.
ClosedPublic

Authored by sameeranjoshi on Nov 5 2020, 3:23 AM.

Details

Summary

OmpStructureChecker has too much boilerplate code in source file.

This patch:

  1. Use helpers from check-directive-structure.h and reduces the boilerplate.
  2. Use TableGen infrastructure as much as possible.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Nov 5 2020, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2020, 3:23 AM
sameeranjoshi requested review of this revision.Nov 5 2020, 3:23 AM
sameeranjoshi added a project: Restricted Project.Nov 5 2020, 3:33 AM

Thanks for moving those to TableGen and common infra. This makes lots of sense. Small comment about the list with comma.

flang/lib/Semantics/check-omp-structure.cpp
492

Why not using llvm::interleave here? There are lots of example in the LLVM code base that do the same things.

Use llvm::interleave.

clementval added inline comments.Nov 12 2020, 7:53 AM
flang/lib/Semantics/check-omp-structure.cpp
489

Can you directly assign the result of llvm::interleave to mapEnumListToString? Looks like there is an indirection here with an additional temporary.

Address review comments.

Small style comment

flang/lib/Semantics/check-omp-structure.cpp
488

Should this be lowercase commaSeperatedMapTypes? Seems like there are mixed styles here.

sameeranjoshi marked 3 inline comments as done.Nov 18 2020, 10:58 PM

Looks OK to me. Couple of minor comments.

flang/lib/Semantics/check-omp-structure.cpp
13

Nit: Should there be a line before the namespace?

489

Is commaSeperatedMapTypes created (and not used) in cases where there is no error?

flang/test/Semantics/omp-combined-constructs.f90
208

#justsaying: I liked the "or" in the message.

Thanks for taking a look at the patch @Kiran(arm).
Address review comments.

sameeranjoshi marked 3 inline comments as done.Nov 21 2020, 8:21 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
489

Yes, thanks for catching those needless allocations.
I will allocate them only when there is error case.

flang/test/Semantics/omp-combined-constructs.f90
208

I had thought initially, by passing (total-size-of-list-1) elements to llvm::interleave, and use "or [last-element-in-list]" inside the error message, but that would have probably needed a check based on the size of list and specifically for 0 size tests, also it looked too odd to reference only the last element in list, hence skipped it.

This revision is now accepted and ready to land.Nov 21 2020, 8:25 AM
This revision was landed with ongoing or failed builds.Nov 21 2020, 9:08 AM
This revision was automatically updated to reflect the committed changes.
sameeranjoshi marked 2 inline comments as done.