Page MenuHomePhabricator

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

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



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.Thu, Nov 5, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 5, 3:23 AM
sameeranjoshi requested review of this revision.Thu, Nov 5, 3:23 AM
sameeranjoshi added a project: Restricted Project.Thu, Nov 5, 3:33 AM

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


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.Thu, Nov 12, 7:53 AM

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


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

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

Looks OK to me. Couple of minor comments.


Nit: Should there be a line before the namespace?


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


#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.Sat, Nov 21, 8:21 AM
sameeranjoshi added inline comments.

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


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.Sat, Nov 21, 8:25 AM
This revision was landed with ongoing or failed builds.Sat, Nov 21, 9:08 AM
This revision was automatically updated to reflect the committed changes.
sameeranjoshi marked 2 inline comments as done.