[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.


================
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.
----------------
peter.smith wrote:
> 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`  
If I were a newcomer to this code I think I'd find it easier to grep for multilib.yaml so on that basis I'd prefer to stick with the current wording. Should another file format be adopted then the comment could be updated at that point.


================
Comment at: clang/include/clang/Driver/Multilib.h:140
+
+  bool parse(llvm::MemoryBufferRef, llvm::SourceMgr::DiagHandlerTy = nullptr,
+             void *DiagHandlerCtxt = nullptr);
----------------
peter.smith wrote:
> 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.
Agreed. Renamed to `parseYaml`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932