[PATCH] D86137: Add ignore-unknown-options flag to clang-format.
Jake Merdich via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 7 08:07:07 PDT 2020
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
More information about the cfe-commits
mailing list