Currently newer clang-format options cannot be included in .clang-format files, if not all users can be forced to use an updated version.
This patch tries to solve this by adding an option to clang-format, enabling to ignore unknown (newer) options.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This has caught me out from time to time, but the presence of such an option can lead to users mixing clang-format versions which could lead to flip/flopping of changes, so I'm not 100% sure it won't drive bad behaviour.
However breaking based on the option can be a pain if the code you are formatting isn't even using the new option
I wonder is it possible to do this without impacting the LLVM support libraries?
I see the possible issue with the possible version mismatches and that is why I'd make people opt-in for this option, to still being able to format their files (e.g. if using out-dated built-in versions like in Visual Studio - I know you can specify your own binary) but the real formatting can be done with a pre-commit script or similar, where the new options are actually supported.
Regarding not touching the LLVM support library: I'd love to find a way, but as clang-format uses the >> operator to read the YAML and this operator automatically emits the errors, I don't see any other obvious way.. (maybe there's a LLVM trick to suppress those errors that I haven't seen, yet :))
Regarding not touching the LLVM support library: I'd love to find a way, but as clang-format uses the >> operator
We need to find some reviewers who look after a wider area of LLVM, if you want to make a change out there in LLVM/support, but it above my pay grade to approve ;-) maybe one of the more experienced devs could help identify the correct person @aaron.ballman @sammccall or alternatively take a look via git log as to who has been maintaining that file @JDevlieghere, they may have an opinion about how this should be done.
clang/tools/clang-format/ClangFormat.cpp | ||
---|---|---|
108 | feels like a mouthful is there nothing shorter we could use? -Wignore (or something) | |
llvm/include/llvm/Support/YAMLTraits.h | ||
792 | I'm not a massive fan of functions with out parameter names, but I see your following the local style. | |
1523 | is this clang-formatted? | |
llvm/lib/Support/YAMLTraits.cpp | ||
203 | do we want to flat out ignore or just report but not fatally. (just a thought) silent failures are hard to diagnose |
Thank you so far for the feedback!
maybe you can give further guidance on the comments on the comments :)
As of the git history it seems that @grimar did some work on YAML error handling..
clang/tools/clang-format/ClangFormat.cpp | ||
---|---|---|
108 | hmm... -Wunknown and something like -Wno-error=unknown is not really shorter... | |
llvm/include/llvm/Support/YAMLTraits.h | ||
1523 | the patch is... the original alignment of the members is not. (also see the CurrentNode's formatting). | |
llvm/lib/Support/YAMLTraits.cpp | ||
203 | 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. |
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.
llvm/include/llvm/Support/YAMLTraits.h | ||
---|---|---|
1511 | I'd add a parameter name here. Seems the style is mixed in this file, Probably the same applies for setIgnoreUnknown in the base class. | |
llvm/lib/Support/YAMLTraits.cpp | ||
203 | Yes, I think we might want to introduce a method, like Input::reportWarning | |
758 | You don't actually need it for Output, right? |
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.
clang/tools/clang-format/ClangFormat.cpp | ||
---|---|---|
108 | 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. | |
llvm/lib/Support/YAMLTraits.cpp | ||
203 | 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. |
Incorporating review comments:
- renaming option to -Wno-error=unknown and adding warning in description
- emit warnings instead of fully ignoring the issues
Documentation and unit tests will follow
llvm/lib/Support/YAMLTraits.cpp | ||
---|---|---|
204–210 | Please don't use auto when the variable type is not obvious. | |
205 | The same here, but also the result type here is Twine, and you shouldn't use it like this. Documentation says: You can probably inline it or use std::string, since this is a error path only code. | |
387 | Why do you need both reportWarning methods? I think you can have only one for now. |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16079 ↗ | (On Diff #291405) | It looks like it is very similar to "Test 7:" and can be a part of it? ... llvm::consumeError(Style7a.takeError()); // <comment> auto Style7b = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true); ASSERT_TRUE((bool)Style8); |
llvm/unittests/ObjectYAML/YAMLTest.cpp | ||
39 ↗ | (On Diff #291405) | UnknownOption |
40 ↗ | (On Diff #291405) | std::string->StringRef? |
41 ↗ | (On Diff #291405) | Will this work if we switch the order of InvalidKey and Binary? std::string InputYAML = "InvalidKey: InvalidValue\n" "Binary: AAAA"; I expect that yes and I think it is reasonable to do it to show that we don't stop parsing an YAML |
44 ↗ | (On Diff #291405) | No full stop at the end. |
57 ↗ | (On Diff #291405) | Do you need BH_GT? Can it be just: // test 2: only warn about invalid key if actively set. llvm::yaml::Input Input2(InputYAML); BinaryHolder BH2; Input2.setAllowUnknownKeys(true); Input2 >> BH2; EXPECT_EQ(Input2.error().value(), 0); EXPECT_EQ(BH2.Binary, yaml::BinaryRef("AAAA")); |
LGTM. It worth wainting for a second approvement and/or other comments to verify that people are happy with doing this for clang-format.
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16062 ↗ | (On Diff #291826) | nit: since you have Style7b, this perhaps should be Style7a. |
Fix the nit :)
Anyone with access may commit this now (ideally ofc someone with a final opinion on the clang-format part).
IMHO I feel that given that these changes are more wide reaching than just clang-format that it might be more correct for your to request commit access yourself.
You need to own these changes, because if the llvm/Support changes fail the build or unit tests the person who commits it is going to have to resolve those issues.
It doesn't take long to request commit access.
Okay, I requested commit access.
So the patch is fine with you from a clang-format perspective?
it certainly LGTM, like I said I've had met this issue before and I think this does get around that situation where perhaps your not even using the option it's complaining about (especially if its a C++ option and you are using clang-format for C#)
So yes I think it's good. Thank you for the patch and getting the commit access.
When you commit don't forget to keep an eye on the build_bots at http://lab.llvm.org:8011/ this was something I didn't learn about until after my first commit... (to my cost)
I think the = in the option name is confusing the option parser,
➜ clang-format --Wno-error=unknown clang-format: Unknown command line argument '--Wno-error=unknown'. Try: 'clang-format --help' clang-format: Did you mean '--Werror=unknown'?
Should the name be changed to Wno-error-unknown?
I can reproduce the issue, but not sure what introduced this.
I don't think this should be the solution, as we aimed for the common syntax using the -Wno-error= style.
I just tried to prepare a patch, changing the way, this is handled, it works already in theory, but the formatting of the --help page is still slightly off (I guess cl::values descriptions are supposed to be single line), so I guess I have to investigate my formatting options here or move the more extensive description somewhere else... if I don't find an obvious solution, I guess we can discuss that after I created the patch..
Clang-format options: --Werror - If set, changes formatting warnings to errors --Wno-error=<value> - If set don't error out on the specified warning type. =unknown - If set, unknown format options are only warned about. This can be used to enable formatting, even if the configuration contains unknown (newer) options. Use with caution, as this might lead to dramatically differing format depending on an option being supported or not. --assume-filename=<string> - Override filename used to determine the language. When reading from stdin, clang-format assumes this filename to determine the language. --cursor=<uint> - The position of the cursor when invoking ...
Btw. is there a way to add tests for command line options, so this won't happen unnoticed again?
feels like a mouthful is there nothing shorter we could use? -Wignore (or something)