grimar added a comment. I am not familar with `clang-format`, but have a few comments inlined about the rest. I think the new `setIgnoreUnknown` YAMLlib API is probably OK generally. I'd perhaps call it differently, e.g. `setAllowUnknownKeys` though.
Also, I think you need to add a unit testing for the new functionality. It seems appropriate places are: `clang\unittests\Format\FormatTest.cpp` (to test clang-format) and `llvm\unittests\ObjectYAML\YAMLTest.cpp` (to test new YAML `Input::setIgnoreUnknown` API) I'll add people who might have something useful to say too. ================ Comment at: llvm/include/llvm/Support/YAMLTraits.h:1508 + void setIgnoreUnknown(bool) override; + ---------------- I'd add a parameter name here. Seems the style is mixed in this file, but it is a public member, and having no names for arguments actually reads bad I think. Not sure why it was done initially. Probably the same applies for `setIgnoreUnknown` in the base class. ================ Comment at: llvm/lib/Support/YAMLTraits.cpp:199 + if (IgnoreUnkown) + return; for (const auto &NN : MN->Mapping) { ---------------- fodinabor wrote: > MyDeveloperDay wrote: > > do we want to flat out ignore or just report but not fatally. (just a > > thought) silent failures are hard to diagnose > true.. don't know what's the best option? > > keep it as a printed out error and just don't return an error code on exit? > This option would make it a clang-format only change, but feels really dirty. > > Otherwise I'd have to dig my way through to `llvm::yaml::Stream::printError` > (or maybe rather add a `printWarning`) to conditionally change the message > type for the ignore case. Yes, I think we might want to introduce a method, like `Input::reportWarning` which will call ` Strm->printError(node, message);`, but will not set a `EC`. Also, it should print "warning: ..." instead of "error: ..." prefix somehow. ================ Comment at: llvm/lib/Support/YAMLTraits.cpp:742 +void Output::setIgnoreUnknown(bool Value) {} + ---------------- You don't actually need it for `Output`, right? I think instead you can have a default implementation in the base class, which should call `llvm_unreachable`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86137/new/ https://reviews.llvm.org/D86137 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits