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

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

I can see the use of this, but I am also wary that ignoring style options will lead to people producing different results on different versions of clang-format. This is both because having set-or-unset an option will naturally lead to different code and also that newer options are a de-facto check that clang-format is at least a certain version (we have minor differences between major versions as bugs are fixed). In any case, I see this being *very* easy to misuse and the documentation should have a warning reflecting that.

As far as docs go, the bulk are in clang/docs/ClangFormat.rst and require no additional 'publish' step but definitely should be updated. The command line options are implicitly generated for the cli tool's --help flag, but the docs for them are all in that rst file and manually maintained.



================
Comment at: clang/tools/clang-format/ClangFormat.cpp:108
+static cl::opt<bool>
+    IgnoreUnkownOptions("ignore-unknown-options",
+                        cl::desc("If set, unknown format options are ignored."),
----------------
fodinabor wrote:
> MyDeveloperDay wrote:
> > feels like a mouthful is there nothing shorter we could use?  -Wignore (or something)
> hmm... `-Wunknown`
> but the `-W` does not really make it clear that the default "errors" should now be treated as warnings instead. From compiler conventions, I'd expect the `-W` to enable a warning ... 
> 
> and something like `-Wno-error=unknown` is not really shorter...
I personally like -Wno-error=unknown if the behavior is to emit warnings and -Wno-unknown if the behavior is to be silent, for consistency with clang/gcc.

I would also put a note in the description saying that ignoring options can lead to dramatically different output between versions that do and don't support a given option, so it should be used with care.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:199
+  if (IgnoreUnkown)
+    return;
   for (const auto &NN : MN->Mapping) {
----------------
grimar wrote:
> 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.
> 
Something like how clang/gcc only report unknown -Wno-foo options if there's another error is an idea, but clang-format almost never fails unless there's a bad config or input file so that's not too useful.

I'm fine with either warning or being silent. If the user has opted-into ignoring missing options, we can assume they're willing to accept the consequences of such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137