Differential D144709
[clang-format] Improve QualifierAlignment Authored by AlexanderHederstaf on Feb 24 2023, 1:18 AM.
Details Qualifiers were not moved for non-pointer non-simple types. Fixes https://github.com/llvm/llvm-project/issues/57154 and
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Highlight by me. That is not acceptable, running the output of clang-format through clang-format shall not result in any change. In that case it's better to not touch it in any way.
Comment Actions
That is no longer an issue in the latest patch. Comment Actions just as a heads up, I'm applying https://reviews.llvm.org/D144537 since it's a quick bug fix; this patch *would* replace that one, but may need more time. (also conflicts may appear) Comment Actions Thanks for the review so far! Passes all tests and works in one shot on my test files as well as 3k+ files repository, i.e. it does not have to be run twice. As one use-case of this rule is to be able to checkout code and freely swap between left/right. Then the left alignment would also need to handle the same cases as right alignment. I should also make templates with requires work, though I am not familiar with the variations that syntax may have. @rymiel Good, this might take a few more iterations.
Comment Actions when I originally did this work, I checked out the whole fresh area of LLVM and applied the both east then back to west const and built both to ensure I didn't break anything. Please ensure you do the same, I'm going to be honest you are changing it massively, I'm going to need some time to understand the changes. Comment Actions Great! Please mark comments as done. From my point this looks okay, but I did not look very deeply into it, I'll let @MyDeveloperDay do that.
Comment Actions Rebase after change to pointer to member was merged. Add commented-out tests for two cases where right to left Comment Actions Fixed comments and rebased. Tested right to left on the output of left to right and discovered two cases where the output code would not compile. Added new tests for those cases and I am working on improvements for right to left.
Comment Actions Update right to left analyzer Add new, and uncomment old tests Comment Actions There are a lot of complicated cases when trying to extend this functionality. I addressed a few more and thought that it might be acceptable to avoid adjusting the qualifiers for variable declared as struct Foo const; -> const struct Foo;
const struct Foo; -> struct Foo const;
const struct {
int a;
} foo = {5};
->
struct {
int a;
} const foo = {5};as they are difficult to distinguish from const struct Foo {
int a;
} foo = {5};where this is not valid code struct const Foo {
int a;
} foo = {5};
struct Foo const {
int a;
} foo = {5};I have still not tested left->right->left on the llvm codebase, I'll update you when it's done. Comment Actions Simplify insertQualifierAfter. Comment Actions Discovered incorrect code generation for llvm/include/llvm/ADT/StringMap.h. using base = StringMapIterBase<StringMapConstIterator<ValueTy>,
const StringMapEntry<ValueTy>>;was converted to, removing two characters. I checked the Token length in InsertQualifierAfter and found that it was not 1. How should I proceed with this particular issue @MyDeveloperDay using base = StringMapIterBase<StringMapConstIterator<ValueTy>,
StringMapEntry<ValueTy> constI commited the change to SetLength(1) but that looks a bit much like a hack, perhaps anyone more familiar with the codebase can have a look at this issue. Comment Actions As tests currently fail can you set it to "planned changes" until they pass [ FAILED ] 5 tests, listed below: [ FAILED ] QualifierFixerTest.RightQualifier [ FAILED ] QualifierFixerTest.LeftQualifier [ FAILED ] QualifierFixerTest.ConstVolatileQualifiersOrder [ FAILED ] QualifierFixerTest.MoveConstBeyondType [ FAILED ] QualifierFixerTest.TemplatesRight Comment Actions This code from llvm/tools/llvm-exegesis/lib/SerialSnippetGenerator.cpp failed with Right but without a full list of specifiers. As static was not configured, the } was not discovered by IsRightQualifier. Changed some of the qualifier tests to check against the full list instead. struct ExecutionClass {
ExecutionMode Mask;
const char *Description;
} static const kExecutionClasses[] = {
{ExecutionMode::ALWAYS_SERIAL_IMPLICIT_REGS_ALIAS |
ExecutionMode::ALWAYS_SERIAL_TIED_REGS_ALIAS,
"Repeating a single implicitly serial instruction"},
{ExecutionMode::SERIAL_VIA_EXPLICIT_REGS,
"Repeating a single explicitly serial instruction"},
{ExecutionMode::SERIAL_VIA_MEMORY_INSTR |
ExecutionMode::SERIAL_VIA_NON_MEMORY_INSTR,
"Repeating two instructions"},
};Resulted in } static kExecutionClasses const[] = {Which is now fixed. This struct will be left unchanged as with other structs. I used clang-format on all files in clang/ and llvm/, where I had added QualifierAlignment: Right to any .clang-format. I built with cmake -S llvm -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang" cmake --build build Is that sufficient or did you imagine some other build? Comment Actions Change order for typename and :: as Add code for handling ::template. Comment Actions
Performed the test for Comment Actions Right down here where you post your reply. The "Add Action..." has "Plan Changes" to set the state. Comment Actions Firstly let me say thank you for this review, your perseverance demonstrates to me that there is a desire to make this capability better, which tells me that a feature that ultimately took 1+ year to get landed in the first place was perhaps worth it. My hope is that these changes will help to further demonstrate that its ok for clang-format to make modifications (which was a major discussion point at the time it first landed) From a personal perspective I feel you've heavily changed my original implementation quite a bit. ( Its ok I'm not precious ), but I feel you've commented this code nicely and I feel that likely this is now easier to read. For which I thank you. I also appreciate the extensive testing and the efforts you've made to fix other issue that were ultimately contained originally. I'm inclined to accept the revision , get it into trunk so it can live for sometime before the next branch. (i.e. it wouldn't land in mainstream releases until 17.0 which could be ~6 months away) Comment Actions Thanks for the comments and help on this review. There are a lot of special cases I had not considered before, and while it works on the clang/ and llvm/ subfolders as well as tests and the other repository I tested on, I suppose there could still be some hidden issues. Allowing any identifier without the requirement that it is a pointer type adds a lot of complexity. It sounds good to close the existing issues, let this version live some time and watch out for new issues. Comment Actions This patch also fixes: https://github.com/llvm/llvm-project/issues/56111, https://github.com/llvm/llvm-project/issues/58696, and https://github.com/llvm/llvm-project/issues/54730 (some of those could probably be made duplicates but it seems they're all going down in one blow anyway) +1, I certainly wouldn't have such perseverance, which is why I stick to bug reports
Comment Actions Missed right->left in the fix. Also looking a bit at comments, e.g. ns::/*ns2::*/vector v; Comment Actions I might have been tempted to handle the comments in a separate commit, I don't like to do too much in one go or the review gets hard to manage. Comment Actions I had the same thought but as the comments change is not only an extension of the capability I included it to be safe. Without the change it would lead to code that doesn't compile. Comment Actions If you have commit access you can go ahead and commit, if not and you want one of us to do it, we need your name and email address Comment Actions I don't have commit access. Please go ahead if you'd wish to do it for me. Should I share my email in some other forum than posting it in this comment section? Comment Actions It will be public on GitHub (and any fork), so you should choose one you are comfortable to share here. ;) Comment Actions Okay thanks. If the + options works let's go with Thanks again for the review and answers. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||