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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
