[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

[George Rimar via Phabricator via cfe-commits]

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