[PATCH] D142932: Multilib YAML parsing

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left some comments that can be applied prior to commit if you want to take them up.



================
Comment at: clang/include/clang/Driver/Multilib.h:66
+  /// options and look similar to them, and others can be defined by a
+  /// particular multilib.yaml. A multilib is considered compatible if its tags
+  /// are a subset of the tags derived from the Clang command line options.
----------------
With the context of https://discourse.llvm.org/t/rfc-multilib/67494 and potential other formats. I think it will be worth making this comment not specific to multilib.yaml. For example if there is another non-YAML DSL that has tags.

For example `multilib DSL` rather than `multilib.yaml`  


================
Comment at: clang/include/clang/Driver/Multilib.h:140
+
+  bool parse(llvm::MemoryBufferRef, llvm::SourceMgr::DiagHandlerTy = nullptr,
+             void *DiagHandlerCtxt = nullptr);
----------------
With reference to https://discourse.llvm.org/t/rfc-multilib/67494 perhaps rename this to `parseMultilibYaml` as parse is generic, yet there is no indication it is working on a yaml file here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932