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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D53482: Add... Wouter van Oortmerssen via Phabricator via cfe-commits
    • [PATCH] D53482... MyDeveloperDay via Phabricator via cfe-commits
    • [PATCH] D53482... Wouter van Oortmerssen via Phabricator via cfe-commits
    • [PATCH] D53482... MyDeveloperDay via Phabricator via cfe-commits

Reply via email to