[PATCH] D53482: Add clang-format stability check with FormatTests

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

As @krasimir said we wouldn't normally commit tests which just break as they'd fail on the buildbot and someone would revert your change within 24 hours.. however, what you have here raises an interesting conversation

This bug of instability is often raised, I've even seen it myself with my own code and I normally see new bugs coming in about this on a regular basis

https://bugs.llvm.org/show_bug.cgi?id=43845
https://bugs.llvm.org/show_bug.cgi?id=42509

Firstly those developers who have clang-formated integrated into say vim,emac,vs,vscode with format on save likely never see this because they often format multiple times during their development and its likely those instabilities iron themselves out pretty quickly, but this does show up for those people who perhaps only do a final git clang-format before committing or who are reformatting a largely unformatted code base, then the CI system likely complains on commit if you have a check.

It would be good to investigate the root causes, but my experience seems to be that it often feels related to the alignment rules and the positioning of trailing comments. (at least that my experience)

It would also be good to understand if this is always solved with a second run or does it sometimes require more than two runs, or has anyone seen a case where the format flip-flops between 2 styles.

Ultimately fixing the alignment routines to try and make them completely stable with regard to future downstream rules, I believe would simply make them individually more complex and unstable if new rules were added.

It might be worth simply rerunning the specific alignXXX routine again on the subsequent changes.

I'd also be interested to understand if any real thought was put into the order of these routines? Or if just conceptually if you are trying to align declarations, assignments and trailing comments something has to go first and you don't know where the best place is until someone takes a first stab.

  llvm::sort(Changes, Change::IsBeforeInFile(SourceMgr));
  calculateLineBreakInformation();
  alignConsecutiveMacros();
  alignConsecutiveDeclarations();
  alignConsecutiveAssignments();
  alignTrailingComments();
  alignEscapedNewlines();
  generateChanges();

I'd would like to think there was a more elegant approach than just running clang-format twice! one solution might be to simply rerun the whole of reformat() function more than once. Of course, we have to keep one eye on performance because for large files clang-format can already be a little slow.

But running reformat() wouldn't be 2x because we definitely don't need to rerun the sort includes twice and we don't need to reannotate the tokens, detemine the spaces between or  apply the replacements twice (+ no startup costs, dll loading, code rewriting), so I actually think a second pass of reformat might not be terrible. (maybe undesirable but not terrible)

What if we experiment with a `-stable` command line switch which simply reruns those parts of reformat that are causing the issues? and then measure the impact to performance?


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

https://reviews.llvm.org/D53482