[PATCH] D142932: Multilib YAML parsing

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/include/clang/Driver/Multilib.h:150-152
+  bool parseYaml(llvm::MemoryBufferRef,
+                 llvm::SourceMgr::DiagHandlerTy = nullptr,
+                 void *DiagHandlerCtxt = nullptr);
----------------
Rather than `parseYaml` modifying the state of `MultilibSet`, can we instead have a static factory method or a standalone function that parses, constructs and returns `llvm::Expected<MultilibSet>`. That's more in line with how we handle YAML deserialization elsewhere in LLVM, and also follows the notion of `MultilibSet` being immutable.


================
Comment at: clang/lib/Driver/Multilib.cpp:188
+    if (StringRef(V.Dir).starts_with("/"))
+      return "paths must be relative. \"" + V.Dir + "\" starts with \"/\"\n";
+    return std::string{};
----------------
This is just a nit, but the error messages in this file are inconsistent. Sometimes they are full sentences (capitalized, ending with a dot), sometimes they are not (not-capitalized, no dot at the end) and in this particular case it's a mixture (not-capitalized, ending with a dot, although it seems like dot here is only used as a separator). I don't have a strong preference for one style over the other, but I think we should pick one and be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932