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
Unit Tests Event TimelineComment Actions I tried to refactor QualifierAlignmentFixer to handle more types, notably removing the *,&,&& requirement as e.g. const Foo did not work. To avoid moving qualifiers too far, a test for "already east const" is introduced. However, to get the correct order for the east qualifiers Clang-Format must be used twice. I assume it's related to how fixes/replacements are handled. Would you like to have a look and perhaps suggest any improvements? As it must be used twice, some tests related to volatile/const order fail, but I noticed most of them would also fails previously if the type is not simple. Comment Actions Can you please add unit tests for the cases you added, can you please add a link to the github issue you are trying to resolve, or log one if there isn't one. Also all the existing tests need to pass, otherwise its a mute point. When I added this, I knew that there were limitations, there is just sometimes things that can't be determined and so to air on the side of caution we don't fixup those but please bring specific examples to the table so we can review them 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. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
In the initial commit concerns were raised that East and West are westcentric terms and should not be used. Thus you should stick here to Left and Right the same terms we use in the configuration.