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

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:204
     if (!is_contained(MN->ValidKeys, NN.first())) {
-      setError(NN.second.get(), Twine("unknown key '") + NN.first() + "'");
-      break;
+      auto ReportNode = NN.second.get();
+      auto ReportMessage = Twine("unknown key '") + NN.first() + "'";
----------------
Please don't use `auto` when the variable type is not obvious.
Use the real type name instead.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:205
+      auto ReportNode = NN.second.get();
+      auto ReportMessage = Twine("unknown key '") + NN.first() + "'";
+      if (!AllowUnknownKeys) {
----------------
The same here, but also the result type here is `Twine`, and you shouldn't use it like this. Documentation says:
"A Twine is not intended for use directly and should not be stored, its implementation relies on the ability to store pointers to temporary stack objects which may be deallocated at the end of a statement. Twines should only be used accepted as const references in arguments, when an API wishes to accept possibly-concatenated strings."
(https://llvm.org/doxygen/classllvm_1_1Twine.html#details)

You can probably inline it or use `std::string`, since this is a error path only code.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:387
+
+void Input::reportWarning(Node *node, const Twine &message) {
+  Strm->printError(node, message, SourceMgr::DK_Warning);
----------------
Why do you need both `reportWarning` methods? I think you can have only one for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137